From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 6CDB61FF0B2 for ; Fri, 25 Sep 2026 14:11:59 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 3CDBC216A3; Fri, 25 Sep 2026 14:11:59 +0200 (CEST) Message-ID: <9f98a194-bf1e-4f94-a14f-2a1596445a9f@proxmox.com> Date: Fri, 25 Sep 2026 14:11:54 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Christian Ebner Subject: Re: [RFC PATCH proxmox-backup 2/4] fix #7908: datastore: create zero-byte markers when re-using datastore To: Shan Shaji , pbs-devel@lists.proxmox.com References: <20260915155935.135460-1-s.shaji@proxmox.com> <20260915155935.135460-3-s.shaji@proxmox.com> Content-Language: en-US, de-DE In-Reply-To: <20260915155935.135460-3-s.shaji@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790338314866 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.619 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: R6PS4KA63EBHR4GCSLIU3OR2OQKLXGYN X-Message-ID-Hash: R6PS4KA63EBHR4GCSLIU3OR2OQKLXGYN X-MailFrom: c.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 9/15/26 6:00 PM, Shan Shaji wrote: > Previously, recreating an S3-backed datastore with the --reuse-datastore > flag fetched only the metadata files into the local cache, without > creating chunk markers. Garbage collection (GC) had to be run to > recreate markers for chunks referenced by the backup indexes. > > In order to fix this, create local zero-byte chunk markers by listing > the chunk objects in S3 when reusing a datastore. This can only work if the whole series depends on [0], so the datastore config lock is not held during the whole listing and chunk marker creation, which depending on the number of chunks can take a very long time. The current s3 refresh and other operations which keep the config lock for too long were already problematic, but this patch here would greatly escalate the problem. [0] https://lore.proxmox.com/pbs-devel/20260803090747.265683-1-c.ebner@proxmox.com/T/#t > Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7908 > > Signed-off-by: Shan Shaji > --- > pbs-datastore/src/datastore.rs | 35 +++++++++++++++++++++++++++++++++- > src/api2/admin/datastore.rs | 10 +++++++--- > src/api2/config/datastore.rs | 6 +++++- > 3 files changed, 46 insertions(+), 5 deletions(-) > > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs > index f3eab8a28..d00a0ef1c 100644 > --- a/pbs-datastore/src/datastore.rs > +++ b/pbs-datastore/src/datastore.rs > @@ -3171,9 +3171,38 @@ impl DataStore { > *OLD_LOCKING > } > > + fn create_empty_s3_chunk_markers( > + &self, > + worker: &dyn WorkerTaskContext, > + s3_client: &S3Client, > + ) -> Result<(), Error> { > + let _guard = self.inner.chunk_store.mutex().lock().unwrap(); Holding the chunk store lock for the whole time here is only acceptable since the whole s3-refresh runs protected by it's maintenance mode which grants it exclusive access anyways, so that warrants a dedicated comment. And the helper should document that requirement as well. Further, this would greatly benefit from some progress logging, which is still lacking for phase 2 of GC on S3 backend as well. Log flooding can be avoided by using e.g. the log throttle implementation recently introduced by Thomas [0]. [0] https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=pbs-client/src/log_throttle.rs;h=cfaa6c4948a01b7878c04cce08081880891b8a47;hb=709be1a29c2797a320b01be93fa9befcbd51a203 > + > + self.paginate_s3_chunk_objects(worker, s3_client, |object_key, _| -> Result<(), Error> { > + let (_, digest, _) = match digest_from_object_key(&object_key) { > + Some(result) => result, > + None => return Ok(()), > + }; > + > + unsafe { > + self.inner > + .chunk_store > + .replace_chunk_with_marker_or_create_marker(&digest)?; This is not ideal! Any pre-existing, valid locally cached chunk is now cleared (and might still be referenced by the in-memory LRU cache). It would rather make sense to implement a dedicated chunk store helper on top of ChunkStore::create_marker_file() which checks for the file to pre-exist? > + } > + > + Ok(()) > + })?; > + > + Ok(()) > + } > + > /// Fetch contents from S3 object store, clear and replace the local cache store contents. > /// Returns with error for non-S3 datastore backends. > - pub async fn s3_refresh(self: &Arc) -> Result<(), Error> { > + pub async fn s3_refresh( > + self: &Arc, > + rebuild_chunk_markers: bool, > + worker: &dyn WorkerTaskContext, > + ) -> Result<(), Error> { > match self.backend()? { > DatastoreBackend::Filesystem => bail!("store '{}' not backed by S3", self.name()), > DatastoreBackend::S3(s3_client) => { > @@ -3190,6 +3219,10 @@ impl DataStore { > let _ = std::fs::remove_dir_all(&tmp_base); > return Err(err); > } > + > + if rebuild_chunk_markers { > + self.create_empty_s3_chunk_markers(worker, &s3_client)?; > + } > } > } > Ok(()) > diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs > index bc2b2436e..34907fa0a 100644 > --- a/src/api2/admin/datastore.rs > +++ b/src/api2/admin/datastore.rs > @@ -2867,7 +2867,7 @@ pub fn s3_refresh(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result Some(store.clone()), > auth_id.to_string(), > to_stdout, > - move |worker| do_s3_refresh(&store, &worker), > + move |worker| do_s3_refresh(&store, &worker, false), > )?; > > Ok(json!(upid)) > @@ -2875,10 +2875,14 @@ pub fn s3_refresh(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result > /// Performs an s3 refresh for given datastore. Expects the store to already be in maintenance mode > /// s3-refresh. > -pub(crate) fn do_s3_refresh(store: &str, worker: &dyn WorkerTaskContext) -> Result<(), Error> { > +pub(crate) fn do_s3_refresh( > + store: &str, > + worker: &dyn WorkerTaskContext, > + rebuild_chunk_markers: bool, > +) -> Result<(), Error> { > let datastore = DataStore::lookup_datastore(lookup_with(store, Operation::Lookup))?; > run_maintenance_locked(store, MaintenanceType::S3Refresh, worker, || { > - proxmox_async::runtime::block_on(datastore.s3_refresh()) > + proxmox_async::runtime::block_on(datastore.s3_refresh(rebuild_chunk_markers, worker)) > }) > } > > diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs > index e7028480c..e82e8969a 100644 > --- a/src/api2/config/datastore.rs > +++ b/src/api2/config/datastore.rs > @@ -366,7 +366,11 @@ pub fn create_datastore( > } > > if reuse_datastore && backend == DatastoreBackendType::S3 { > - crate::api2::admin::datastore::do_s3_refresh(&store_name, &worker)?; > + crate::api2::admin::datastore::do_s3_refresh( > + &store_name, > + &worker, > + reuse_datastore, > + )?; > } > Ok(()) > },