Re: [pbs-devel] [PATCH proxmox-backup v5 3/4] partial fix #6049: datastore: use config fast-path in Drop

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

On November 28, 2025 10:03 am, Samuel Rufinatscha wrote:
> On 11/26/25 4:15 PM, Fabian Grünbichler wrote:
>> On November 24, 2025 6:04 pm, Samuel Rufinatscha wrote:
>>> @@ -307,12 +332,12 @@ impl DatastoreThreadSettings {
>>>   /// - If the cached generation matches the current generation, the
>>>   ///   cached config is returned.
>>>   /// - Otherwise the config is re-read from disk. If `update_cache` is
>>> -///   `true`, the new config and current generation are stored in the
>>> +///   `true`, the new config and bumped generation are stored in the
>>>   ///   cache. Callers that set `update_cache = true` must hold the
>>>   ///   datastore config lock to avoid racing with concurrent config
>>>   ///   changes.
>>>   /// - If `update_cache` is `false`, the freshly read config is returned
>>> -///   but the cache is left unchanged.
>>> +///   but the cache and generation are left unchanged.
>>>   ///
>>>   /// If `ConfigVersionCache` is not available, the config is always read
>>>   /// from disk and `None` is returned as the generation.
>>> @@ -333,14 +358,23 @@ fn datastore_section_config_cached(
>> 
>> does this part here make any sense in this patch?
>> 
>> we don't check the generation in the Drop handler anyway, so it will get
>> the latest cached version, no matter what?
>> 
> 
> we don't check the generation in the Drop handler, but the drop handler
> depends on this to potentially get a most fresh cached version?

datastore_section_config_cached will only reload the config if it was
changed over our API and the generation in the cached entry does no
longer match the current generation number. in that case there is no
need to bump the generation number, since that was already done by
whichever call saved the config and caused the generation number
mismatch in the first place - this already invalidated all previously
cached entries..

bumping the generation number only makes sense once we introduce the
force-reload mechanism in patch #4.

> 
>> we'd only end up in this part of the code via lookup_datastore, and only
>> if:
>> - the previous cached entry and the current one have a different
>>    generation -> no need to bump again, the cache is already invalidated
>> - there is no previous cached entry -> nothing to invalidate
>> 
>> I think this part should move to the next patch..
> 
> Shouldn't it be rather in PATCH 2 then, instead part of the TTL feature
> Also I would adjust the comment below then, so that it doesn't
> necessarily just benefit the drop handler that calls
> datastore_section_config_cached(false) but would in general future uses
> of datastore_section_config_cached(false)?

it has no benefit at this point in the series (or after/at patch #2),
see above. bumping only makes sense if we detect the generation number
is not valid, which we can only do via the digest check from patch#4.
and the digest check only makes sense with the TTL force-reload, because
else we can never end up in the code path where we read the config
without the cache already being invalid anyway.

> 
>> 
>>>           let (config_raw, _digest) = pbs_config::datastore::config()?;
>>>           let config = Arc::new(config_raw);
>>>   
>>> +        let mut effective_gen = current_gen;
>>>           if update_cache {
>>> +            // Bump the generation. 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 = version_cache.increase_datastore_generation();
>>> +            effective_gen = prev_gen + 1;
>>> +
>>> +            // Persist
>>>               *config_cache = Some(DatastoreConfigCache {
>>>                   config: config.clone(),
>>> -                last_generation: current_gen,
>>> +                last_generation: effective_gen,
>>>               });
>>>           }
>>>   
>>> -        Ok((config, Some(current_gen)))
>>> +        Ok((config, Some(effective_gen)))
>>>       } else {
>>>           // Fallback path, no config version cache: read datastore.cfg and return None as generation
>>>           *config_cache = None;
>>> -- 
>>> 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