Re: [pbs-devel] [PATCH proxmox-backup v3 5/6] partial fix #6049: datastore: add reload flag to config cache helper

[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:
>> Extend datastore_section_config_cached() with an `allow_reload` flag to
>> separate two use cases:
>>
>> 1) lookup_datastore() passes `true` and is allowed to reload
>>    datastore.cfg from disk when the cache is missing, the generation
>>    changed or the TTL expired. The helper may bump the datastore
>>    generation if the digest changed.
>>
>> 2) DataStore::drop() passes `false` and only consumes the most recent
>>    cached entry without touching the disk, TTL or generation. If the
>>    cache was never initialised, it returns an error.
>>
>> This avoids races between Drop and concurrent config changes.
>>
>> Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
>> ---
>>   pbs-datastore/src/datastore.rs | 36 ++++++++++++++++++++++++++++++----
>>   1 file changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>> index 1711c753..12076f31 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -226,7 +226,7 @@ impl Drop for DataStore {
>>                   return;
>>               }
>>   
>> -            let (section_config, _gen) = match datastore_section_config_cached() {
>> +            let (section_config, _gen) = match datastore_section_config_cached(false) {
>>                   Ok(v) => v,
>>                   Err(err) => {
>>                       log::error!(
>> @@ -299,14 +299,42 @@ impl DatastoreBackend {
>>       }
>>   }
>>   
>> -/// Return the cached datastore SectionConfig and its generation.
>> -fn datastore_section_config_cached() -> Result<(Arc<SectionConfigData>, Option<usize>), Error> {
>> +/// Returns the cached `datastore.cfg` and its generation.
>> +///
>> +/// When `allow_reload` is `true`, callers are expected to hold the datastore config. It may:
>> +///   - Reload `datastore.cfg` from disk if either
>> +///       - no cache exists yet, or cache is unavailable
>> +///       - the cached generation does not match the shared generation
>> +///       - the cache entry is older than `DATASTORE_CONFIG_CACHE_TTL_SECS`
>> +///   - Updates the cache with the new config, timestamp and digest.
>> +///   - Bumps the datastore generation in `ConfigVersionCache` only if
>> +///     there was a previous cached entry and the digest changed (manual edit or
>> +///     API write). If the digest is unchanged, the timestamp is refreshed but the
>> +///     generation is kept to avoid unnecessary invalidations.
>> +///
>> +/// When `allow_reload` is `false`:
>> +///   - Never touches the disk or the shared generation.
>> +///   - Ignores TTL and simply returns the most recent cached entry if available.
>> +///   - Returns an error if the cache has not been initialised yet.
>> +///
>> +/// Intended for use with `Datastore::drop` where no config lock is held
>> +/// and eventual stale data is acceptable.
>> +fn datastore_section_config_cached(
>> +    allow_reload: bool,
>> +) -> Result<(Arc<SectionConfigData>, Option<usize>), Error> {
>>       let now = epoch_i64();
>>       let version_cache = ConfigVersionCache::new().ok();
>>       let current_gen = version_cache.as_ref().map(|c| c.datastore_generation());
>>   
>>       let mut guard = DATASTORE_CONFIG_CACHE.lock().unwrap();
>>   
>> +    if !allow_reload {
>> +        if let Some(cache) = guard.as_ref() {
>> +            return Ok((cache.config.clone(), Some(cache.last_generation)));
>> +        }
>> +        bail!("datastore config cache not initialized");
>> +    }
> 
> this is not quite what I intended, we are actually allowed to reload,
> just not bump the generation number and store the result ;) the
> difference is basically whether we
> - hold the lock and can be sure that nothing modifies the
>    config/generation number while we do the lookup and bump
> - don't hold the lock and can just compare and reload, but not bump and
>    persist
> 
> if the code is restructured then this is should boil down to an if
> wrapping the generation bump and cache update, leaving the rest as it
> was..
> 

Makes sense, thanks Fabian! I will restructure it and fix the flag
check. The check should then wrap only bump and update as you
suggested. I think it could look like this:

fn datastore_section_config_cached(
     update_cache_and_generation: bool,
) -> Result<(Arc<SectionConfigData>, Option<usize>), Error> {
     let mut guard = DATASTORE_CONFIG_CACHE.lock().unwrap();

     if let Some(version_cache) = ConfigVersionCache::new().ok() {
         let now = epoch_i64();
         let current_gen = version_cache.datastore_generation();

         if let Some(cached) = guard.as_ref() {
             // Fast path: re-use cached datastore.cfg if cache is 
available, generation matches and TTL not expired
             if cached.last_generation == current_gen
                 && now - cached.last_update < 
DATASTORE_CONFIG_CACHE_TTL_SECS
             {
                 return Ok((cached.config.clone(), 
Some(cached.last_generation)));
             }
         }

         // Slow path: re-read datastore.cfg
         let (config_raw, digest) = pbs_config::datastore::config()?;
         let config = Arc::new(config_raw);

         let mut effective_gen = current_gen;
         if update_cache_and_generation {
             let (prev_gen, prev_digest) = guard
                 .as_ref()
                 .map(|c| (Some(c.last_generation), Some(c.digest)))
                 .unwrap_or((None, None));

             let manual_edit = match (prev_gen, prev_digest) {
                 (Some(prev_g), Some(prev_d)) => prev_g == current_gen 
&& prev_d != digest,
                 _ => false,
             };

             if manual_edit {
                 let old = version_cache.increase_datastore_generation();
                 effective_gen = old + 1;
             }

             // Update cache
             *guard = Some(DatastoreConfigCache {
                 config: config.clone(),
                 digest,
                 last_generation: effective_gen,
                 last_update: now,
             });
         }

         Ok((config, Some(effective_gen)))
     } else {
         // Fallback path, no config version cache: read datastore.cfg
         *guard = None;
         let (config_raw, _digest) = pbs_config::datastore::config()?;
         Ok((Arc::new(config_raw), None))
     }
}

>> +
>>       // Fast path: re-use cached datastore.cfg if cache is available, generation matches and TTL not expired
>>       if let (Some(current_gen), Some(config_cache)) = (current_gen, guard.as_ref()) {
>>           let gen_matches = config_cache.last_generation == current_gen;
>> @@ -423,7 +451,7 @@ impl DataStore {
>>           let _config_lock = pbs_config::datastore::lock_config()?;
>>   
>>           // Get the current datastore.cfg generation number and cached config
>> -        let (section_config, gen_num) = datastore_section_config_cached()?;
>> +        let (section_config, gen_num) = datastore_section_config_cached(true)?;
>>   
>>           let datastore_cfg: DataStoreConfig = section_config.lookup("datastore", name)?;
>>           let maintenance_mode = datastore_cfg.get_maintenance_mode();
>> -- 
>> 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