[pbs-devel] [PATCH v4 proxmox-backup 6/8] datastore: conditionally use custom GC atime cutoff if set

[Fabian Grünbichler]

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 <c.ebner at proxmox.com>
> ---
> 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 / <human readable min_atime timestamp>
[Older backup writer started at <human readable timestamp> detected, extending cutoff to <timestamp>]

>              }
>  
> +            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 at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>