Re: [RFC PATCH proxmox-backup 2/4] fix #7908: datastore: create zero-byte markers when re-using datastore

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

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 <s.shaji@proxmox.com>
> ---
>   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<Self>) -> Result<(), Error> {
> +    pub async fn s3_refresh(
> +        self: &Arc<Self>,
> +        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<Valu
>           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<Valu
>   
>   /// 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(())
>           },