From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 84BFF1FF179 for ; Wed, 15 Oct 2025 18:40:39 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9EECFB4C; Wed, 15 Oct 2025 18:40:55 +0200 (CEST) From: Christian Ebner To: pbs-devel@lists.proxmox.com Date: Wed, 15 Oct 2025 18:40:06 +0200 Message-ID: <20251015164008.975591-9-c.ebner@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20251015164008.975591-1-c.ebner@proxmox.com> References: <20251015164008.975591-1-c.ebner@proxmox.com> MIME-Version: 1.0 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1760546418209 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.042 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 proxmox-backup v3 6/8] api: chunk upload: fix race between chunk backend upload and insert 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" Chunk are first uploaded to the object store for S3 backed datastores, only then inserted into the local datastore cache. By this, it is assured that the chunk is only ever considered as valid after successful upload. Garbage collection does however rely on the local marker file to be present, only then considering the chunk as in-use. While this marker is created if not present during phase 1 of garbage collection, this only happens for chunks which are already referenced by a complete index file. Therefore, there remains a race window between garbage collection listing the chunks (upload completed) and lookup of the local marker file being present (chunk cache insert after upload). This can lead to chunks which just finished upload, but were not yet inserted into the local cache store to be removed again. To close this race window, mark chunks which are currently being uploaded to the backend by an additional marker file, checked by the garbage collection as well if the regular marker is not found. The upload marker file is only removed after successful chunk insert or by garbage collection if the atime is older than the cutoff (cleanup in case of failed uploads). Concurrent chunk uploads are still possible, updating the upload marker file as it is then pre-existing. The upload marker file is not removed in case of upload errors, assuring it remains in place for the other upload. Avoid this overhead for regular datastores by only performing these operations on s3 backend datastores. Signed-off-by: Christian Ebner --- pbs-datastore/src/datastore.rs | 25 +++++++++++-------- .../src/local_datastore_lru_cache.rs | 3 ++- src/api2/backup/upload_chunk.rs | 9 ++++--- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs index ed994eb0b..aa34ab037 100644 --- a/pbs-datastore/src/datastore.rs +++ b/pbs-datastore/src/datastore.rs @@ -4,7 +4,7 @@ use std::os::unix::ffi::OsStrExt; use std::os::unix::io::AsRawFd; use std::path::{Path, PathBuf}; use std::sync::{Arc, LazyLock, Mutex}; -use std::time::{Duration, SystemTime}; +use std::time::Duration; use anyhow::{bail, format_err, Context, Error}; use http_body_util::BodyExt; @@ -1658,16 +1658,10 @@ impl DataStore { // Check local markers (created or atime updated during phase1) and // keep or delete chunk based on that. - let atime = match std::fs::metadata(&chunk_path) { - Ok(stat) => stat - .accessed()? - .duration_since(SystemTime::UNIX_EPOCH)? - .as_secs() as i64, - Err(err) if err.kind() == std::io::ErrorKind::NotFound => { - // File not found, delete by setting atime to unix epoch - 0 - } - Err(err) => return Err(err.into()), + let atime = unsafe { + self.inner + .chunk_store + .sweep_chunk_marker_files(&digest, min_atime)? }; let bad = chunk_path @@ -1868,6 +1862,15 @@ impl DataStore { self.inner.chunk_store.insert_chunk(chunk, digest) } + /// Insert a new backend upload marker or update atime if pre-existing, signaling to garbage + /// collection that there is an in-progress upload for this chunk. + /// + /// Returns true if the marker was created or touched, returns false if the chunk has been + /// inserted since, the marker file not being created and the upload must be avoided. + pub fn touch_backend_upload_marker(&self, digest: &[u8; 32]) -> Result { + self.inner.chunk_store.touch_backend_upload_marker(digest) + } + pub fn stat_chunk(&self, digest: &[u8; 32]) -> Result { let (chunk_path, _digest_str) = self.inner.chunk_store.chunk_path(digest); std::fs::metadata(chunk_path).map_err(Error::from) diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs index fe3b51a55..cdad77031 100644 --- a/pbs-datastore/src/local_datastore_lru_cache.rs +++ b/pbs-datastore/src/local_datastore_lru_cache.rs @@ -34,7 +34,8 @@ impl LocalDatastoreLruCache { /// /// Fails if the chunk cannot be inserted successfully. pub fn insert(&self, digest: &[u8; 32], chunk: &DataBlob) -> Result<(), Error> { - self.store.insert_chunk(chunk, digest)?; + self.store + .insert_chunk_and_remove_upload_marker(chunk, digest)?; self.cache .insert(*digest, (), |digest| self.store.clear_chunk(&digest)) } diff --git a/src/api2/backup/upload_chunk.rs b/src/api2/backup/upload_chunk.rs index 64e8d6e63..0640f3652 100644 --- a/src/api2/backup/upload_chunk.rs +++ b/src/api2/backup/upload_chunk.rs @@ -259,6 +259,7 @@ async fn upload_to_backend( data.len() ); } + let datastore = env.datastore.clone(); if env.no_cache { let object_key = pbs_datastore::s3::object_key_from_digest(&digest)?; @@ -272,11 +273,11 @@ async fn upload_to_backend( // Avoid re-upload to S3 if the chunk is either present in the LRU cache or the chunk // file exists on filesystem. The latter means that the chunk has been present in the // past an was not cleaned up by garbage collection, so contained in the S3 object store. - if env.datastore.cache_contains(&digest) { + if datastore.cache_contains(&digest) { tracing::info!("Skip upload of cached chunk {}", hex::encode(digest)); return Ok((digest, size, encoded_size, true)); } - if let Ok(true) = env.datastore.cond_touch_chunk(&digest, false) { + if let Ok(true) = datastore.cond_touch_chunk(&digest, false) { tracing::info!( "Skip upload of already encountered chunk {}", hex::encode(digest) @@ -286,6 +287,9 @@ async fn upload_to_backend( tracing::info!("Upload of new chunk {}", hex::encode(digest)); let object_key = pbs_datastore::s3::object_key_from_digest(&digest)?; + if !datastore.touch_backend_upload_marker(&digest)? { + return Ok((digest, size, encoded_size, true)); + } let is_duplicate = s3_client .upload_replace_on_final_retry(object_key, data.clone()) .await @@ -295,7 +299,6 @@ async fn upload_to_backend( // Although less performant than doing this in parallel, it is required for consisency // since chunks are considered as present on the backend if the file exists in the local // cache store. - let datastore = env.datastore.clone(); tracing::info!("Caching of chunk {}", hex::encode(digest)); let _ = tokio::task::spawn_blocking(move || { let chunk = DataBlob::from_raw(data.to_vec())?; -- 2.47.3 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel