From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id AC7FF1FF15F for ; Mon, 18 Nov 2024 16:48:28 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 48C0714347; Mon, 18 Nov 2024 16:48:33 +0100 (CET) From: Christian Ebner To: pbs-devel@lists.proxmox.com Date: Mon, 18 Nov 2024 16:47:42 +0100 Message-Id: <20241118154742.376039-1-c.ebner@proxmox.com> X-Mailer: git-send-email 2.39.5 MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.031 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pbs-devel] [PATCH v2 proxmox-backup] client: pxar: fix race in pxar backup stream X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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, signal the end of the archive creation to the pxar backup stream via a notification. 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 --- changes since version 1: - improved variable naming, distinguish notification sender and notified receiver pbs-client/src/pxar_backup_stream.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pbs-client/src/pxar_backup_stream.rs b/pbs-client/src/pxar_backup_stream.rs index 4370da6cc..6005a8d1e 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,6 +31,7 @@ pub struct PxarBackupStream { pub suggested_boundaries: Option>, handle: Option, error: Arc>>, + archive_finished_notification: Arc, } impl Drop for PxarBackupStream { @@ -79,6 +81,8 @@ impl PxarBackupStream { let error = Arc::new(Mutex::new(None)); let error2 = Arc::clone(&error); + let pxar_backup_stream_notifier = Arc::new(Notify::new()); + let archive_finished_notification = pxar_backup_stream_notifier.clone(); let handler = async move { if let Err(err) = crate::pxar::create_archive( dir, @@ -100,6 +104,8 @@ impl PxarBackupStream { let mut error = error2.lock().unwrap(); *error = Some(err); } + // Notify upload stream that archiver is finished (with or without error) + pxar_backup_stream_notifier.notify_one(); }; let (handle, registration) = AbortHandle::new_pair(); @@ -111,6 +117,7 @@ impl PxarBackupStream { suggested_boundaries: None, handle: Some(handle.clone()), error: Arc::clone(&error), + archive_finished_notification: archive_finished_notification.clone(), }; let backup_payload_stream = payload_rx.map(|rx| Self { @@ -118,6 +125,7 @@ impl PxarBackupStream { suggested_boundaries: suggested_boundaries_rx, handle: Some(handle), error, + archive_finished_notification, }); Ok((backup_stream, backup_payload_stream)) @@ -151,6 +159,10 @@ impl Stream for PxarBackupStream { match proxmox_async::runtime::block_in_place(|| self.rx.as_ref().unwrap().recv()) { Ok(data) => Poll::Ready(Some(data)), Err(_) => { + // Wait until archiver signals finished to catch eventual errors + proxmox_async::runtime::block_in_place(|| { + self.archive_finished_notification.notified() + }); let mut error = self.error.lock().unwrap(); if let Some(err) = error.take() { return Poll::Ready(Some(Err(err))); -- 2.39.5 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel