Re: [PATCH proxmox-backup 4/9] datastore: add method to create zero-byte chunk markers

[Christian Ebner <c.ebner@proxmox.com>]

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 <s.shaji@proxmox.com>
> ---
>   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<Self>) -> Result<(), Error> {