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 92A511FF165 for ; Thu, 6 Nov 2025 10:37:09 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 281B711CA8; Thu, 6 Nov 2025 10:37:50 +0100 (CET) Date: Thu, 06 Nov 2025 10:37:42 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20251105122233.439382-1-c.ebner@proxmox.com> <20251105122233.439382-19-c.ebner@proxmox.com> In-Reply-To: <20251105122233.439382-19-c.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1762418497.lnn72w5td0.astroid@yuna.none> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762421846470 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.048 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: Re: [pbs-devel] [PATCH proxmox-backup v3 18/23] GC: lock chunk marker before cleanup in phase 3 on s3 backends 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" On November 5, 2025 1:22 pm, Christian Ebner wrote: > To make sure there is no race between atime check and deletion with > possible re-insertion. could you expand on this? sweep_unused_chunks holds the chunk store mutex for the full processing of each chunk it iterates over.. so we have for each chunk: - obtain mutex - fstat - call cond_sweep_chunk - lock chunk - fstat again - drop mutex - remove from cache (which re-obtains mutex) shouldn't we protect this race by obtaining the mutex when conditionally touching the chunk in `insert_chunk_cached()`, or even in `datastore.cond_touch_chunk()`? then there is no way for a chunk insertion to update the atime here.. AFAICT the latter would also close a race with pull syncing, which touches chunks while they are referenced by tmp indices, which means those touches are not protected by the mutex or the rest of the GC logic? > > By only acquiring the file lock if the chunk marker would be removed > and double stating, the file locking penalty is avoided for the other > cases. > > Signed-off-by: Christian Ebner > --- > changes since version 2: > - no changes > > pbs-datastore/src/chunk_store.rs | 28 +++++++++++++++++++++++++++- > pbs-datastore/src/datastore.rs | 2 ++ > 2 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs > index 49687b2fa..08519fe2b 100644 > --- a/pbs-datastore/src/chunk_store.rs > +++ b/pbs-datastore/src/chunk_store.rs > @@ -5,6 +5,7 @@ use std::sync::{Arc, Mutex}; > use std::time::Duration; > > use anyhow::{bail, format_err, Context, Error}; > +use hex::FromHex; > use tracing::{info, warn}; > > use pbs_api_types::{DatastoreFSyncLevel, GarbageCollectionStatus}; > @@ -22,7 +23,7 @@ use crate::data_blob::DataChunkBuilder; > use crate::file_formats::{ > COMPRESSED_BLOB_MAGIC_1_0, ENCRYPTED_BLOB_MAGIC_1_0, UNCOMPRESSED_BLOB_MAGIC_1_0, > }; > -use crate::DataBlob; > +use crate::{DataBlob, LocalDatastoreLruCache}; > > /// File system based chunk store > pub struct ChunkStore { > @@ -366,6 +367,7 @@ impl ChunkStore { > min_atime: i64, > status: &mut GarbageCollectionStatus, > worker: &dyn WorkerTaskContext, > + cache: Option<&LocalDatastoreLruCache>, > ) -> Result<(), Error> { > // unwrap: only `None` in unit tests > assert!(self.locker.is_some()); > @@ -419,6 +421,30 @@ impl ChunkStore { > bad, > status, > || { basically we have two callbacks here: - remove chunk/chunk marker (called for all bad chunks or non S3 case) - remove chunk via cache (called for non-bad S3 chunks) > + if let Some(cache) = cache { > + // never lock bad chunks > + if filename.to_bytes().len() == 64 { this is already passed in as bool ;) > + let digest = <[u8; 32]>::from_hex(filename.to_bytes())?; > + match self.lock_chunk(&digest, Duration::from_secs(0)) { so this is basically a "skip chunk if currently being uploaded" check, right? might warrant a comment, else this reads like a deadlock waiting to happen.. in phase2 we obtain the chunk lock for all chunks on the S3 side outside of the remove callback, here we do it inside - is there a reason for that? chunks ending up in this state here should be rather rare (compared to chunks existing on S3 being garbage collected), right? but even if we keep this, the whole callback could become (AFAICT): || { // non-bad S3 chunks need to be removed via cache if let Some(cache) = cache { if !bad { let digest = <[u8; 32]>::from_hex(filename.to_bytes())?; // unless there is a concurrent upload pending if let Ok(_guard) = self.lock_chunk(&digest, Duration::from_secs(0)) { drop(lock); cache.remove(&digest)?; } return Ok(()); } } // bad or local chunks unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir).map_err( |err| { format_err!( "unlinking chunk {filename:?} failed on store '{}' - {err}", self.name, ) }, ) }, > + Ok(_guard) => { > + // don't remove if changed since locking > + match fstatat( > + Some(dirfd), > + filename, > + nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW, > + ) { > + Ok(stat) if stat.st_atime < min_atime => { > + let _ = cache.remove(&digest); > + return Ok(()); > + } > + _ => return Ok(()), > + } > + } > + Err(_) => return Ok(()), > + } > + } > + } > + > unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir).map_err( > |err| { > format_err!( > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs > index 8b7333d80..df344974a 100644 > --- a/pbs-datastore/src/datastore.rs > +++ b/pbs-datastore/src/datastore.rs > @@ -1761,6 +1761,7 @@ impl DataStore { > min_atime, > &mut tmp_gc_status, > worker, > + self.cache(), > )?; > } else { > self.inner.chunk_store.sweep_unused_chunks( > @@ -1768,6 +1769,7 @@ impl DataStore { > min_atime, > &mut gc_status, > worker, > + None, > )?; > } > > -- > 2.47.3 > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel