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 584E01FF16B for ; Thu, 6 Mar 2025 12:00:38 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B8CAB322E; Thu, 6 Mar 2025 12:00:33 +0100 (CET) Date: Thu, 06 Mar 2025 12:00:24 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20250305151453.388817-1-c.ebner@proxmox.com> <20250305151453.388817-7-c.ebner@proxmox.com> In-Reply-To: <20250305151453.388817-7-c.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1741257534.lanqwml83c.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.043 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 Subject: Re: [pbs-devel] [PATCH v4 proxmox-backup 6/8] datastore: conditionally use custom GC atime cutoff if set 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 March 5, 2025 4:14 pm, Christian Ebner wrote: > Use the user configured atime cutoff over the default 24h 5m > margin if explicitly set, but only if the atime safety check is > enabled and succeeded. If the atime safety check is not enabled, > fallback to use the current default. > > Move the minimum atime calculation based on the atime cutoff to the > sweep_unused_chunks() callside and pass in the calculated values, as > to have the logic in the same place. > > Signed-off-by: Christian Ebner > --- > changes since version 3: > - move min_atime calculation to the gc atime cutoff logic, pass the > min_atime to sweep_unused_chunks() > > pbs-datastore/src/chunk_store.rs | 10 +--------- > pbs-datastore/src/datastore.rs | 30 ++++++++++++++++++++++++++++-- > 2 files changed, 29 insertions(+), 11 deletions(-) > > diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs > index e482330b3..0b211a44c 100644 > --- a/pbs-datastore/src/chunk_store.rs > +++ b/pbs-datastore/src/chunk_store.rs > @@ -364,7 +364,7 @@ impl ChunkStore { > pub fn sweep_unused_chunks( > &self, > oldest_writer: i64, > - phase1_start_time: i64, > + min_atime: i64, > status: &mut GarbageCollectionStatus, > worker: &dyn WorkerTaskContext, > ) -> Result<(), Error> { > @@ -374,14 +374,6 @@ impl ChunkStore { > use nix::sys::stat::fstatat; > use nix::unistd::{unlinkat, UnlinkatFlags}; > > - let mut min_atime = phase1_start_time - 3600 * 24; // at least 24h (see mount option relatime) > - > - if oldest_writer < min_atime { > - min_atime = oldest_writer; > - } > - > - min_atime -= 300; // add 5 mins gap for safety > - > let mut last_percentage = 0; > let mut chunk_count = 0; > > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs > index b62ddc172..16767832e 100644 > --- a/pbs-datastore/src/datastore.rs > +++ b/pbs-datastore/src/datastore.rs > @@ -1174,12 +1174,38 @@ impl DataStore { > DatastoreTuning::API_SCHEMA > .parse_property_string(gc_store_config.tuning.as_deref().unwrap_or(""))?, > )?; > - if tuning.gc_atime_safety_check.unwrap_or(true) { > + let gc_atime_safety_check = tuning.gc_atime_safety_check.unwrap_or(true); > + if gc_atime_safety_check { > self.inner.chunk_store.check_fs_atime_updates(true)?; > } else { > info!("Filesystem atime safety check disabled by datastore tuning options."); > + }; > + > + let mut min_atime = tuning > + .gc_atime_cutoff > + .and_then(|cutoff| { > + if gc_atime_safety_check { > + info!("Using GC atime cutoff {cutoff}m."); > + // Account for the 5 min default offset subtracted below > + Some(phase1_start_time + 300 - cutoff as i64 * 60) > + } else { > + warn!( > + "Ignoring GC atime cutoff of {cutoff}m since atime check is disabled." should these be tied together like this? the other way round I can see at some point in the future.. (lowering the implicit default if the check is enabled), but why should a non-default cutoff be ignored because the check is also explicitly disabled? > + ); > + None > + } > + }) > + .unwrap_or_else(|| { > + info!("Using default GC atime cutoff of 24h 5m"); > + phase1_start_time - 3600 * 24 > + }); the logging here is a bit confusing.. does it really matter whether the cutoff is default or explicit? > + > + if oldest_writer < min_atime { > + min_atime = oldest_writer; and if we log the above things, shouldn't we also log this? Maybe something like: GC atime cutoff XX minutes / [Older backup writer started at detected, extending cutoff to ] > } > > + min_atime -= 300; // add 5 mins gap for safety > + > info!("Start GC phase1 (mark used chunks)"); > > self.mark_used_chunks(&mut gc_status, worker)?; > @@ -1187,7 +1213,7 @@ impl DataStore { > info!("Start GC phase2 (sweep unused chunks)"); > self.inner.chunk_store.sweep_unused_chunks( > oldest_writer, > - phase1_start_time, > + min_atime, > &mut gc_status, > worker, > )?; > -- > 2.39.5 > > > > _______________________________________________ > 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