Re: [pbs-devel] [PATCH proxmox-backup v5 4/4] partial fix #6049: datastore: add TTL fallback to catch manual config edits

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

nit for the subject: this doesn't fix the reported issue, it just
improves the fix further, so please drop that part and maybe instead add
"lookup" somewhere..

On November 24, 2025 6:04 pm, Samuel Rufinatscha wrote:
> The lookup fast path reacts to API-driven config changes because
> save_config() bumps the generation. Manual edits of datastore.cfg do
> not bump the counter. To keep the system robust against such edits
> without reintroducing config reading and hashing on the hot path, this
> patch adds a TTL to the cache entry.
> 
> If the cached config is older than
> DATASTORE_CONFIG_CACHE_TTL_SECS (set to 60s), the next lookup takes
> the slow path and refreshes the entry. As an optimization, a check to
> catch manual edits was added (if the digest changed but generation
> stayed the same), so that the generation is only bumped when needed.
> 
> Links
> 
> [1] cargo-flamegraph: https://github.com/flamegraph-rs/flamegraph
> 
> Fixes: #6049
> Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>

one style nit below, otherwise:

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

> ---
> Changes:
> 
> From v1 → v2
> - Store last_update timestamp in DatastoreConfigCache type.
> 
> From v2 → v3
> No changes
> 
> From v3 → v4
> - Fix digest generation bump logic in update_cache, thanks @Fabian.
> 
> From v4 → v5
> - Rebased only, no changes
> 
>  pbs-datastore/src/datastore.rs | 53 ++++++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 7638a899..0fc3fbf2 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -53,8 +53,12 @@ use crate::{DataBlob, LocalDatastoreLruCache};
>  struct DatastoreConfigCache {
>      // Parsed datastore.cfg file
>      config: Arc<SectionConfigData>,
> +    // Digest of the datastore.cfg file
> +    digest: [u8; 32],
>      // Generation number from ConfigVersionCache
>      last_generation: usize,
> +    // Last update time (epoch seconds)
> +    last_update: i64,
>  }
>  
>  static DATASTORE_CONFIG_CACHE: LazyLock<Mutex<Option<DatastoreConfigCache>>> =
> @@ -63,6 +67,8 @@ static DATASTORE_CONFIG_CACHE: LazyLock<Mutex<Option<DatastoreConfigCache>>> =
>  static DATASTORE_MAP: LazyLock<Mutex<HashMap<String, Arc<DataStoreImpl>>>> =
>      LazyLock::new(|| Mutex::new(HashMap::new()));
>  
> +/// Max age in seconds to reuse the cached datastore config.
> +const DATASTORE_CONFIG_CACHE_TTL_SECS: i64 = 60;
>  /// Filename to store backup group notes
>  pub const GROUP_NOTES_FILE_NAME: &str = "notes";
>  /// Filename to store backup group owner
> @@ -329,13 +335,14 @@ impl DatastoreThreadSettings {
>  /// generation.
>  ///
>  /// Uses `ConfigVersionCache` to detect stale entries:
> -/// - If the cached generation matches the current generation, the
> -///   cached config is returned.
> +/// - If the cached generation matches the current generation and TTL is
> +///   OK, the cached config is returned.
>  /// - Otherwise the config is re-read from disk. If `update_cache` is
> -///   `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.
> +///   `true` and a previous cached entry exists with the same generation
> +///   but a different digest, this indicates the config has changed
> +///   (e.g. manual edit) and the generation must be bumped. 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 and generation are left unchanged.
>  ///
> @@ -347,30 +354,46 @@ fn datastore_section_config_cached(
>      let mut config_cache = DATASTORE_CONFIG_CACHE.lock().unwrap();
>  
>      if let Ok(version_cache) = ConfigVersionCache::new() {
> +        let now = epoch_i64();
>          let current_gen = version_cache.datastore_generation();
>          if let Some(cached) = config_cache.as_ref() {
> -            // Fast path: re-use cached datastore.cfg
> -            if cached.last_generation == current_gen {
> +            // Fast path: re-use cached datastore.cfg if 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_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;
> +            // Bump the generation if the config has been changed manually.
> +            // 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, prev_digest) = config_cache
> +                .as_ref()
> +                .map(|c| (Some(c.last_generation), Some(c.digest)))
> +                .unwrap_or((None, None));

so here we map an option to a tuple of options and unwrap it

> +
> +            let manual_edit = match (prev_gen, prev_digest) {

only to then match and convert it to a boolean again here

> +                (Some(prev_g), Some(prev_d)) => prev_g == current_gen && prev_d != digest,
> +                _ => false,
> +            };
> +
> +            if manual_edit {
> +                let prev_gen = version_cache.increase_datastore_generation();
> +                effective_gen = prev_gen + 1;

to then do some code here, if the boolean is true ;)

this can all just be a single block of code instead:

            if let Some(cached) = config_cache.as_ref() {
                if cached.last_generation == current_gen && cached.digest != digest {
                    effective_gen = version_cache.increase_datastore_generation() + 1;
                }
            }

which also matches the first block higher up in the helper..

> +            }
>  
>              // Persist
>              *config_cache = Some(DatastoreConfigCache {
>                  config: config.clone(),
> +                digest,
>                  last_generation: effective_gen,
> +                last_update: now,
>              });
>          }
>  
> -- 
> 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