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

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

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

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.

 pbs-datastore/src/datastore.rs | 55 ++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 16 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 942656e6..a5c450d0 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -22,7 +22,7 @@ use proxmox_sys::error::SysError;
 use proxmox_sys::fs::{file_read_optional_string, replace_file, CreateOptions};
 use proxmox_sys::linux::procfs::MountInfo;
 use proxmox_sys::process_locker::{ProcessLockExclusiveGuard, ProcessLockSharedGuard};
-use proxmox_time::TimeSpan;
+use proxmox_time::{epoch_i64, TimeSpan};
 use proxmox_worker_task::WorkerTaskContext;
 
 use pbs_api_types::{
@@ -51,8 +51,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>>> =
@@ -61,6 +65,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
@@ -299,13 +305,14 @@ impl DatastoreBackend {
 /// 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.
 ///
@@ -317,30 +324,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));
+
+            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 prev_gen = version_cache.increase_datastore_generation();
+                effective_gen = prev_gen + 1;
+            }
 
             // 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