Re: [pbs-devel] [PATCH v4 proxmox-backup] client: pxar: fix race in pxar backup stream

[Christian Ebner <c.ebner@proxmox.com>]

On 1/24/25 10:20, Christian Ebner wrote:
> On 1/24/25 09:22, Fabian Grünbichler wrote:
>> On December 7, 2024 12:07 pm, Christian Ebner wrote:
>>> Fixes a race condition where the backup upload stream can miss an
>>> error returned by pxar::create_archive, because the error state is
>>> only set after the backup stream was already polled.
>>>
>>> On instantiation, `PxarBackupStream` spawns a future handling the
>>> pxar archive creation, which sends the encoded pxar archive stream
>>> (or streams in case of split archives) through a channel, received
>>> by the pxar backup stream on polling.
>>>
>>> In case this channel is closed as signaled by returning an error, the
>>> poll logic will propagate an eventual error occurred during pxar
>>> creation by taking it from the `PxarBackupStream`.
>>>
>>> As this error might not have been set just yet, this can lead to
>>> incorrectly terminating a backup snapshot with success, eventhough an
>>> error occurred.
>>>
>>> To fix this, introduce `ArchiverState` to hold a finish flag as well
>>> as the error and add a notification channel, allowing the archiver
>>> future to signal the waiting stream. As the notification waiter will
>>> block on subsequent polls even if it has already been notified about
>>> the archive creation finish, or it might not have been registered
>>> just yet when the notification was send out, only block and wait for
>>> notifications if the finished flag in the `ArchiverState` is not set.
>>> If it is set, there is no need to wait for a notification, as the
>>> archiver is finished for sure.
>>>
>>> In case of premature termination of the pxar backup stream, no
>>> additional measures have to been taken, as the abort handle already
>>> terminates the archive creation.
>>>
>>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>>> ---
>>> changes since version 3:
>>> - fix a possible deadlock encountered during further testing by
>>>    strictly limiting the archiver state's mutex lock scope.
>>>
>>>   pbs-client/src/pxar_backup_stream.rs | 61 +++++++++++++++++++++-------
>>>   1 file changed, 47 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/pbs-client/src/pxar_backup_stream.rs b/pbs-client/src/ 
>>> pxar_backup_stream.rs
>>> index 2bfb5cf29..3fb1927d0 100644
>>> --- a/pbs-client/src/pxar_backup_stream.rs
>>> +++ b/pbs-client/src/pxar_backup_stream.rs
>>> @@ -11,6 +11,7 @@ use futures::stream::Stream;
>>>   use nix::dir::Dir;
>>>   use nix::fcntl::OFlag;
>>>   use nix::sys::stat::Mode;
>>> +use tokio::sync::Notify;
>>>   use proxmox_async::blocking::TokioWriterAdapter;
>>>   use proxmox_io::StdChannelWriter;
>>> @@ -30,7 +31,13 @@ pub struct PxarBackupStream {
>>>       rx: Option<std::sync::mpsc::Receiver<Result<Vec<u8>, Error>>>,
>>>       pub suggested_boundaries: Option<std::sync::mpsc::Receiver<u64>>,
>>>       handle: Option<AbortHandle>,
>>> -    error: Arc<Mutex<Option<Error>>>,
>>> +    archiver_state: Arc<Mutex<ArchiverState>>,
>>> +    archiver_finished_notification: Arc<Notify>,
>>
>> I am not sure I follow this change.. wouldn't just having the error and
>> the notification be enough?
> 
> If I recall correctly, the issue here was that one stream can block 
> forever without this in case of split pxar archives.
> The reason being, that it will not be notified of notifications already 
> send out by the archiver before the stream registered to receive 
> notifications.
> So by setting the finished flag in the state, one can avoid to even 
> register and block forever.
> 
>>
>> if we encounter an error during stream procession, we can immediately
>> abort. if the stream is finished, we check for errors, wait for the
>> notification, check for errors again?
>>
>> if we have one Notify per stream, then every stream must either see an
>> error, or get the notification. no more race (provided any encountered
>> error is always set before notifying) and no risk for waiting forever
>> either ;)
> 
> As stated above the archiver might send out the finished notification 
> before the stream registers to be notified, never getting any 
> notification and blocking forever.

Ah no, I see you are right. My statements above are only true for 
`notify_waiters`
https://docs.rs/tokio/latest/tokio/sync/struct.Notify.html#method.notify_waiters, 
which only ever notifies already waiting tasks...

`notify_one` indeed stores the permit and the next task asking to be 
notified will either get the permit or wait for the notification to be 
send. So this indeed needs 2 different `Notify` instances for the 2 
different streams to be notified and does not need the `finished` flag 
at all, as you correctly stated.

Will send a new version for this, thanks!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel