[pbs-devel] [PATCH proxmox-backup v2 3/3] GC: mark cond_sweep_chunk helper as unafe

[Fabian Grünbichler]

it's only allowed to be called in GC context with all the prerequisites like
locking, ensuring proper atime calculation, etc.

ideally, this should be internal to the chunk store, but with S3 the boundaries
between datastore, chunk cache and chunk store became rather blurry, and that
probably requires more refactoring of the interactions..

Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
Reviewed-by: Christian Ebner <c.ebner at proxmox.com>
---
best viewed with -w

 pbs-datastore/src/chunk_store.rs | 39 +++++++++++++++++++-------------
 pbs-datastore/src/datastore.rs   | 34 +++++++++++++++-------------
 2 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 065eb4a08..de34b3323 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -408,22 +408,26 @@ impl ChunkStore {
 
                 chunk_count += 1;
 
-                self.cond_sweep_chunk(
-                    stat.st_atime,
-                    min_atime,
-                    oldest_writer,
-                    stat.st_size as u64,
-                    bad,
-                    status,
-                    || {
-                        unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir).map_err(|err| {
-                            format_err!(
-                                "unlinking chunk {filename:?} failed on store '{}' - {err}",
-                                self.name,
+                unsafe {
+                    self.cond_sweep_chunk(
+                        stat.st_atime,
+                        min_atime,
+                        oldest_writer,
+                        stat.st_size as u64,
+                        bad,
+                        status,
+                        || {
+                            unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir).map_err(
+                                |err| {
+                                    format_err!(
+                                        "unlinking chunk {filename:?} failed on store '{}' - {err}",
+                                        self.name,
+                                    )
+                                },
                             )
-                        })
-                    },
-                )?;
+                        },
+                    )?;
+                }
             }
             drop(lock);
         }
@@ -435,7 +439,10 @@ impl ChunkStore {
     /// status accordingly.
     ///
     /// If the chunk should be removed, the [`remove_callback`] is executed.
-    pub(super) fn cond_sweep_chunk<T: FnOnce() -> Result<(), Error>>(
+    ///
+    /// Unsafe: requires locking and GC checks to be called
+    /// FIXME: make this internal with further refactoring
+    pub(super) unsafe fn cond_sweep_chunk<T: FnOnce() -> Result<(), Error>>(
         &self,
         atime: i64,
         min_atime: i64,
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index f1237af32..038306166 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1664,22 +1664,24 @@ impl DataStore {
                         .extension()
                         .is_some_and(|ext| ext == "bad");
 
-                    self.inner.chunk_store.cond_sweep_chunk(
-                        atime,
-                        min_atime,
-                        oldest_writer,
-                        content.size,
-                        bad,
-                        &mut gc_status,
-                        || {
-                            if let Some(cache) = self.cache() {
-                                // ignore errors, phase 3 will retry cleanup anyways
-                                let _ = unsafe { cache.remove(&digest) };
-                            }
-                            delete_list.push(content.key);
-                            Ok(())
-                        },
-                    )?;
+                    unsafe {
+                        self.inner.chunk_store.cond_sweep_chunk(
+                            atime,
+                            min_atime,
+                            oldest_writer,
+                            content.size,
+                            bad,
+                            &mut gc_status,
+                            || {
+                                if let Some(cache) = self.cache() {
+                                    // ignore errors, phase 3 will retry cleanup anyways
+                                    let _ = cache.remove(&digest);
+                                }
+                                delete_list.push(content.key);
+                                Ok(())
+                            },
+                        )?;
+                    }
 
                     chunk_count += 1;
                 }
-- 
2.47.3