Re: [pbs-devel] [PATCH proxmox-backup v3 6/6] datastore: only bump generation when config digest changes

[Samuel Rufinatscha <s.rufinatscha@proxmox.com>]

On 11/20/25 3:50 PM, Fabian Grünbichler wrote:
> On November 20, 2025 2:03 pm, Samuel Rufinatscha wrote:
>> When reloading datastore.cfg in datastore_section_config_cached(),
>> we currently bump the datastore generation unconditionally. This is
>> only necessary when the on disk content actually changed and when
>> we already had a previous cached entry.
>>
>> This patch extends the DatastoreConfigCache to store the last digest of
>> datastore.cfg and track the previously cached generation and digest.
>> Only when the digest differs from the cached one. On first load, it
>> reuses the existing datastore_generation without bumping.
>>
>> This avoids unnecessary cache invalidations if the config did not
>> change.
>>
>> Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
>> ---
>>   pbs-datastore/src/datastore.rs | 43 ++++++++++++++++++++++++----------
>>   1 file changed, 30 insertions(+), 13 deletions(-)
>>
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>> index 12076f31..bf04332e 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -51,6 +51,8 @@ use crate::{DataBlob, LocalDatastoreLruCache};
>>   struct DatastoreConfigCache {
>>       // Parsed datastore.cfg file
>>       config: Arc<SectionConfigData>,
>> +    // Digest of the datastore.cfg file
>> +    last_digest: [u8; 32],
>>       // Generation number from ConfigVersionCache
>>       last_generation: usize,
>>       // Last update time (epoch seconds)
>> @@ -349,29 +351,44 @@ fn datastore_section_config_cached(
>>       }
>>   
>>       // Slow path: re-read datastore.cfg
>> -    let (config_raw, _digest) = pbs_config::datastore::config()?;
>> +    let (config_raw, digest) = pbs_config::datastore::config()?;
>>       let config = Arc::new(config_raw);
>>   
>> -    // Update cache
>> +    // Decide whether to bump the shared generation.
>> +    // Only bump if we already had a cached generation and the digest changed (manual edit or API write)
>> +    let (prev_gen, prev_digest) = guard
>> +        .as_ref()
>> +        .map(|c| (Some(c.last_generation), Some(c.last_digest)))
>> +        .unwrap_or((None, None));
>> +
>>       let new_gen = if let Some(handle) = version_cache {
>> -        // Bump datastore generation whenever we reload the config.
>> -        // This ensures that Drop handlers will detect that a newer config exists
>> -        // and will not rely on a stale cached entry for maintenance mandate.
>> -        let prev_gen = handle.increase_datastore_generation();
>> -        let new_gen = prev_gen + 1;
>> +        match (prev_gen, prev_digest) {
>> +            // We had a previous generation and the digest changed => bump generation.
>> +            (Some(_prev_gen), Some(prev_digest)) if prev_digest != digest => {
> 
> this is not quite the correct logic - I think.
> 
> we only need to bump *iff* the digest doesn't match, but the generation
> does - that implies somebody changed the config behind our back.
> 
> if the generation is different, we should *expect* the digest to also
> not be identical, but we don't have to care in that case, since the
> generation was already bumped (compared to the last cached state with
> the different digest), and that invalidates all the old cache references
> anyway..
> 

Makes sense and good point! I will restrict bumping here for the case 
you mentioned (*iff* the digest doesn't match, but the generation does).
So in the case the generation is different, we can rely on the current gen.

> again, I think this would be easier to follow along if the structure of
> the ifs is changed ;)
> 

I agree, changing :-)

>> +                let old = handle.increase_datastore_generation();
>> +                Some(old + 1)
>> +            }
>> +            // We had a previous generation but the digest stayed the same:
>> +            // keep the existing generation, just refresh the timestamp.
>> +            (Some(prev_gen), _) => Some(prev_gen),
>> +            // We didn't have a previous generation, just use the current one.
>> +            (None, _) => Some(handle.datastore_generation()),
>> +        }
>> +    } else {
>> +        None
>> +    };
>>   
>> +    if let Some(gen_val) = new_gen {
>>           *guard = Some(DatastoreConfigCache {
>>               config: config.clone(),
>> -            last_generation: new_gen,
>> +            last_digest: digest,
>> +            last_generation: gen_val,
>>               last_update: now,
>>           });
>> -
>> -        Some(new_gen)
>>       } else {
>> -        // if the cache was not available, use again the slow path next time
>> +        // If the shared version cache is not available, don't cache.
>>           *guard = None;
>> -        None
>> -    };
>> +    }
>>   
>>       Ok((config, new_gen))
>>   }
>> -- 
>> 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