Re: [pbs-devel] [PATCH v3 proxmox-backup 7/9] datastore: conditionally use custom GC atime cutoff if set

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On March 4, 2025 7:35 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.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 2:
> - Adapt datastore tuning option names
> - Fallback to default on relatime behaviour
> 
>  pbs-datastore/src/chunk_store.rs |  9 ++++++++-
>  pbs-datastore/src/datastore.rs   | 28 +++++++++++++++++++++++++---
>  2 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index e529dcc9c..f591412a2 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -371,6 +371,7 @@ impl ChunkStore {
>          &self,
>          oldest_writer: i64,
>          phase1_start_time: i64,
> +        atime_cutoff: Option<usize>,

we could just pass in min_atime instead of phase1_start_time?

>          status: &mut GarbageCollectionStatus,
>          worker: &dyn WorkerTaskContext,
>      ) -> Result<(), Error> {
> @@ -380,7 +381,13 @@ 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)
> +        let mut min_atime = if let Some(atime_cutoff) = atime_cutoff {
> +            // account for the default 5 min safety gap subtracted below
> +            phase1_start_time + 300 - atime_cutoff as i64 * 60
> +        } else {
> +            // at least 24h (see mount option relatime)
> +            phase1_start_time - 3600 * 24
> +        };

since this should move below

>  
>          if oldest_writer < min_atime {
>              min_atime = oldest_writer;
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index ef932b47b..4f07cfc60 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1174,11 +1174,32 @@ 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) {
> -                self.inner.chunk_store.atime_safety_check()?;
> -                info!("Filesystem atime safety check successful.");
> +            let gc_atime_safety_check = tuning.gc_atime_safety_check.unwrap_or(true);
> +            let atime_updated = if gc_atime_safety_check {
> +                if self.inner.chunk_store.atime_safety_check()? {
> +                    info!("Filesystem atime safety check successful.");
> +                    true
> +                } else {
> +                    info!("Filesystem atime safety check successful with relatime behaviour.");
> +                    false
> +                }
>              } else {
>                  info!("Filesystem atime safety check disabled by datastore tuning options.");
> +                false
> +            };
> +            let mut atime_cutoff = None;
> +            if let Some(cutoff) = tuning.gc_atime_cutoff {
> +                if atime_updated {
> +                    info!("Using GC atime cutoff {cutoff}m.");
> +                    atime_cutoff = Some(cutoff);
> +                } else {
> +                    warn!(
> +                        "Ignoring GC atime cutoff of {cutoff}m, because atime check is \
> +                        disabled or relatime behaviour detected."
> +                    );
> +                }
> +            } else {
> +                info!("Using default GC atime cutoff of 24h 5m");
>              }

since both the check and the decision is made here, and there is no
reason to split it between the two modules..

>  
>              info!("Start GC phase1 (mark used chunks)");
> @@ -1189,6 +1210,7 @@ impl DataStore {
>              self.inner.chunk_store.sweep_unused_chunks(
>                  oldest_writer,
>                  phase1_start_time,
> +                atime_cutoff,
>                  &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