[pbs-devel] [PATCH proxmox-backup] GC: s3: fix local marker cleanup for unreferenced, s3 only chunks

[pbs-devel] [PATCH proxmox-backup] GC: s3: fix local marker cleanup for unreferenced, s3 only chunks

[Christian Ebner]

If a chunk object is located on the s3 object store only, not being
referenced by any index file and having no local marker file it is
marked for cleanup by pretending an atime equal to the unix epoch.

While this will mark the chunk for deletion from the backend and
include it in the delete list for the next s3 delete objects call, it
also will lead to the chunk marker and LRU cache entry being tried to
clean up locally, which however fails since there is no marker to be
cleaned up.

In order to treat this edge case with the same cleanup logic, simply
insert the marker file if not present, for it to get correctly
cleaned up as expected afterwards. This should not happen under
normal datastore operation anyways, most likely to appear after
re-creation of the datastore from existing bucket contents containing
such unreferenced chunks.

Fixes: https://forum.proxmox.com/threads/176567/
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 65299cca9..a24392d9f 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1711,11 +1711,12 @@ impl DataStore {
                     let atime = match std::fs::metadata(&chunk_path) {
                         Ok(stat) => stat.accessed()?,
                         Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
+                            unsafe {
+                                // chunk store lock held
+                                // insert marke unconditionally, cleaned up again below if required
+                                self.inner.chunk_store.replace_chunk_with_marker(&digest)?;
+                            }
                             if self.inner.chunk_store.clear_chunk_expected_mark(&digest)? {
-                                unsafe {
-                                    // chunk store lock held
-                                    self.inner.chunk_store.replace_chunk_with_marker(&digest)?;
-                                }
                                 SystemTime::now()
                             } else {
                                 // File not found, delete by setting atime to unix epoch
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup] GC: s3: fix local marker cleanup for unreferenced, s3 only chunks

[Thomas Lamprecht]

Am 22.11.25 um 11:41 schrieb Christian Ebner:
> If a chunk object is located on the s3 object store only, not being
> referenced by any index file and having no local marker file it is
> marked for cleanup by pretending an atime equal to the unix epoch.
> 
> While this will mark the chunk for deletion from the backend and
> include it in the delete list for the next s3 delete objects call, it
> also will lead to the chunk marker and LRU cache entry being tried to
> clean up locally, which however fails since there is no marker to be
> cleaned up.
> 
> In order to treat this edge case with the same cleanup logic, simply
> insert the marker file if not present, for it to get correctly
> cleaned up as expected afterwards. This should not happen under
> normal datastore operation anyways, most likely to appear after
> re-creation of the datastore from existing bucket contents containing
> such unreferenced chunks.
> 
> Fixes: https://forum.proxmox.com/threads/176567/
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 65299cca9..a24392d9f 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1711,11 +1711,12 @@ impl DataStore {
>                      let atime = match std::fs::metadata(&chunk_path) {
>                          Ok(stat) => stat.accessed()?,
>                          Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
> +                            unsafe {
> +                                // chunk store lock held
> +                                // insert marke unconditionally, cleaned up again below if required
> +                                self.inner.chunk_store.replace_chunk_with_marker(&digest)?;
> +                            }
>                              if self.inner.chunk_store.clear_chunk_expected_mark(&digest)? {
> -                                unsafe {
> -                                    // chunk store lock held
> -                                    self.inner.chunk_store.replace_chunk_with_marker(&digest)?;
> -                                }
>                                  SystemTime::now()

Why not drop that whole branch instead, it does not really makes sense IIUC.

And `replace_chunk_with_marker` replaces the chunk file directly (no extension) whereas
`clear_chunk_expected_mark` checks the chunk.using file, so does your reordering even
change anything, or is there a bug in `replace_chunk_with_marker`?

And independent of that, would it be better (more performant and less confusing) if
we ignore the "not present in LRU or no marker" in that edge case rather than creating
a file (doing more IO) just to delete that then again?

>                              } else {
>                                  // File not found, delete by setting atime to unix epoch



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel