From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 68AAD1FF184 for ; Thu, 20 Nov 2025 15:50:54 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F3F81B910; Thu, 20 Nov 2025 15:51:00 +0100 (CET) Date: Thu, 20 Nov 2025 15:50:24 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20251120130342.248815-1-s.rufinatscha@proxmox.com> <20251120130342.248815-6-s.rufinatscha@proxmox.com> In-Reply-To: <20251120130342.248815-6-s.rufinatscha@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1763648760.ojcxikon5f.astroid@yuna.none> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1763650195532 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.047 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup v3 5/6] partial fix #6049: datastore: add reload flag to config cache helper X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 > --- > 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, Option), 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, Option), 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.. > + > // 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