[pbs-devel] [PATCH proxmox-backup 1/3] GC: refactor chunk removal helper

[Christian Ebner]

one comment inline

On 10/15/25 10:38 AM, Fabian Grünbichler wrote:
> simplify the callback, and move the error handling to the helper..
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
>   pbs-datastore/src/chunk_store.rs | 27 ++++++++++++---------------
>   pbs-datastore/src/datastore.rs   |  2 +-
>   2 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index 6e50327cb..1c7df9074 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -415,19 +415,13 @@ impl ChunkStore {
>                       stat.st_size as u64,
>                       bad,
>                       status,
> -                    |status| {
> -                        if let Err(err) =
> -                            unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir)
> -                        {
> -                            if bad {
> -                                status.still_bad += 1;
> -                            }
> -                            bail!(
> +                    || {
> +                        unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir).map_err(|err| {
> +                            format_err!(
>                                   "unlinking chunk {filename:?} failed on store '{}' - {err}",
>                                   self.name,
> -                            );
> -                        }
> -                        Ok(())
> +                            )
> +                        })
>                       },
>                   )?;
>               }
> @@ -441,9 +435,7 @@ impl ChunkStore {
>       /// status accordingly.
>       ///
>       /// If the chunk should be removed, the [`remove_callback`] is executed.
> -    pub(super) fn check_atime_and_update_gc_status<
> -        T: FnOnce(&mut GarbageCollectionStatus) -> Result<(), Error>,
> -    >(
> +    pub(super) fn check_atime_and_update_gc_status<T: FnOnce() -> Result<(), Error>>(
>           atime: i64,
>           min_atime: i64,
>           oldest_writer: i64,
> @@ -453,7 +445,12 @@ impl ChunkStore {
>           remove_callback: T,
>       ) -> Result<(), Error> {
>           if atime < min_atime {
> -            remove_callback(gc_status)?;
> +            if let Err(err) = remove_callback() {
> +                if bad {
> +                    gc_status.still_bad += 1;
> +                    return Err(err);

Unless I'm overseeing something, this will now no longer propagate the 
error in case the removal of a non-bad chunk fails? Previously the error 
was returned independent from the `bad` state.

> +                }
> +            }
>               if bad {
>                   gc_status.removed_bad += 1;
>               } else {
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index a6b17e3c3..21998a157 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1671,7 +1671,7 @@ impl DataStore {
>                           content.size,
>                           bad,
>                           &mut gc_status,
> -                        |_status| {
> +                        || {
>                               if let Some(cache) = self.cache() {
>                                   // ignore errors, phase 3 will retry cleanup anyways
>                                   let _ = unsafe { cache.remove(&digest) };