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

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

The Drop impl of DataStore re-read datastore.cfg to decide whether
the entry should be evicted from the in-process cache (based on
maintenance mode’s clear_from_cache). During the investigation of
issue #6049 [1], a flamegraph [2] showed that the config reload in Drop
accounted for a measurable share of CPU time under load.

This patch wires the datastore config fast path to the Drop
impl to eventually avoid an expensive config reload from disk to capture
the maintenance mandate.

Behavioral notes

- Drop no longer silently ignores config/lookup failures: failures to
load/parse datastore.cfg are logged at WARN level
- If the datastore no longer exists in datastore.cfg when the last
handle is dropped, the cached instance is evicted from DATASTORE_MAP
if available (without checking maintenance mode).

Links

[1] Bugzilla: https://bugzilla.proxmox.com/show_bug.cgi?id=6049
[2] cargo-flamegraph: https://github.com/flamegraph-rs/flamegraph

Fixes: #6049
Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
---
Changes:

From v1 → v2
- Replace caching logic with the datastore_section_config_cached()
  helper.

From v2 → v3
No changes

From v3 → v4, thanks @Fabian
- Pass datastore_section_config_cached(false) in Drop to avoid
  concurrent cache updates.

From v4 → v5
- Rebased only, no changes

From v5 → v6
- Rebased
- Styling: restructured cache eviction condition
- Drop impl: log cache-related failures to load/parse datastore.cfg at
  WARN level instead of ERROR
- Note logging change in the patch message, thanks @Fabian
- Remove cached entry from DATASTORE_MAP (if available) if datastore no
  longer exists in datastore.cfg when the last handle is dropped,
  thanks @Fabian
- Removed slow-path generation bumping in
  datastore_section_config_cached, since API changes already
  bump the generation on config save. Moved to subsequent patch,
  relevant for TTL-based mechanism to bump on non-API edits, thanks @Fabian

 pbs-datastore/src/datastore.rs | 35 ++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index aa366826..8adb0e3b 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -224,14 +224,33 @@ impl Drop for DataStore {
 
             // remove datastore from cache iff
             //  - last task finished, and
-            //  - datastore is in a maintenance mode that mandates it
-            let remove_from_cache = last_task
-                && pbs_config::datastore::config()
-                    .and_then(|(s, _)| s.lookup::<DataStoreConfig>("datastore", self.name()))
-                    .is_ok_and(|c| {
-                        c.get_maintenance_mode()
-                            .is_some_and(|m| m.clear_from_cache())
-                    });
+            //  - datastore is in a maintenance mode that mandates it, or the datastore was removed from datastore.cfg
+
+            // first check: check if last task finished
+            if !last_task {
+                return;
+            }
+
+            // determine whether we should evict from DATASTORE_MAP.
+            let remove_from_cache = match datastore_section_config_cached(false) {
+                Ok((section_config, _gen)) => {
+                    match section_config.lookup::<DataStoreConfig>("datastore", self.name()) {
+                        // second check: check if maintenance mode requires closing FDs
+                        Ok(config) => config
+                            .get_maintenance_mode()
+                            .is_some_and(|m| m.clear_from_cache()),
+                        Err(err) => {
+                            // datastore removed from config; evict cached entry if available (without checking maintenance mode)
+                            log::warn!("DataStore::drop: datastore '{}' missing from datastore.cfg; evicting cached instance: {err}", self.name());
+                            true
+                        }
+                    }
+                }
+                Err(err) => {
+                    log::warn!("DataStore::drop: failed to load datastore.cfg for '{}'; skipping cache-eviction: {err}", self.name());
+                    false
+                }
+            };
 
             if remove_from_cache {
                 DATASTORE_MAP.lock().unwrap().remove(self.name());
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel