[RFC PATCH proxmox-backup 0/4] fix #7903: rebuild local S3 chunk markers

[RFC PATCH proxmox-backup 0/4] fix #7903: rebuild local S3 chunk markers

[Shan Shaji]

Sending this as an RFC to get an initial feedback. I'd really appreciate
any suggestions or pointers to anything I may have missed.

Reusing an S3-backed datastore currently restores the backup metadata to
the local cache without creating local chunk markers. A garbage collection
run is then needed to recreate markers for chunks referenced by the backup
indexes.

This series addresses #7908 by listing the chunk objects in S3 and creating
zero-byte local markers when a datastore is recreated with
--reuse-datastore. It also adds an optional rebuild-chunk-markers parameter
to the s3-refresh API and exposes it through proxmox-backup-manager:

    proxmox-backup-manager datastore s3-refresh <store> --rebuild-chunk-markers true

The option is disabled by default for explicit refreshes. When enabled,
it creates markers for all chunk objects with valid chunk keys, including
chunks not referenced by the restored backup indexes. Existing cached
chunk files are replaced with empty markers.

Shan Shaji (4):
  datastore: factor out s3 chunk objects pagination into a helper
  fix #7908: datastore: create zero-byte markers when re-using datastore
  datastore: api: add option to rebuild S3 chunk markers
  datastore: backup-manager: expose option to rebuild s3 chunk markers

 pbs-datastore/src/datastore.rs              | 120 +++++++++++++++-----
 src/api2/admin/datastore.rs                 |  23 +++-
 src/api2/config/datastore.rs                |   6 +-
 src/bin/proxmox_backup_manager/datastore.rs |   7 ++
 4 files changed, 121 insertions(+), 35 deletions(-)

--
2.47.3

[RFC PATCH proxmox-backup 1/4] datastore: factor out s3 chunk objects pagination into a helper

[Shan Shaji]

This is in preparation to re-use the chunk iteration logic for creating
empty marker files during s3 refresh.

Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 85 ++++++++++++++++++++++------------
 1 file changed, 56 insertions(+), 29 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index c3db52269..f3eab8a28 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -2382,6 +2382,47 @@ impl DataStore {
         }
     }
 
