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 231FD1FF184 for ; Thu, 20 Nov 2025 15:50:23 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0F7CEB893; Thu, 20 Nov 2025 15:50:29 +0100 (CET) Date: Thu, 20 Nov 2025 15:50:22 +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-7-s.rufinatscha@proxmox.com> In-Reply-To: <20251120130342.248815-7-s.rufinatscha@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1763649810.vhbvbdgak6.astroid@yuna.none> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1763650194112 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.045 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [datastore.rs, proxmox.com] Subject: Re: [pbs-devel] [PATCH proxmox-backup v3 6/6] datastore: only bump generation when config digest changes 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: > When reloading datastore.cfg in datastore_section_config_cached(), > we currently bump the datastore generation unconditionally. This is > only necessary when the on disk content actually changed and when > we already had a previous cached entry. > > This patch extends the DatastoreConfigCache to store the last digest of > datastore.cfg and track the previously cached generation and digest. > Only when the digest differs from the cached one. On first load, it > reuses the existing datastore_generation without bumping. > > This avoids unnecessary cache invalidations if the config did not > change. > > Signed-off-by: Samuel Rufinatscha > --- > pbs-datastore/src/datastore.rs | 43 ++++++++++++++++++++++++---------- > 1 file changed, 30 insertions(+), 13 deletions(-) > > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs > index 12076f31..bf04332e 100644 > --- a/pbs-datastore/src/datastore.rs > +++ b/pbs-datastore/src/datastore.rs > @@ -51,6 +51,8 @@ use crate::{DataBlob, LocalDatastoreLruCache}; > struct DatastoreConfigCache { > // Parsed datastore.cfg file > config: Arc, > + // Digest of the datastore.cfg file > + last_digest: [u8; 32], > // Generation number from ConfigVersionCache > last_generation: usize, > // Last update time (epoch seconds) > @@ -349,29 +351,44 @@ fn datastore_section_config_cached( > } > > // 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); > > - // Update cache > + // Decide whether to bump the shared generation. > + // Only bump if we already had a cached generation and the digest changed (manual edit or API write) > + let (prev_gen, prev_digest) = guard > + .as_ref() > + .map(|c| (Some(c.last_generation), Some(c.last_digest))) > + .unwrap_or((None, None)); > + > let new_gen = if let Some(handle) = version_cache { > - // Bump datastore generation whenever we reload the config. > - // 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 = handle.increase_datastore_generation(); > - let new_gen = prev_gen + 1; > + match (prev_gen, prev_digest) { > + // We had a previous generation and the digest changed => bump generation. > + (Some(_prev_gen), Some(prev_digest)) if prev_digest != digest => { this is not quite the correct logic - I think. we only need to bump *iff* the digest doesn't match, but the generation does - that implies somebody changed the config behind our back. if the generation is different, we should *expect* the digest to also not be identical, but we don't have to care in that case, since the generation was already bumped (compared to the last cached state with the different digest), and that invalidates all the old cache references anyway.. again, I think this would be easier to follow along if the structure of the ifs is changed ;) > + let old = handle.increase_datastore_generation(); > + Some(old + 1) > + } > + // We had a previous generation but the digest stayed the same: > + // keep the existing generation, just refresh the timestamp. > + (Some(prev_gen), _) => Some(prev_gen), > + // We didn't have a previous generation, just use the current one. > + (None, _) => Some(handle.datastore_generation()), > + } > + } else { > + None > + }; > > + if let Some(gen_val) = new_gen { > *guard = Some(DatastoreConfigCache { > config: config.clone(), > - last_generation: new_gen, > + last_digest: digest, > + last_generation: gen_val, > last_update: now, > }); > - > - Some(new_gen) > } else { > - // if the cache was not available, use again the slow path next time > + // If the shared version cache is not available, don't cache. > *guard = None; > - None > - }; > + } > > Ok((config, new_gen)) > } > -- > 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