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

[Thomas Lamprecht]

Am 24.11.25 um 09:29 schrieb Fabian Grünbichler:
> On November 24, 2025 9:22 am, Christian Ebner wrote:
>> On 11/24/25 9:13 AM, Fabian Grünbichler wrote:
>>> we basically already have such a boolean marker - we set `atime` to 0 in
>>> this case (and only this case), and we could just ignore the removal
>>> errors then? possibly limited to just ignoring ENOTFOUND?
>>
>> That's a good idea! So I will perform the additional checks based on that.
> 
> alternatively, skipping cond_sweep_chunk entirely would also work (this
> is with `-w`):
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 65299cca9..b9debd2b1 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1709,21 +1709,22 @@ impl DataStore {
>                      // Check local markers (created or atime updated during phase1) and
>                      // keep or delete chunk based on that.
>                      let atime = match std::fs::metadata(&chunk_path) {
> -                        Ok(stat) => stat.accessed()?,
> +                        Ok(stat) => Some(stat.accessed()?),
>                          Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
>                              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()
> +                                Some(SystemTime::now())
>                              } else {
> -                                // File not found, delete by setting atime to unix epoch
> -                                SystemTime::UNIX_EPOCH
> +                                // File not found, only delete from S3
> +                                None
>                              }
>                          }
>                          Err(err) => return Err(err.into()),
>                      };
> +                    if let Some(atime) = atime {
>                          let atime = atime.duration_since(SystemTime::UNIX_EPOCH)?.as_secs() as i64;
>  
>                          unsafe {
> @@ -1752,6 +1753,13 @@ impl DataStore {
>                                  },
>                              )?;
>                          }
> +                    } else {
> +                        // set age based on first insertion
> +                        if delete_list.is_empty() {
> +                            delete_list_age = epoch_i64();
> +                        }
> +                        delete_list.push((content.key, _chunk_guard));
> +                    }


This is easier to understand for me change-wise and looks OK to me (no in-depth review
though).