+    /// Fetch chunk objects in batches of max 1000 per request.
+    /// For each objects listed, the `callback` will be invoked with its
+    /// S3 object key and size.
+    fn paginate_s3_chunk_objects<F>(
+        &self,
+        worker: &dyn WorkerTaskContext,
+        s3_client: &S3Client,
+        mut callback: F,
+    ) -> Result<(), Error>
+    where
+        F: FnMut(S3ObjectKey, u64) -> Result<(), Error>,
+    {
+        let prefix = S3PathPrefix::Some(".chunks/".to_string());
+        let mut list_bucket_result =
+            proxmox_async::runtime::block_on(s3_client.list_objects_v2(&prefix, None))
+                .context("failed to list chunk in s3 object store")?;
+
+        loop {
+            for content in list_bucket_result.contents {
+                worker.check_abort()?;
+                worker.fail_on_shutdown()?;
+
+                callback(content.key, content.size)?;
+            }
+
+            // Process next batch of chunks if there are more.
+            if list_bucket_result.is_truncated {
+                list_bucket_result = proxmox_async::runtime::block_on(s3_client.list_objects_v2(
+                    &prefix,
+                    list_bucket_result.next_continuation_token.as_deref(),
+                ))?;
+
+                continue;
+            }
+
+            break;
+        }
+
+        Ok(())
+    }
+
     fn garbage_collection_impl(
         &self,
         worker: &dyn WorkerTaskContext,
@@ -2473,28 +2514,23 @@ impl DataStore {
 
         if let Some(ref s3_client) = s3_client {
             let mut chunk_count = 0;
-            let prefix = S3PathPrefix::Some(".chunks/".to_string());
-            // Operates in batches of 1000 objects max per request
-            let mut list_bucket_result =
-                proxmox_async::runtime::block_on(s3_client.list_objects_v2(&prefix, None))
-                    .context("failed to list chunk in s3 object store")?;
-
             let mut delete_list =
                 S3DeleteList::with_thresholds(S3_DELETE_BATCH_LIMIT, S3_DELETE_DEFER_LIMIT_SECONDS);
-            loop {
-                for content in list_bucket_result.contents {
-                    worker.check_abort()?;
-                    worker.fail_on_shutdown()?;
+
+            self.paginate_s3_chunk_objects(
+                worker,
+                s3_client,
+                |object_key, size| -> Result<(), Error> {
                     let (chunk_path, digest, bad) =
-                        match self.chunk_path_from_object_key(&content.key) {
+                        match self.chunk_path_from_object_key(&object_key) {
                             Some(path) => path,
-                            None => continue,
+                            None => return Ok(()),
                         };
 
                     let timeout = std::time::Duration::from_secs(0);
                     let _chunk_guard = match self.inner.chunk_store.lock_chunk(&digest, timeout) {
                         Ok(guard) => guard,
-                        Err(_) => continue,
+                        Err(_) => return Ok(()),
                     };
                     let _guard = self.inner.chunk_store.mutex().lock().unwrap();
 
@@ -2526,7 +2562,7 @@ impl DataStore {
                                 atime,
                                 min_atime,
                                 oldest_writer,
-                                content.size,
+                                size,
                                 bad,
                                 &mut gc_status,
                                 || {
@@ -2537,15 +2573,15 @@ impl DataStore {
                                             std::fs::remove_file(chunk_path)?;
                                         }
                                     }
-                                    delete_list.push(content.key, _chunk_guard);
+                                    delete_list.push(object_key, _chunk_guard);
                                     Ok(())
                                 },
                             )?;
                         }
                     } else {
                         gc_status.removed_chunks += 1;
-                        gc_status.removed_bytes += content.size;
-                        delete_list.push(content.key, _chunk_guard);
+                        gc_status.removed_bytes += size;
+                        delete_list.push(object_key, _chunk_guard);
                     }
 
                     chunk_count += 1;
@@ -2555,19 +2591,10 @@ impl DataStore {
 
                     // limit pending deletes to avoid holding too many chunk flocks
                     delete_list.conditional_delete_and_drop_locks(s3_client)?;
-                }
-                // Process next batch of chunks if there is more
-                if list_bucket_result.is_truncated {
-                    list_bucket_result =
-                        proxmox_async::runtime::block_on(s3_client.list_objects_v2(
-                            &prefix,
-                            list_bucket_result.next_continuation_token.as_deref(),
-                        ))?;
-                    continue;
-                }
 
-                break;
-            }
+                    Ok(())
+                },
+            )?;
 
             // delete the last batch of objects, if there are any remaining
             delete_list.delete_and_drop_locks(s3_client)?;
-- 
2.47.3

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

[Shan Shaji]

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.

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();
+
+        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)?;
+            }
+
+            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(())
         },
-- 
2.47.3

[RFC PATCH proxmox-backup 3/4] datastore: api: add option to rebuild S3 chunk markers

[Shan Shaji]

Add the optional `rebuild-chunk-markers` flag to the datastore
`s3-refresh` API endpoint. When enabled, the refresh lists all chunk
objects in S3 and recreates their local marker files.

Existing local chunk files are replaced with empty marker files. The
option is disabled by default.

Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
---
 src/api2/admin/datastore.rs | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 34907fa0a..5df5a6b63 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -2835,6 +2835,13 @@ pub async fn unmount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<V
             store: {
                 schema: DATASTORE_SCHEMA,
             },
+            "rebuild-chunk-markers": {
+                description: "Create local chunk marker files for all chunks stored in S3. This \
+                requires listing all chunk objects and may take long time for large datastores.",
+                type: bool,
+                optional: true,
+                default: false,
+            },
         }
     },
     returns: {
@@ -2845,7 +2852,11 @@ pub async fn unmount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<V
     },
 )]
 /// Refresh datastore contents from S3 to local cache store.
-pub fn s3_refresh(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+pub fn s3_refresh(
+    store: String,
+    rebuild_chunk_markers: bool,
+    rpcenv: &mut dyn RpcEnvironment,
+) -> Result<Value, Error> {
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
 
@@ -2867,7 +2878,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, false),
+        move |worker| do_s3_refresh(&store, &worker, rebuild_chunk_markers),
     )?;
 
     Ok(json!(upid))
