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

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

On 11/28/25 11:46 AM, Fabian Grünbichler wrote:
> 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.
>

Makes sense, I see. Thanks for clarifying Fabian!
Will add it to patch 4.

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