From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 18FCB1FF0B3 for ; Wed, 11 Nov 2026 16:04:55 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 9E89721611; Wed, 11 Nov 2026 16:04:54 +0100 (CET) Message-ID: <3f1efc7a-04ed-4dfa-9c8f-2d2eb9aeaa68@proxmox.com> Date: Wed, 11 Nov 2026 16:04:47 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Christian Ebner Subject: Re: [PATCH proxmox-backup 4/9] datastore: add method to create zero-byte chunk markers To: Shan Shaji , pbs-devel@lists.proxmox.com References: <20261019160359.20851-1-s.shaji@proxmox.com> <20261019160359.20851-5-s.shaji@proxmox.com> Content-Language: en-US, de-DE In-Reply-To: <20261019160359.20851-5-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: 1794409488153 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.419 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: ZVBNDGRFZYLTJIWXEPP4445DCR3ZPTFG X-Message-ID-Hash: ZVBNDGRFZYLTJIWXEPP4445DCR3ZPTFG 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: One comment inline. On 10/19/26 6:04 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, add method to create zero-byte chunk markers by > listing the chunk objects in S3 when reusing a datastore. > > Signed-off-by: Shan Shaji > --- > pbs-datastore/src/datastore.rs | 50 ++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs > index 7bb0c187c..17fa47cc3 100644 > --- a/pbs-datastore/src/datastore.rs > +++ b/pbs-datastore/src/datastore.rs > @@ -13,6 +13,7 @@ use hyper::Method; > use hyper::body::Bytes; > use nix::unistd::{UnlinkatFlags, unlinkat}; > use pbs_tools::lru_cache::LruCache; > +use proxmox_log::log_throttle::{self, LogThrottle}; > use tokio::io::AsyncWriteExt; > use tracing::{info, warn}; > > @@ -3292,6 +3293,55 @@ impl DataStore { > *OLD_LOCKING > } > > + /// The caller must hold the maintenance-mode lock with the datastore in > + /// `S3Refresh` mode. > + pub fn create_empty_s3_chunk_markers( > + &self, > + worker: &dyn WorkerTaskContext, > + ) -> Result<(), Error> { > + let DatastoreBackend::S3(s3_client) = self.backend()? else { > + bail!("store '{}' not backed by S3", self.name()); > + }; > + > + let start = std::time::Instant::now(); > + > + // Holding the chunk store lock for the whole time here is > + // only acceptable since the maintenance mode lock is held > + // during the entire datastore creation. > + let _guard = self.inner.chunk_store.mutex().lock().unwrap(); > + > + let mut chunk_marker_count = 0; > + let mut log_throttle = > + LogThrottle::new(log_throttle::INITIAL_INTERVAL, log_throttle::MAX_INTERVAL); > + > + self.paginate_s3_chunk_objects( > + worker, > + s3_client.as_ref(), > + |chunk_store, object_key, _| -> Result<(), Error> { > + let (_, digest, _) = match digest_from_object_key(&object_key) { > + Some(result) => result, > + None => return Ok(()), > + }; comment: the object keys here do also potentially include bad chunks with their respective suffix, so the markers would be created for these as well, leading to no further good chunks being uploaded and corrupt backups. Further I think there still is a pre-existing issue with the marker files for bad chunks potentially being missing, which would lead to removing these from the S3 backend even if no new, good chunk exists. Above might further be written a bit more concise with the chunk path helper to already check for bad chunk suffix by: ``` - let (_, digest, _) = match digest_from_object_key(&object_key) { - Some(result) => result, - None => return Ok(()), + let Some((_, digest, bad)) = self.chunk_path_from_object_key(&object_key) else { + return Ok(()); ``` > + > + unsafe { > + chunk_store.replace_chunk_with_marker_or_create_marker(&digest)?; > + } > + > + chunk_marker_count += 1; > + if log_throttle.should_log() { > + info!("created {chunk_marker_count} chunk markers"); > + } > + > + Ok(()) > + }, > + )?; > + > + let elapsed = TimeSpan::from(start.elapsed()); > + info!("created {chunk_marker_count} chunk markers in {elapsed}"); > + > + 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> {