-- 
2.47.3

[RFC PATCH proxmox-backup 4/4] datastore: backup-manager: expose option to rebuild s3 chunk markers

[Shan Shaji]

Add the optional --rebuild-chunk-markers flag to the datastore
s3-refresh command.

Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
---
 src/bin/proxmox_backup_manager/datastore.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
index 4c1d9daf5..e595f27be 100644
--- a/src/bin/proxmox_backup_manager/datastore.rs
+++ b/src/bin/proxmox_backup_manager/datastore.rs
@@ -330,6 +330,13 @@ async fn uuid_mount(mut param: Value, _rpcenv: &mut dyn RpcEnvironment) -> Resul
             store: {
                 schema: DATASTORE_SCHEMA,
             },
+            "rebuild-chunk-markers": {
+                description: "Create local chunk marker files for all chunks stored in S3. This \
+                requires listing all chunk objects and may take long time for large datastores.",
+                type: bool,
+                optional: true,
+                default: false,
+            },
         },
     },
 )]
-- 
2.47.3

Re: [RFC PATCH proxmox-backup 0/4] fix #7903: rebuild local S3 chunk markers

[Shan Shaji]

Ahh, added the wrong bugzilla entry in the cover letter subject. It
should be #7908. Sorry for the noise.

- https://bugzilla.proxmox.com/show_bug.cgi?id=7908

On Tue Sep 15, 2026 at 5:59 PM CEST, Shan Shaji wrote:
> Sending this as an RFC to get an initial feedback. I'd really appreciate
> any suggestions or pointers to anything I may have missed.
>
> Reusing an S3-backed datastore currently restores the backup metadata to
> the local cache without creating local chunk markers. A garbage collection
> run is then needed to recreate markers for chunks referenced by the backup
> indexes.
>
> This series addresses #7908 by listing the chunk objects in S3 and creating
> zero-byte local markers when a datastore is recreated with
> --reuse-datastore. It also adds an optional rebuild-chunk-markers parameter
> to the s3-refresh API and exposes it through proxmox-backup-manager:
>
>     proxmox-backup-manager datastore s3-refresh <store> --rebuild-chunk-markers true
>
> The option is disabled by default for explicit refreshes. When enabled,
> it creates markers for all chunk objects with valid chunk keys, including
> chunks not referenced by the restored backup indexes. Existing cached
> chunk files are replaced with empty markers.
>
> Shan Shaji (4):
>   datastore: factor out s3 chunk objects pagination into a helper
>   fix #7908: datastore: create zero-byte markers when re-using datastore
>   datastore: api: add option to rebuild S3 chunk markers
>   datastore: backup-manager: expose option to rebuild s3 chunk markers
>
>  pbs-datastore/src/datastore.rs              | 120 +++++++++++++++-----
>  src/api2/admin/datastore.rs                 |  23 +++-
>  src/api2/config/datastore.rs                |   6 +-
>  src/bin/proxmox_backup_manager/datastore.rs |   7 ++
>  4 files changed, 121 insertions(+), 35 deletions(-)
>
> --
> 2.47.3

Re: [RFC PATCH proxmox-backup 1/4] datastore: factor out s3 chunk objects pagination into a helper

[Christian Ebner]

Code changes look good to me in general, just a few suggestions to make 
the dependency on the datastore's chunk store instance a bit more explicit.

