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

[Christian Ebner]

On 10/15/25 10:38 AM, Fabian Grünbichler wrote:
> 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;
>                   }

LGTM!

Reviewed-by: Christian Ebner <c.ebner at proxmox.com>