[pbs-devel] [PATCH proxmox-backup 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>
---
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 f81603971..94ff53b86 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