On 9/15/26 6:00 PM, Shan Shaji wrote:
> This is in preparation to re-use the chunk iteration logic for creating
> empty marker files during s3 refresh.
> 
> Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
> ---
>   pbs-datastore/src/datastore.rs | 85 ++++++++++++++++++++++------------
>   1 file changed, 56 insertions(+), 29 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index c3db52269..f3eab8a28 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -2382,6 +2382,47 @@ impl DataStore {
>           }
>       }
>   
> +    /// Fetch chunk objects in batches of max 1000 per request.
> +    /// For each objects listed, the `callback` will be invoked with its
> +    /// S3 object key and size.
> +    fn paginate_s3_chunk_objects<F>(
> +        &self,

comment: As is, this does not need to be a method of DataStore, the 
reference to self is never used after all.

But I would keep it as such and make the dependency on the datastore and 
it's chunk store explicit.

To do this, I suggest to pass in the chunk store by reference to the 
callback and use the reference for all chunk store operations in the 
callback so the relation becomes stringent. So ...

> +        worker: &dyn WorkerTaskContext,
> +        s3_client: &S3Client,
> +        mut callback: F,
> +    ) -> Result<(), Error>
> +    where
> +        F: FnMut(S3ObjectKey, u64) -> Result<(), Error>,

comment: Instead of the generic, this could use an `impl Trait` 
parameter declaration, which makes the method signature cleaner, and 
with the suggested reference to the chunk store passed along would become:

