Re: [pbs-devel] [PATCH proxmox-backup v2 6/8] datastore: move bad chunk touching logic to chunk store

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On December 11, 2025 4:38 pm, Christian Ebner wrote:
> Provide an dedicated method and encapsulate the bad chunk filename
> generation and access time updates. This is an implementation detail
> of the chunks store, the garbage collection should not be concerned
> about this.
> 
> In preparation for making the bad filename generation a dedicated
> helper as well, all contained within the chunk store module.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-datastore/src/chunk_store.rs | 15 +++++++++++++++
>  pbs-datastore/src/datastore.rs   | 11 +----------
>  2 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index e05e67dfe..2ad90757a 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -276,6 +276,21 @@ impl ChunkStore {
>          Ok(true)
>      }
>  
> +    /// Update access timestamp on all bad chunks for given digest
> +    pub(super) fn cond_touch_bad_chunks(&self, digest: &[u8; 32]) -> Result<bool, Error> {

please document the locking semantics for this new interface!

AFAICT, this is only called once next to calls to cond_touch_chunk which
obtain the chunk store Mutex, so should this also do so?

> +        let (chunk_path, _digest_str) = self.chunk_path(digest);
> +        let mut is_bad = false;
> +        for i in 0..=9 {
> +            let bad_ext = format!("{i}.bad");
> +            let mut bad_path = chunk_path.clone();
> +            bad_path.set_extension(bad_ext);
> +            if self.cond_touch_path(&bad_path, false)? {
> +                is_bad = true;
> +            }
> +        }
> +        Ok(is_bad)
> +    }
> +
>      fn get_chunk_store_iterator(
>          &self,
>      ) -> Result<
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 58fd034fc..2bab20294 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1355,19 +1355,10 @@ impl DataStore {
>              }
>  
>              if !self.inner.chunk_store.cond_touch_chunk(digest, false)? {
> -                let (chunk_path, _digest_str) = self.chunk_path(digest);
>                  // touch any corresponding .bad files to keep them around, meaning if a chunk is
>                  // rewritten correctly they will be removed automatically, as well as if no index
>                  // file requires the chunk anymore (won't get to this loop then)
> -                let mut is_bad = false;
> -                for i in 0..=9 {
> -                    let bad_ext = format!("{i}.bad");
> -                    let mut bad_path = chunk_path.clone();
> -                    bad_path.set_extension(bad_ext);
> -                    if self.inner.chunk_store.cond_touch_path(&bad_path, false)? {

this was the last call to cond_touch_path outside of chunk_store.rs ,
which means we can make it private now (which was part of the point of this series ;)).

> -                        is_bad = true;
> -                    }
> -                }
> +                let is_bad = self.inner.chunk_store.cond_touch_bad_chunks(digest)?;
>  
>                  if let Some(ref _s3_client) = s3_client {
>                      // Do not retry here, this is very unlikely to happen as chunk markers will
> -- 
> 2.47.3
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel