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

[Christian Ebner <c.ebner@proxmox.com>]

On 1/13/26 11:24 AM, Fabian Grünbichler wrote:
> 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?

Acked, yes needs chunk store mutex lock for exclusive access, although 
there should be no concurrent operations which would overwrite this 
AFAIK? GC is already exclusive so removing based on atime will not be a 
problem and bad chunk renames check for filename, not atime.

> 
>> +        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 ;)).

Right, will do!

> 
>> -                        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
> 
> 



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