fn paginate_s3_chunk_objects(
      &self,
      worker: &dyn WorkerTaskContext,
      s3_client: &S3Client,
      mut callback: impl FnMut(&ChunkStore, S3ObjectKey, u64) -> 
Result<(), Error>,
) -> Result<(), Error> {

> +    {
> +        let prefix = S3PathPrefix::Some(".chunks/".to_string());
> +        let mut list_bucket_result =
> +            proxmox_async::runtime::block_on(s3_client.list_objects_v2(&prefix, None))
> +                .context("failed to list chunk in s3 object store")?;
> +
> +        loop {
> +            for content in list_bucket_result.contents {
> +                worker.check_abort()?;
> +                worker.fail_on_shutdown()?;
> +

...  here we would now call:
```
callback(self.inner.chunk_store.as_ref(), content.key, content.size)?;
```

> +                callback(content.key, content.size)?;
> +            }
> +
> +            // Process next batch of chunks if there are more.
> +            if list_bucket_result.is_truncated {
> +                list_bucket_result = proxmox_async::runtime::block_on(s3_client.list_objects_v2(
> +                    &prefix,
> +                    list_bucket_result.next_continuation_token.as_deref(),
> +                ))?;
> +
> +                continue;
> +            }
> +
> +            break;
> +        }
> +
> +        Ok(())
> +    }
> +
>       fn garbage_collection_impl(
>           &self,
>           worker: &dyn WorkerTaskContext,
> @@ -2473,28 +2514,23 @@ impl DataStore {
>   
>           if let Some(ref s3_client) = s3_client {
>               let mut chunk_count = 0;
> -            let prefix = S3PathPrefix::Some(".chunks/".to_string());
> -            // Operates in batches of 1000 objects max per request
> -            let mut list_bucket_result =
> -                proxmox_async::runtime::block_on(s3_client.list_objects_v2(&prefix, None))
> -                    .context("failed to list chunk in s3 object store")?;
> -
>               let mut delete_list =
>                   S3DeleteList::with_thresholds(S3_DELETE_BATCH_LIMIT, S3_DELETE_DEFER_LIMIT_SECONDS);
> -            loop {
> -                for content in list_bucket_result.contents {
> -                    worker.check_abort()?;
> -                    worker.fail_on_shutdown()?;
> +
> +            self.paginate_s3_chunk_objects(
> +                worker,
> +                s3_client,
> +                |object_key, size| -> Result<(), Error> {

... this would then have the direct reference to the chunk store:

```
|chunk_store, object_key, size| -> Result<(), Error> {
```

>                       let (chunk_path, digest, bad) =
> -                        match self.chunk_path_from_object_key(&content.key) {
> +                        match self.chunk_path_from_object_key(&object_key) {
>                               Some(path) => path,
> -                            None => continue,
> +                            None => return Ok(()),
>                           };
>   
>                       let timeout = std::time::Duration::from_secs(0);
>                       let _chunk_guard = match self.inner.chunk_store.lock_chunk(&digest, timeout) {

... so operations on that would use that directly instead of binding to 
self, so here for example
```
let _chunk_guard = match chunk_store.lock_chunk(&digest, timeout) {
```

>                           Ok(guard) => guard,
> -                        Err(_) => continue,
> +                        Err(_) => return Ok(()),
>                       };
>                       let _guard = self.inner.chunk_store.mutex().lock().unwrap();
>   
> @@ -2526,7 +2562,7 @@ impl DataStore {
>                                   atime,
>                                   min_atime,
>                                   oldest_writer,
> -                                content.size,
> +                                size,
>                                   bad,
>                                   &mut gc_status,
>                                   || {
> @@ -2537,15 +2573,15 @@ impl DataStore {
>                                               std::fs::remove_file(chunk_path)?;
>                                           }
>                                       }
> -                                    delete_list.push(content.key, _chunk_guard);
> +                                    delete_list.push(object_key, _chunk_guard);
>                                       Ok(())
>                                   },
>                               )?;
>                           }
>                       } else {
>                           gc_status.removed_chunks += 1;
> -                        gc_status.removed_bytes += content.size;
> -                        delete_list.push(content.key, _chunk_guard);
> +                        gc_status.removed_bytes += size;
> +                        delete_list.push(object_key, _chunk_guard);
>                       }
>   
>                       chunk_count += 1;
> @@ -2555,19 +2591,10 @@ impl DataStore {
>   
>                       // limit pending deletes to avoid holding too many chunk flocks
>                       delete_list.conditional_delete_and_drop_locks(s3_client)?;
> -                }
> -                // Process next batch of chunks if there is more
> -                if list_bucket_result.is_truncated {
> -                    list_bucket_result =
> -                        proxmox_async::runtime::block_on(s3_client.list_objects_v2(
> -                            &prefix,
> -                            list_bucket_result.next_continuation_token.as_deref(),
> -                        ))?;
> -                    continue;
> -                }
>   
> -                break;
> -            }
> +                    Ok(())
> +                },
> +            )?;
>   
>               // delete the last batch of objects, if there are any remaining
>               delete_list.delete_and_drop_locks(s3_client)?;

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

[Christian Ebner]

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(())
>           },

Re: [RFC PATCH proxmox-backup 0/4] fix #7903: rebuild local S3 chunk markers

[Christian Ebner]

Thanks for the patches, left a few suggestions and major concerns on 
individual patches. It is especially crucial to take the possible 
runtime of this into account!

What is currently complete missing is any form of exposing this optional 
behavior to the user in the UI. It might make sense to have such a flag 
in the datastore create window, which is the main use-case for this. I 
see less use for normal s3 refresh operations.

On 9/15/26 6:00 PM, Shan Shaji wrote:
> Sending this as an RFC to get an initial feedback. I'd really appreciate
> any suggestions or pointers to anything I may have missed.
> 
> Reusing an S3-backed datastore currently restores the backup metadata to
> the local cache without creating local chunk markers. A garbage collection
> run is then needed to recreate markers for chunks referenced by the backup
> indexes.
> 
> This series addresses #7908 by listing the chunk objects in S3 and creating
> zero-byte local markers when a datastore is recreated with
> --reuse-datastore. It also adds an optional rebuild-chunk-markers parameter
> to the s3-refresh API and exposes it through proxmox-backup-manager:
> 
>      proxmox-backup-manager datastore s3-refresh <store> --rebuild-chunk-markers true
> 
> The option is disabled by default for explicit refreshes. When enabled,
> it creates markers for all chunk objects with valid chunk keys, including
> chunks not referenced by the restored backup indexes. Existing cached
> chunk files are replaced with empty markers.
> 
> Shan Shaji (4):
>    datastore: factor out s3 chunk objects pagination into a helper
>    fix #7908: datastore: create zero-byte markers when re-using datastore
>    datastore: api: add option to rebuild S3 chunk markers
>    datastore: backup-manager: expose option to rebuild s3 chunk markers
> 
>   pbs-datastore/src/datastore.rs              | 120 +++++++++++++++-----
>   src/api2/admin/datastore.rs                 |  23 +++-
>   src/api2/config/datastore.rs                |   6 +-
>   src/bin/proxmox_backup_manager/datastore.rs |   7 ++
>   4 files changed, 121 insertions(+), 35 deletions(-)
> 
> --
> 2.47.3
> 
> 
> 
>