[pbs-devel] [PATCH v3 proxmox-backup 0/9] fix #5982: check atime update is honored

[pbs-devel] [PATCH v3 proxmox-backup 0/9] fix #5982: check atime update is honored

[Christian Ebner]

These patches add a check to phase 1 of garbage collection in order
to detect when the filesystem backing the chunk store does not honor
atime updates. This avoids possible data loss for situations where
garbage collection could otherwise delete chunks still referenced by
a backup snaphost's index file.

The check is performed by inserting a fixed size 4 MiB unencrypted and
compressed chunk of all-zeros. The check is performed by setting the
atime to 1 second in the past followed by setting it to now in order to
test if atime updates are honored immediately. The test is performed
on datastore creation for early detection and before garbage collection
phase 1. The test is enabled by default, but an opt-out option can be
set via the datastore tuning parameters for backwards compatibility.

Further, add a datastore tuning parameter to reduce the wait period for
chunk removal in phase 2 of garbage collection. Make this conditional on
the atime update check being enabled and successful, to avoid possible
data loss.

Most notable changes sice version 1 (thanks Fabian and Thomas for
comments and suggestions):
- Take into account Linux timestamp granularity, do not set timestamp
  to the past, as that introduces other error paths such as lack of
  permissions or fs limitations.
- Check relatime behavior, if atime behaviour is not honored. Fallback
  to original cutoff in that case.
- Adapt tuning parameter names.

Most notable changes sice version 1 (thanks Fabian and Thomas for
comments and suggestions):
- Optimize check by using the all zero chunk
- Enable the check by default and fail GC job if not honored, but allow
  to opt-out
- Add GC wait period tuning option

Link to the issue in the bugtracker:
https://bugzilla.proxmox.com/show_bug.cgi?id=5982

proxmox:

Christian Ebner (2):
  pbs api types: add garbage collection atime safety check flag
  pbs api types: add option to set GC chunk cleanup atime cutoff

 pbs-api-types/src/datastore.rs | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

proxmox-backup:

Christian Ebner (7):
  datastore: use libc's timespec constants instead of redefinition
  fix #5982: garbage collection: check atime updates are honored
  ui: expose GC atime safety check flag in datastore tuning options
  docs: mention GC atime update check for tuning options
  datastore: conditionally use custom GC atime cutoff if set
  ui: expose GC atime cutoff in datastore tuning option
  docs: mention gc-atime-cutoff as datastore tuning option

 docs/storage.rst                 |  19 +++-
 pbs-datastore/src/chunk_store.rs | 144 ++++++++++++++++++++++++++++---
 pbs-datastore/src/datastore.rs   |  32 +++++++
 src/api2/config/datastore.rs     |   1 +
 www/Utils.js                     |   9 ++
 www/datastore/OptionView.js      |  17 ++++
 6 files changed, 208 insertions(+), 14 deletions(-)

-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v3 proxmox 1/2] pbs api types: add garbage collection atime safety check flag

[Christian Ebner]

Add the `gc-atime-safety-check` flag to the datastore tuning
parameters. This flag allows to enable/disable a check to detect if
the atime update is honored by the filesystem backing the chunk store
during phase 1 of garbage collection and during datastore creation.

The default is to perform the check.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 2:
- rename datastore tuning parameter to be more specific

 pbs-api-types/src/datastore.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index ddd8d3c6..1a4d9e2d 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -229,6 +229,14 @@ pub enum DatastoreFSyncLevel {
             type: ChunkOrder,
             optional: true,
         },
+        "gc-atime-safety-check": {
+            description:
+                "Check filesystem atime updates are honored during store creation and garbage \
+                collection",
+            optional: true,
+            default: true,
+            type: bool,
+        },
     },
 )]
 #[derive(Serialize, Deserialize, Default)]
@@ -240,6 +248,8 @@ pub struct DatastoreTuning {
     pub chunk_order: Option<ChunkOrder>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub sync_level: Option<DatastoreFSyncLevel>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub gc_atime_safety_check: Option<bool>,
 }
 
 pub const DATASTORE_TUNING_STRING_SCHEMA: Schema = StringSchema::new("Datastore tuning options")
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v3 proxmox 2/2] pbs api types: add option to set GC chunk cleanup atime cutoff

[Christian Ebner]

Add the `gc-atime-cutoff` option to the datastore tuning parameters.
This allows to specify the time after which the chunks are not
considered in use anymore if their atime has not been updated since
then. This option is only considered, if the `gc-atime-safety-check`
is enabled, to avoid potential data loss.

The default is to keep chunks within the 24h 5m timespan (given no
active writers).

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 2:
- rename datastore tuning parameter to be more specific
- adapt allowed values range to cover from 1 - 2880 minutes

 pbs-api-types/src/datastore.rs | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 1a4d9e2d..467417e8 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -223,6 +223,15 @@ pub enum DatastoreFSyncLevel {
     Filesystem,
 }
 
+pub const GC_ATIME_CUTOFF_SCHEMA: Schema = IntegerSchema::new(
+    "Cutoff (in minutes) for chunk cleanup atime check in garbage collection phase 2 \
+        (default 24h 5m)",
+)
+.minimum(1) // safety margin for kernel timestamp granularity, but stay within minute range
+.maximum(2880) // 2 days
+.default(1445)
+.schema();
+
 #[api(
     properties: {
         "chunk-order": {
@@ -237,6 +246,10 @@ pub enum DatastoreFSyncLevel {
             default: true,
             type: bool,
         },
+        "gc-atime-cutoff": {
+            schema: GC_ATIME_CUTOFF_SCHEMA,
+            optional: true,
+        },
     },
 )]
 #[derive(Serialize, Deserialize, Default)]
@@ -250,6 +263,8 @@ pub struct DatastoreTuning {
     pub sync_level: Option<DatastoreFSyncLevel>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub gc_atime_safety_check: Option<bool>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub gc_atime_cutoff: Option<usize>,
 }
 
 pub const DATASTORE_TUNING_STRING_SCHEMA: Schema = StringSchema::new("Datastore tuning options")
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v3 proxmox-backup 3/9] datastore: use libc's timespec constants instead of redefinition

[Christian Ebner]

Use the UTIME_NOW and UTIME_OMIT constants defined in libc crate
instead of redefining them. This improves consistency, as utimesat
and its timespec parameter are also defined via the libc crate.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 2:
- use libc's constants instead of re-defining them

 pbs-datastore/src/chunk_store.rs | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 29d5874a1..5e02909a1 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -220,19 +220,16 @@ impl ChunkStore {
         // unwrap: only `None` in unit tests
         assert!(self.locker.is_some());
 
-        const UTIME_NOW: i64 = (1 << 30) - 1;
-        const UTIME_OMIT: i64 = (1 << 30) - 2;
-
         let times: [libc::timespec; 2] = [
             // access time -> update to now
             libc::timespec {
                 tv_sec: 0,
-                tv_nsec: UTIME_NOW,
+                tv_nsec: libc::UTIME_NOW,
             },
             // modification time -> keep as is
             libc::timespec {
                 tv_sec: 0,
-                tv_nsec: UTIME_OMIT,
+                tv_nsec: libc::UTIME_OMIT,
             },
         ];
 
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] applied: [PATCH v3 proxmox-backup 3/9] datastore: use libc's timespec constants instead of redefinition

[Fabian Grünbichler]

applied this one already :)

On March 4, 2025 7:35 pm, Christian Ebner wrote:
> Use the UTIME_NOW and UTIME_OMIT constants defined in libc crate
> instead of redefining them. This improves consistency, as utimesat
> and its timespec parameter are also defined via the libc crate.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 2:
> - use libc's constants instead of re-defining them
> 
>  pbs-datastore/src/chunk_store.rs | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index 29d5874a1..5e02909a1 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -220,19 +220,16 @@ impl ChunkStore {
>          // unwrap: only `None` in unit tests
>          assert!(self.locker.is_some());
>  
> -        const UTIME_NOW: i64 = (1 << 30) - 1;
> -        const UTIME_OMIT: i64 = (1 << 30) - 2;
> -
>          let times: [libc::timespec; 2] = [
>              // access time -> update to now
>              libc::timespec {
>                  tv_sec: 0,
> -                tv_nsec: UTIME_NOW,
> +                tv_nsec: libc::UTIME_NOW,
>              },
>              // modification time -> keep as is
>              libc::timespec {
>                  tv_sec: 0,
> -                tv_nsec: UTIME_OMIT,
> +                tv_nsec: libc::UTIME_OMIT,
>              },
>          ];
>  
> -- 
> 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

[pbs-devel] [PATCH v3 proxmox-backup 4/9] fix #5982: garbage collection: check atime updates are honored

[Christian Ebner]

Check if the filesystem backing the chunk store actually updates the
atime to avoid potential data loss in phase 2 of garbage collection,
in case the atime update is not honored.

Perform the check before phase 1 of garbage collection, as well as
on datastore creation. The latter to early detect and disallow
datastore creation on filesystem configurations which otherwise most
likely would lead to data losses.

Enable the atime update check by default, but allow to opt-out by
setting a datastore tuning parameter flag for backwards compatibility.
This is honored by both, garbage collection and datastore creation.

The check uses a 4 MiB fixed sized, unencypted and compressed chunk
as test marker, inserted if not present. This all zero-chunk is very
likely anyways for unencrypted backup contents with large all-zero
regions using fixed size chunking (e.g. VMs).

To avoid cases were the timestamp will not be updated because of the
Linux kernels timestamp granularity, sleep in-between stating and
utimensat for 1 second.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5982
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 2:
- Take Linux timestamp granularity into account by sleeping 1 second
  in-between operations instead of setting timestamp to the past.
- Check relatime behavior if atime behaviour not honored.
- adapt datastore tuning variables to new names


 pbs-datastore/src/chunk_store.rs | 128 +++++++++++++++++++++++++++++--
 pbs-datastore/src/datastore.rs   |  10 +++
 src/api2/config/datastore.rs     |   1 +
 3 files changed, 134 insertions(+), 5 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 5e02909a1..e529dcc9c 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -1,6 +1,8 @@
+use std::cmp::Ordering;
 use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex};
+use std::time::Duration;
 
 use anyhow::{bail, format_err, Error};
 use tracing::info;
@@ -13,6 +15,7 @@ use proxmox_sys::process_locker::{
 };
 use proxmox_worker_task::WorkerTaskContext;
 
+use crate::data_blob::DataChunkBuilder;
 use crate::file_formats::{
     COMPRESSED_BLOB_MAGIC_1_0, ENCRYPTED_BLOB_MAGIC_1_0, UNCOMPRESSED_BLOB_MAGIC_1_0,
 };
@@ -93,6 +96,7 @@ impl ChunkStore {
         uid: nix::unistd::Uid,
         gid: nix::unistd::Gid,
         sync_level: DatastoreFSyncLevel,
+        atime_safety_check: bool,
     ) -> Result<Self, Error>
     where
         P: Into<PathBuf>,
@@ -147,7 +151,20 @@ impl ChunkStore {
             }
         }
 
-        Self::open(name, base, sync_level)
+        let chunk_store = Self::open(name, base, sync_level)?;
+        if atime_safety_check {
+            chunk_store
+                .atime_safety_check()
+                .map(|atime_updated| if atime_updated {
+                    info!("atime safety check successful.")
+                } else {
+                    info!("atime safety check successful with relatime behaviour.")
+                })?;
+        } else {
+            info!("atime safety check skipped.");
+        }
+
+        Ok(chunk_store)
     }
 
     fn lockfile_path<P: Into<PathBuf>>(base: P) -> PathBuf {
@@ -442,6 +459,94 @@ impl ChunkStore {
         Ok(())
     }
 
+    /// Check if atime updates are honored by the filesystem backing the chunk store.
+    ///
+    /// Checks if the atime is either update immediately by utimensat or in a relatime manner by
+    /// first setting atime and mtime to now, followed by trying to update the atime.
+    /// If the atime update is honored, return with true, if the relatime update has been honored,
+    /// return with false. Return with error otherwise.
+    /// Uses a 4 MiB fixed size, compressed but unencrypted chunk to test. The chunk is inserted in
+    /// the chunk store if not yet present.
+    pub fn atime_safety_check(&self) -> Result<bool, Error> {
+        let (zero_chunk, digest) = DataChunkBuilder::build_zero_chunk(None, 4096 * 1024, true)?;
+        self.insert_chunk(&zero_chunk, &digest)?;
+        let (path, _digest) = self.chunk_path(&digest);
+
+        let metadata_before = std::fs::metadata(&path).map_err(Error::from)?;
+        let atime_before = metadata_before.accessed()?;
+
+        // Take into account timestamp update granularity in the kernel
+        std::thread::sleep(Duration::from_secs(1));
+        self.cond_touch_path(&path, true)?;
+
+        let metadata_now = std::fs::metadata(&path).map_err(Error::from)?;
+        let atime_now = metadata_now.accessed()?;
+
+        match atime_before.cmp(&atime_now) {
+            Ordering::Less => Ok(true),
+            Ordering::Equal => {
+                // Use the previous mtime here, as that is the one the atime
+                // before update check will been compared to
+                let mtime_before = metadata_before.modified()?;
+                if atime_now < mtime_before {
+                    Err(format_err!(
+                        "atime safety check failed, is atime support enabled on datastore backing \
+                        filesystem?"
+                    ))
+                } else {
+                    self.relatime_safety_check(&path)?;
+                    Ok(false)
+                }
+            }
+            Ordering::Greater => Err(format_err!(
+                "atime safety check failed, unexpected time shift"
+            )),
+        }
+    }
+
+    fn relatime_safety_check(&self, path: &Path) -> Result<(), Error> {
+        // unwrap: only `None` in unit tests
+        assert!(self.locker.is_some());
+
+        // Update atime and mtime to now
+        let times: [libc::timespec; 2] = [
+            libc::timespec {
+                tv_sec: 0,
+                tv_nsec: libc::UTIME_NOW,
+            },
+            libc::timespec {
+                tv_sec: 0,
+                tv_nsec: libc::UTIME_NOW,
+            },
+        ];
+
+        use nix::NixPath;
+        if let Err(err) = path.with_nix_path(|cstr| unsafe {
+            let tmp = libc::utimensat(-1, cstr.as_ptr(), &times[0], libc::AT_SYMLINK_NOFOLLOW);
+            nix::errno::Errno::result(tmp)
+        })? {
+            bail!("update atime failed for chunk/file {path:?} - {err}");
+        }
+
+        // Take into account timestamp update granularity in the kernel
+        std::thread::sleep(Duration::from_secs(1));
+        // Try updating the chunks atime, which should be performed for filesystems
+        // mounted with relatime since mtime is equal
+        self.cond_touch_path(&path, true)?;
+
+        let metadata_now = std::fs::metadata(&path).map_err(Error::from)?;
+        let atime_now = metadata_now.accessed()?;
+        let mtime_now = metadata_now.modified()?;
+        if atime_now <= mtime_now {
+            bail!(
+                "atime safety check failed and relatime update failed, is atime support enabled on \
+                datastore backing filesystem?"
+            )
+        }
+
+        Ok(())
+    }
+
     pub fn insert_chunk(&self, chunk: &DataBlob, digest: &[u8; 32]) -> Result<(bool, u64), Error> {
         // unwrap: only `None` in unit tests
         assert!(self.locker.is_some());
@@ -628,8 +733,15 @@ fn test_chunk_store1() {
     let user = nix::unistd::User::from_uid(nix::unistd::Uid::current())
         .unwrap()
         .unwrap();
-    let chunk_store =
-        ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None).unwrap();
+    let chunk_store = ChunkStore::create(
+        "test",
+        &path,
+        user.uid,
+        user.gid,
+        DatastoreFSyncLevel::None,
+        true,
+    )
+    .unwrap();
 
     let (chunk, digest) = crate::data_blob::DataChunkBuilder::new(&[0u8, 1u8])
         .build()
@@ -641,8 +753,14 @@ fn test_chunk_store1() {
     let (exists, _) = chunk_store.insert_chunk(&chunk, &digest).unwrap();
     assert!(exists);
 
-    let chunk_store =
-        ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None);
+    let chunk_store = ChunkStore::create(
+        "test",
+        &path,
+        user.uid,
+        user.gid,
+        DatastoreFSyncLevel::None,
+        true,
+    );
     assert!(chunk_store.is_err());
 
     if let Err(_e) = std::fs::remove_dir_all(".testdir") { /* ignore */ }
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 75c0c16ab..ef932b47b 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1170,6 +1170,16 @@ impl DataStore {
                 upid: Some(upid.to_string()),
                 ..Default::default()
             };
+            let tuning: DatastoreTuning = serde_json::from_value(
+                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.");
+            } else {
+                info!("Filesystem atime safety check disabled by datastore tuning options.");
+            }
 
             info!("Start GC phase1 (mark used chunks)");
 
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index fe3260f6d..35847fc45 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -119,6 +119,7 @@ pub(crate) fn do_create_datastore(
                 backup_user.uid,
                 backup_user.gid,
                 tuning.sync_level.unwrap_or_default(),
+                tuning.gc_atime_safety_check.unwrap_or(true),
             )
             .map(|_| ())
         } else {
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH v3 proxmox-backup 4/9] fix #5982: garbage collection: check atime updates are honored

[Fabian Grünbichler]

On March 4, 2025 7:35 pm, Christian Ebner wrote:
> Check if the filesystem backing the chunk store actually updates the
> atime to avoid potential data loss in phase 2 of garbage collection,
> in case the atime update is not honored.
> 
> Perform the check before phase 1 of garbage collection, as well as
> on datastore creation. The latter to early detect and disallow
> datastore creation on filesystem configurations which otherwise most
> likely would lead to data losses.
> 
> Enable the atime update check by default, but allow to opt-out by
> setting a datastore tuning parameter flag for backwards compatibility.
> This is honored by both, garbage collection and datastore creation.
> 
> The check uses a 4 MiB fixed sized, unencypted and compressed chunk
> as test marker, inserted if not present. This all zero-chunk is very
> likely anyways for unencrypted backup contents with large all-zero
> regions using fixed size chunking (e.g. VMs).
> 
> To avoid cases were the timestamp will not be updated because of the
> Linux kernels timestamp granularity, sleep in-between stating and
> utimensat for 1 second.
> 
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5982
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 2:
> - Take Linux timestamp granularity into account by sleeping 1 second
>   in-between operations instead of setting timestamp to the past.
> - Check relatime behavior if atime behaviour not honored.
> - adapt datastore tuning variables to new names
> 
> 
>  pbs-datastore/src/chunk_store.rs | 128 +++++++++++++++++++++++++++++--
>  pbs-datastore/src/datastore.rs   |  10 +++
>  src/api2/config/datastore.rs     |   1 +
>  3 files changed, 134 insertions(+), 5 deletions(-)
> 
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index 5e02909a1..e529dcc9c 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -1,6 +1,8 @@
> +use std::cmp::Ordering;
>  use std::os::unix::io::AsRawFd;
>  use std::path::{Path, PathBuf};
>  use std::sync::{Arc, Mutex};
> +use std::time::Duration;
>  
>  use anyhow::{bail, format_err, Error};
>  use tracing::info;
> @@ -13,6 +15,7 @@ use proxmox_sys::process_locker::{
>  };
>  use proxmox_worker_task::WorkerTaskContext;
>  
> +use crate::data_blob::DataChunkBuilder;
>  use crate::file_formats::{
>      COMPRESSED_BLOB_MAGIC_1_0, ENCRYPTED_BLOB_MAGIC_1_0, UNCOMPRESSED_BLOB_MAGIC_1_0,
>  };
> @@ -93,6 +96,7 @@ impl ChunkStore {
>          uid: nix::unistd::Uid,
>          gid: nix::unistd::Gid,
>          sync_level: DatastoreFSyncLevel,
> +        atime_safety_check: bool,
>      ) -> Result<Self, Error>
>      where
>          P: Into<PathBuf>,
> @@ -147,7 +151,20 @@ impl ChunkStore {
>              }
>          }
>  
> -        Self::open(name, base, sync_level)
> +        let chunk_store = Self::open(name, base, sync_level)?;
> +        if atime_safety_check {
> +            chunk_store
> +                .atime_safety_check()
> +                .map(|atime_updated| if atime_updated {
> +                    info!("atime safety check successful.")
> +                } else {
> +                    info!("atime safety check successful with relatime behaviour.")
> +                })?;
> +        } else {
> +            info!("atime safety check skipped.");
> +        }
> +
> +        Ok(chunk_store)
>      }
>  
>      fn lockfile_path<P: Into<PathBuf>>(base: P) -> PathBuf {
> @@ -442,6 +459,94 @@ impl ChunkStore {
>          Ok(())
>      }
>  
> +    /// Check if atime updates are honored by the filesystem backing the chunk store.
> +    ///
> +    /// Checks if the atime is either update immediately by utimensat or in a relatime manner by
> +    /// first setting atime and mtime to now, followed by trying to update the atime.
> +    /// If the atime update is honored, return with true, if the relatime update has been honored,
> +    /// return with false. Return with error otherwise.
> +    /// Uses a 4 MiB fixed size, compressed but unencrypted chunk to test. The chunk is inserted in
> +    /// the chunk store if not yet present.
> +    pub fn atime_safety_check(&self) -> Result<bool, Error> {
> +        let (zero_chunk, digest) = DataChunkBuilder::build_zero_chunk(None, 4096 * 1024, true)?;
> +        self.insert_chunk(&zero_chunk, &digest)?;

we might want to remember whether we insert or not (return value here)
for log output below.

> +        let (path, _digest) = self.chunk_path(&digest);
> +
> +        let metadata_before = std::fs::metadata(&path).map_err(Error::from)?;
> +        let atime_before = metadata_before.accessed()?;
> +
> +        // Take into account timestamp update granularity in the kernel
> +        std::thread::sleep(Duration::from_secs(1));

small nit: if we re-order the stat and sleep above, we have higher
chances of actually testing our change and not some side-effect of
concurrent actions (see below).

> +        self.cond_touch_path(&path, true)?;

it might be worth a comment to note that this is actually most likely
the second touch (the first one having happened by insert_chunk above).

this means this test actually doesn't just test that atime updating
works, but that file creation/touching followed by a second touch
shortly after works, which is much more like what we want for GC :)

> +
> +        let metadata_now = std::fs::metadata(&path).map_err(Error::from)?;
> +        let atime_now = metadata_now.accessed()?;
> +
> +        match atime_before.cmp(&atime_now) {
> +            Ordering::Less => Ok(true),

there is a small risk of false positives here if some other action on
the file path caused atime to change (e.g., a backup uploading a
better-compressed copy of the zero chunk that becomes visible exactly at
the right moment, or whatever).

not sure whether we want to care about that, but wanted to mention it
anyway for completeness' sake. we could improve robustness by checking
that the inode and birth time is still the same, and retry the check if
not? it is probably really unlikely to hit in practice ;)

> +            Ordering::Equal => {
> +                // Use the previous mtime here, as that is the one the atime
> +                // before update check will been compared to
> +                let mtime_before = metadata_before.modified()?;
> +                if atime_now < mtime_before {
> +                    Err(format_err!(
> +                        "atime safety check failed, is atime support enabled on datastore backing \
> +                        filesystem?"
> +                    ))
> +                } else {
> +                    self.relatime_safety_check(&path)?;
> +                    Ok(false)

this would be very unexpected behaviour (it would probably make sense to
systematically test *all common* storages that we can easily test to see
if any exhibit this behaviour?), and if it occurs, how can we be sure
that it would honor the 24h (since the behaviour of that
filesystem/storage is already out of spec).

I think if we want to go down that route, we'd need to switch the
touching over to also set the mtime to be safe. we know touching in that
fashion works (since we test it here), we don't know much else about
it's semantics. or we say such setups are unsupported, and require users
to explicitly opt into potentially unsafe operations by disabling the
check.

> +                }
> +            }
> +            Ordering::Greater => Err(format_err!(
> +                "atime safety check failed, unexpected time shift"
> +            )),
> +        }
> +    }
> +
> +    fn relatime_safety_check(&self, path: &Path) -> Result<(), Error> {
> +        // unwrap: only `None` in unit tests
> +        assert!(self.locker.is_some());
> +
> +        // Update atime and mtime to now
> +        let times: [libc::timespec; 2] = [
> +            libc::timespec {
> +                tv_sec: 0,
> +                tv_nsec: libc::UTIME_NOW,
> +            },
> +            libc::timespec {
> +                tv_sec: 0,
> +                tv_nsec: libc::UTIME_NOW,
> +            },
> +        ];

this is identical to passing null to utimensat and could be written as
such ;)

> +
> +        use nix::NixPath;
> +        if let Err(err) = path.with_nix_path(|cstr| unsafe {
> +            let tmp = libc::utimensat(-1, cstr.as_ptr(), &times[0], libc::AT_SYMLINK_NOFOLLOW);
> +            nix::errno::Errno::result(tmp)
> +        })? {
> +            bail!("update atime failed for chunk/file {path:?} - {err}");

nit: error message is wrong

> +        }
> +
> +        // Take into account timestamp update granularity in the kernel
> +        std::thread::sleep(Duration::from_secs(1));
> +        // Try updating the chunks atime, which should be performed for filesystems
> +        // mounted with relatime since mtime is equal

this is a misleading comment. we don't care about normal filesystems
being mounted relatime.

what we are checking here is whether some weird filesystem/storage uses
"something like relatime semantics" for explicit timestamp updates. the
kernel itself doesn't, it resolves the explicit update and sets the file
attributes via the filesystem (utimensat -> .. -> notify_changed ->
setattr), which is a totally different code path than relatime handling
which only applies for implicit, automatic updates when doing *other* file
accesses (file_accessed -> touch_atime -> atime_needs_update, which is
also where noatime is handled).

lazytime is handled in yet another fashion (the inode is marked as
having dirty timestamps, and that is checked in various places to decide
whether to sync it out or not, but the inode in memory is always
correctly updated).

> +        self.cond_touch_path(&path, true)?;
> +
> +        let metadata_now = std::fs::metadata(&path).map_err(Error::from)?;
> +        let atime_now = metadata_now.accessed()?;
> +        let mtime_now = metadata_now.modified()?;
> +        if atime_now <= mtime_now {

we still want to check that atime_now is later than atime_before (and
maybe that mtime_now is later than mtime_before), this condition is
wrong..

> +            bail!(
> +                "atime safety check failed and relatime update failed, is atime support enabled on \
> +                datastore backing filesystem?"
> +            )
> +        }
> +
> +        Ok(())
> +    }
> +
>      pub fn insert_chunk(&self, chunk: &DataBlob, digest: &[u8; 32]) -> Result<(bool, u64), Error> {
>          // unwrap: only `None` in unit tests
>          assert!(self.locker.is_some());
> @@ -628,8 +733,15 @@ fn test_chunk_store1() {
>      let user = nix::unistd::User::from_uid(nix::unistd::Uid::current())
>          .unwrap()
>          .unwrap();
> -    let chunk_store =
> -        ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None).unwrap();
> +    let chunk_store = ChunkStore::create(
> +        "test",
> +        &path,
> +        user.uid,
> +        user.gid,
> +        DatastoreFSyncLevel::None,
> +        true,
> +    )
> +    .unwrap();
>  
>      let (chunk, digest) = crate::data_blob::DataChunkBuilder::new(&[0u8, 1u8])
>          .build()
> @@ -641,8 +753,14 @@ fn test_chunk_store1() {
>      let (exists, _) = chunk_store.insert_chunk(&chunk, &digest).unwrap();
>      assert!(exists);
>  
> -    let chunk_store =
> -        ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None);
> +    let chunk_store = ChunkStore::create(
> +        "test",
> +        &path,
> +        user.uid,
> +        user.gid,
> +        DatastoreFSyncLevel::None,
> +        true,
> +    );
>      assert!(chunk_store.is_err());
>  
>      if let Err(_e) = std::fs::remove_dir_all(".testdir") { /* ignore */ }
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 75c0c16ab..ef932b47b 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1170,6 +1170,16 @@ impl DataStore {
>                  upid: Some(upid.to_string()),
>                  ..Default::default()
>              };
> +            let tuning: DatastoreTuning = serde_json::from_value(
> +                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.");
> +            } else {
> +                info!("Filesystem atime safety check disabled by datastore tuning options.");
> +            }
>  
>              info!("Start GC phase1 (mark used chunks)");
>  
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index fe3260f6d..35847fc45 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -119,6 +119,7 @@ pub(crate) fn do_create_datastore(
>                  backup_user.uid,
>                  backup_user.gid,
>                  tuning.sync_level.unwrap_or_default(),
> +                tuning.gc_atime_safety_check.unwrap_or(true),
>              )
>              .map(|_| ())
>          } else {
> -- 
> 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

Re: [pbs-devel] [PATCH v3 proxmox-backup 4/9] fix #5982: garbage collection: check atime updates are honored

[Christian Ebner]

On 3/5/25 10:41, Fabian Grünbichler wrote:
> On March 4, 2025 7:35 pm, Christian Ebner wrote:
>> Check if the filesystem backing the chunk store actually updates the
>> atime to avoid potential data loss in phase 2 of garbage collection,
>> in case the atime update is not honored.
>>
>> Perform the check before phase 1 of garbage collection, as well as
>> on datastore creation. The latter to early detect and disallow
>> datastore creation on filesystem configurations which otherwise most
>> likely would lead to data losses.
>>
>> Enable the atime update check by default, but allow to opt-out by
>> setting a datastore tuning parameter flag for backwards compatibility.
>> This is honored by both, garbage collection and datastore creation.
>>
>> The check uses a 4 MiB fixed sized, unencypted and compressed chunk
>> as test marker, inserted if not present. This all zero-chunk is very
>> likely anyways for unencrypted backup contents with large all-zero
>> regions using fixed size chunking (e.g. VMs).
>>
>> To avoid cases were the timestamp will not be updated because of the
>> Linux kernels timestamp granularity, sleep in-between stating and
>> utimensat for 1 second.
>>
>> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5982
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>> changes since version 2:
>> - Take Linux timestamp granularity into account by sleeping 1 second
>>    in-between operations instead of setting timestamp to the past.
>> - Check relatime behavior if atime behaviour not honored.
>> - adapt datastore tuning variables to new names
>>
>>
>>   pbs-datastore/src/chunk_store.rs | 128 +++++++++++++++++++++++++++++--
>>   pbs-datastore/src/datastore.rs   |  10 +++
>>   src/api2/config/datastore.rs     |   1 +
>>   3 files changed, 134 insertions(+), 5 deletions(-)
>>
>> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
>> index 5e02909a1..e529dcc9c 100644
>> --- a/pbs-datastore/src/chunk_store.rs
>> +++ b/pbs-datastore/src/chunk_store.rs
>> @@ -1,6 +1,8 @@
>> +use std::cmp::Ordering;
>>   use std::os::unix::io::AsRawFd;
>>   use std::path::{Path, PathBuf};
>>   use std::sync::{Arc, Mutex};
>> +use std::time::Duration;
>>   
>>   use anyhow::{bail, format_err, Error};
>>   use tracing::info;
>> @@ -13,6 +15,7 @@ use proxmox_sys::process_locker::{
>>   };
>>   use proxmox_worker_task::WorkerTaskContext;
>>   
>> +use crate::data_blob::DataChunkBuilder;
>>   use crate::file_formats::{
>>       COMPRESSED_BLOB_MAGIC_1_0, ENCRYPTED_BLOB_MAGIC_1_0, UNCOMPRESSED_BLOB_MAGIC_1_0,
>>   };
>> @@ -93,6 +96,7 @@ impl ChunkStore {
>>           uid: nix::unistd::Uid,
>>           gid: nix::unistd::Gid,
>>           sync_level: DatastoreFSyncLevel,
>> +        atime_safety_check: bool,
>>       ) -> Result<Self, Error>
>>       where
>>           P: Into<PathBuf>,
>> @@ -147,7 +151,20 @@ impl ChunkStore {
>>               }
>>           }
>>   
>> -        Self::open(name, base, sync_level)
>> +        let chunk_store = Self::open(name, base, sync_level)?;
>> +        if atime_safety_check {
>> +            chunk_store
>> +                .atime_safety_check()
>> +                .map(|atime_updated| if atime_updated {
>> +                    info!("atime safety check successful.")
>> +                } else {
>> +                    info!("atime safety check successful with relatime behaviour.")
>> +                })?;
>> +        } else {
>> +            info!("atime safety check skipped.");
>> +        }
>> +
>> +        Ok(chunk_store)
>>       }
>>   
>>       fn lockfile_path<P: Into<PathBuf>>(base: P) -> PathBuf {
>> @@ -442,6 +459,94 @@ impl ChunkStore {
>>           Ok(())
>>       }
>>   
>> +    /// Check if atime updates are honored by the filesystem backing the chunk store.
>> +    ///
>> +    /// Checks if the atime is either update immediately by utimensat or in a relatime manner by
>> +    /// first setting atime and mtime to now, followed by trying to update the atime.
>> +    /// If the atime update is honored, return with true, if the relatime update has been honored,
>> +    /// return with false. Return with error otherwise.
>> +    /// Uses a 4 MiB fixed size, compressed but unencrypted chunk to test. The chunk is inserted in
>> +    /// the chunk store if not yet present.
>> +    pub fn atime_safety_check(&self) -> Result<bool, Error> {
>> +        let (zero_chunk, digest) = DataChunkBuilder::build_zero_chunk(None, 4096 * 1024, true)?;
>> +        self.insert_chunk(&zero_chunk, &digest)?;
> 
> we might want to remember whether we insert or not (return value here)
> for log output below.

Acked, will include this information as well.

> 
>> +        let (path, _digest) = self.chunk_path(&digest);
>> +
>> +        let metadata_before = std::fs::metadata(&path).map_err(Error::from)?;
>> +        let atime_before = metadata_before.accessed()?;
>> +
>> +        // Take into account timestamp update granularity in the kernel
>> +        std::thread::sleep(Duration::from_secs(1));
> 
> small nit: if we re-order the stat and sleep above, we have higher
> chances of actually testing our change and not some side-effect of
> concurrent actions (see below).

Ah yes, that's way better, a lot can happen within a second...

>> +        self.cond_touch_path(&path, true)?;
> 
> it might be worth a comment to note that this is actually most likely
> the second touch (the first one having happened by insert_chunk above).

Okay, will include a comment mentioning this explicitly.

> this means this test actually doesn't just test that atime updating
> works, but that file creation/touching followed by a second touch
> shortly after works, which is much more like what we want for GC :)
> 
>> +
>> +        let metadata_now = std::fs::metadata(&path).map_err(Error::from)?;
>> +        let atime_now = metadata_now.accessed()?;
>> +
>> +        match atime_before.cmp(&atime_now) {
>> +            Ordering::Less => Ok(true),
> 
> there is a small risk of false positives here if some other action on
> the file path caused atime to change (e.g., a backup uploading a
> better-compressed copy of the zero chunk that becomes visible exactly at
> the right moment, or whatever).
> 
> not sure whether we want to care about that, but wanted to mention it
> anyway for completeness' sake. we could improve robustness by checking
> that the inode and birth time is still the same, and retry the check if
> not? it is probably really unlikely to hit in practice ;)

Yes, had this in mind as well yesterday, another option would be to not 
rely on an actual chunk file for the test, but rather use a different 
file. But I discarded that option mainly because I didn't want to 
pollute the chunk store with other files and it actually does not fix 
the issue (the file might still be touched unexpectedly by something 
else). So checking the inode and comparing the ctime is a good 
alternative, with little cost. Will incorporate this additional checks.

> 
>> +            Ordering::Equal => {
>> +                // Use the previous mtime here, as that is the one the atime
>> +                // before update check will been compared to
>> +                let mtime_before = metadata_before.modified()?;
>> +                if atime_now < mtime_before {
>> +                    Err(format_err!(
>> +                        "atime safety check failed, is atime support enabled on datastore backing \
>> +                        filesystem?"
>> +                    ))
>> +                } else {
>> +                    self.relatime_safety_check(&path)?;
>> +                    Ok(false)
> 
> this would be very unexpected behaviour (it would probably make sense to
> systematically test *all common* storages that we can easily test to see
> if any exhibit this behaviour?), and if it occurs, how can we be sure
> that it would honor the 24h (since the behaviour of that
> filesystem/storage is already out of spec).
> 
> I think if we want to go down that route, we'd need to switch the
> touching over to also set the mtime to be safe. we know touching in that
> fashion works (since we test it here), we don't know much else about
> it's semantics. or we say such setups are unsupported, and require users
> to explicitly opt into potentially unsafe operations by disabling the
> check.

Acked, I will check if I can find a filesystem which does behave as 
tested, but if none is to be found I would opt for the latter option, 
making this an error and only allow the users to opt-out.
If there are then reports, it is still possible to extend these checks 
and adapt GC to use mtime as well or even go for a totally different 
approach.

> 
>> +                }
>> +            }
>> +            Ordering::Greater => Err(format_err!(
>> +                "atime safety check failed, unexpected time shift"
>> +            )),
>> +        }
>> +    }
>> +
>> +    fn relatime_safety_check(&self, path: &Path) -> Result<(), Error> {
>> +        // unwrap: only `None` in unit tests
>> +        assert!(self.locker.is_some());
>> +
>> +        // Update atime and mtime to now
>> +        let times: [libc::timespec; 2] = [
>> +            libc::timespec {
>> +                tv_sec: 0,
>> +                tv_nsec: libc::UTIME_NOW,
>> +            },
>> +            libc::timespec {
>> +                tv_sec: 0,
>> +                tv_nsec: libc::UTIME_NOW,
>> +            },
>> +        ];
> 
> this is identical to passing null to utimensat and could be written as
> such ;)

Acked, will adapt this accordingly.

> 
>> +
>> +        use nix::NixPath;
>> +        if let Err(err) = path.with_nix_path(|cstr| unsafe {
>> +            let tmp = libc::utimensat(-1, cstr.as_ptr(), &times[0], libc::AT_SYMLINK_NOFOLLOW);
>> +            nix::errno::Errno::result(tmp)
>> +        })? {
>> +            bail!("update atime failed for chunk/file {path:?} - {err}");
> 
> nit: error message is wrong

Acked!

> 
>> +        }
>> +
>> +        // Take into account timestamp update granularity in the kernel
>> +        std::thread::sleep(Duration::from_secs(1));
>> +        // Try updating the chunks atime, which should be performed for filesystems
>> +        // mounted with relatime since mtime is equal
> 
> this is a misleading comment. we don't care about normal filesystems
> being mounted relatime.

Ah, yes this is outdated, leftover from my iterations with lacking 
understanding of the implications from lazytime and relatime, which we 
cleared up yesterday.

> 
> what we are checking here is whether some weird filesystem/storage uses
> "something like relatime semantics" for explicit timestamp updates. the
> kernel itself doesn't, it resolves the explicit update and sets the file
> attributes via the filesystem (utimensat -> .. -> notify_changed ->
> setattr), which is a totally different code path than relatime handling
> which only applies for implicit, automatic updates when doing *other* file
> accesses (file_accessed -> touch_atime -> atime_needs_update, which is
> also where noatime is handled).
> 
> lazytime is handled in yet another fashion (the inode is marked as
> having dirty timestamps, and that is checked in various places to decide
> whether to sync it out or not, but the inode in memory is always
> correctly updated).
> 
>> +        self.cond_touch_path(&path, true)?;
>> +
>> +        let metadata_now = std::fs::metadata(&path).map_err(Error::from)?;
>> +        let atime_now = metadata_now.accessed()?;
>> +        let mtime_now = metadata_now.modified()?;
>> +        if atime_now <= mtime_now {
> 
> we still want to check that atime_now is later than atime_before (and
> maybe that mtime_now is later than mtime_before), this condition is
> wrong..

I see, will adapt  accordingly, thx!

> 
>> +            bail!(
>> +                "atime safety check failed and relatime update failed, is atime support enabled on \
>> +                datastore backing filesystem?"
>> +            )
>> +        }
>> +
>> +        Ok(())
>> +    }
>> +
>>       pub fn insert_chunk(&self, chunk: &DataBlob, digest: &[u8; 32]) -> Result<(bool, u64), Error> {
>>           // unwrap: only `None` in unit tests
>>           assert!(self.locker.is_some());
>> @@ -628,8 +733,15 @@ fn test_chunk_store1() {
>>       let user = nix::unistd::User::from_uid(nix::unistd::Uid::current())
>>           .unwrap()
>>           .unwrap();
>> -    let chunk_store =
>> -        ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None).unwrap();
>> +    let chunk_store = ChunkStore::create(
>> +        "test",
>> +        &path,
>> +        user.uid,
>> +        user.gid,
>> +        DatastoreFSyncLevel::None,
>> +        true,
>> +    )
>> +    .unwrap();
>>   
>>       let (chunk, digest) = crate::data_blob::DataChunkBuilder::new(&[0u8, 1u8])
>>           .build()
>> @@ -641,8 +753,14 @@ fn test_chunk_store1() {
>>       let (exists, _) = chunk_store.insert_chunk(&chunk, &digest).unwrap();
>>       assert!(exists);
>>   
>> -    let chunk_store =
>> -        ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None);
>> +    let chunk_store = ChunkStore::create(
>> +        "test",
>> +        &path,
>> +        user.uid,
>> +        user.gid,
>> +        DatastoreFSyncLevel::None,
>> +        true,
>> +    );
>>       assert!(chunk_store.is_err());
>>   
>>       if let Err(_e) = std::fs::remove_dir_all(".testdir") { /* ignore */ }
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>> index 75c0c16ab..ef932b47b 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -1170,6 +1170,16 @@ impl DataStore {
>>                   upid: Some(upid.to_string()),
>>                   ..Default::default()
>>               };
>> +            let tuning: DatastoreTuning = serde_json::from_value(
>> +                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.");
>> +            } else {
>> +                info!("Filesystem atime safety check disabled by datastore tuning options.");
>> +            }
>>   
>>               info!("Start GC phase1 (mark used chunks)");
>>   
>> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
>> index fe3260f6d..35847fc45 100644
>> --- a/src/api2/config/datastore.rs
>> +++ b/src/api2/config/datastore.rs
>> @@ -119,6 +119,7 @@ pub(crate) fn do_create_datastore(
>>                   backup_user.uid,
>>                   backup_user.gid,
>>                   tuning.sync_level.unwrap_or_default(),
>> +                tuning.gc_atime_safety_check.unwrap_or(true),
>>               )
>>               .map(|_| ())
>>           } else {
>> -- 
>> 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
> 
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH v3 proxmox-backup 4/9] fix #5982: garbage collection: check atime updates are honored

[Fabian Grünbichler]

On March 4, 2025 7:35 pm, Christian Ebner wrote:
> Check if the filesystem backing the chunk store actually updates the
> atime to avoid potential data loss in phase 2 of garbage collection,
> in case the atime update is not honored.
> 
> Perform the check before phase 1 of garbage collection, as well as
> on datastore creation. The latter to early detect and disallow
> datastore creation on filesystem configurations which otherwise most
> likely would lead to data losses.
> 
> Enable the atime update check by default, but allow to opt-out by
> setting a datastore tuning parameter flag for backwards compatibility.
> This is honored by both, garbage collection and datastore creation.
> 
> The check uses a 4 MiB fixed sized, unencypted and compressed chunk
> as test marker, inserted if not present. This all zero-chunk is very
> likely anyways for unencrypted backup contents with large all-zero
> regions using fixed size chunking (e.g. VMs).
> 
> To avoid cases were the timestamp will not be updated because of the
> Linux kernels timestamp granularity, sleep in-between stating and
> utimensat for 1 second.
> 
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5982
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 2:
> - Take Linux timestamp granularity into account by sleeping 1 second
>   in-between operations instead of setting timestamp to the past.
> - Check relatime behavior if atime behaviour not honored.
> - adapt datastore tuning variables to new names
> 
> 
>  pbs-datastore/src/chunk_store.rs | 128 +++++++++++++++++++++++++++++--
>  pbs-datastore/src/datastore.rs   |  10 +++
>  src/api2/config/datastore.rs     |   1 +
>  3 files changed, 134 insertions(+), 5 deletions(-)
> 
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index 5e02909a1..e529dcc9c 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -1,6 +1,8 @@
> +use std::cmp::Ordering;
>  use std::os::unix::io::AsRawFd;
>  use std::path::{Path, PathBuf};
>  use std::sync::{Arc, Mutex};
> +use std::time::Duration;
>  
>  use anyhow::{bail, format_err, Error};
>  use tracing::info;
> @@ -13,6 +15,7 @@ use proxmox_sys::process_locker::{
>  };
>  use proxmox_worker_task::WorkerTaskContext;
>  
> +use crate::data_blob::DataChunkBuilder;
>  use crate::file_formats::{
>      COMPRESSED_BLOB_MAGIC_1_0, ENCRYPTED_BLOB_MAGIC_1_0, UNCOMPRESSED_BLOB_MAGIC_1_0,
>  };
> @@ -93,6 +96,7 @@ impl ChunkStore {
>          uid: nix::unistd::Uid,
>          gid: nix::unistd::Gid,
>          sync_level: DatastoreFSyncLevel,
> +        atime_safety_check: bool,
>      ) -> Result<Self, Error>
>      where
>          P: Into<PathBuf>,
> @@ -147,7 +151,20 @@ impl ChunkStore {
>              }
>          }
>  
> -        Self::open(name, base, sync_level)
> +        let chunk_store = Self::open(name, base, sync_level)?;
> +        if atime_safety_check {
> +            chunk_store
> +                .atime_safety_check()
> +                .map(|atime_updated| if atime_updated {
> +                    info!("atime safety check successful.")
> +                } else {
> +                    info!("atime safety check successful with relatime behaviour.")

we hope to never encounter this though, should we make it sounds more
like a warning to increase chances of that information getting back to
us?

> +                })?;
> +        } else {
> +            info!("atime safety check skipped.");
> +        }
> +
> +        Ok(chunk_store)
>      }
>  
>      fn lockfile_path<P: Into<PathBuf>>(base: P) -> PathBuf {
> @@ -442,6 +459,94 @@ impl ChunkStore {
>          Ok(())
>      }
>  
> +    /// Check if atime updates are honored by the filesystem backing the chunk store.
> +    ///
> +    /// Checks if the atime is either update immediately by utimensat or in a relatime manner by
> +    /// first setting atime and mtime to now, followed by trying to update the atime.
> +    /// If the atime update is honored, return with true, if the relatime update has been honored,
> +    /// return with false. Return with error otherwise.
> +    /// Uses a 4 MiB fixed size, compressed but unencrypted chunk to test. The chunk is inserted in
> +    /// the chunk store if not yet present.
> +    pub fn atime_safety_check(&self) -> Result<bool, Error> {

not a huge fan of Result<bool, ..>, as it makes the code at the call
site harder to parse (what does true mean? what does false mean? what
does an error mean?)

why not make the result an enum encoding the expected results of the
test, with errors reserved for "failure to check"?

e.g., something like (ignore the ugly name ;))

enum ATimeSemantics {
    Working,
    RelatimeLike,
    Ignored,
}

the Ignored part could be further split if we want to differentiate the
different variants (atime < mtime and atime update doesn't work, atime
 >= mtime and atime+mtime update didn't work, atime moved to the past
during test, ..) - but I think they all have the same effect outside of
the test (if atime setting doesn't work, we can't rely on it and have to
abort GC/datastore creation), so maybe just logging the details and
having a single value is okay..

the error messages below could then be logging statements and provide
more details for all cases except "atime works as expected", like before
and after timestamps for each sub-test, ..

> +        let (zero_chunk, digest) = DataChunkBuilder::build_zero_chunk(None, 4096 * 1024, true)?;
> +        self.insert_chunk(&zero_chunk, &digest)?;
> +        let (path, _digest) = self.chunk_path(&digest);
> +
> +        let metadata_before = std::fs::metadata(&path).map_err(Error::from)?;
> +        let atime_before = metadata_before.accessed()?;
> +
> +        // Take into account timestamp update granularity in the kernel
> +        std::thread::sleep(Duration::from_secs(1));
> +        self.cond_touch_path(&path, true)?;
> +
> +        let metadata_now = std::fs::metadata(&path).map_err(Error::from)?;
> +        let atime_now = metadata_now.accessed()?;
> +
> +        match atime_before.cmp(&atime_now) {
> +            Ordering::Less => Ok(true),
> +            Ordering::Equal => {
> +                // Use the previous mtime here, as that is the one the atime
> +                // before update check will been compared to
> +                let mtime_before = metadata_before.modified()?;
> +                if atime_now < mtime_before {
> +                    Err(format_err!(
> +                        "atime safety check failed, is atime support enabled on datastore backing \
> +                        filesystem?"
> +                    ))
> +                } else {
> +                    self.relatime_safety_check(&path)?;
> +                    Ok(false)
> +                }
> +            }
> +            Ordering::Greater => Err(format_err!(
> +                "atime safety check failed, unexpected time shift"
> +            )),
> +        }
> +    }
> +
> +    fn relatime_safety_check(&self, path: &Path) -> Result<(), Error> {
> +        // unwrap: only `None` in unit tests
> +        assert!(self.locker.is_some());
> +
> +        // Update atime and mtime to now
> +        let times: [libc::timespec; 2] = [
> +            libc::timespec {
> +                tv_sec: 0,
> +                tv_nsec: libc::UTIME_NOW,
> +            },
> +            libc::timespec {
> +                tv_sec: 0,
> +                tv_nsec: libc::UTIME_NOW,
> +            },
> +        ];
> +
> +        use nix::NixPath;
> +        if let Err(err) = path.with_nix_path(|cstr| unsafe {
> +            let tmp = libc::utimensat(-1, cstr.as_ptr(), &times[0], libc::AT_SYMLINK_NOFOLLOW);
> +            nix::errno::Errno::result(tmp)
> +        })? {
> +            bail!("update atime failed for chunk/file {path:?} - {err}");
> +        }
> +
> +        // Take into account timestamp update granularity in the kernel
> +        std::thread::sleep(Duration::from_secs(1));
> +        // Try updating the chunks atime, which should be performed for filesystems
> +        // mounted with relatime since mtime is equal
> +        self.cond_touch_path(&path, true)?;
> +
> +        let metadata_now = std::fs::metadata(&path).map_err(Error::from)?;
> +        let atime_now = metadata_now.accessed()?;
> +        let mtime_now = metadata_now.modified()?;
> +        if atime_now <= mtime_now {
> +            bail!(
> +                "atime safety check failed and relatime update failed, is atime support enabled on \
> +                datastore backing filesystem?"
> +            )
> +        }
> +
> +        Ok(())
> +    }
> +
>      pub fn insert_chunk(&self, chunk: &DataBlob, digest: &[u8; 32]) -> Result<(bool, u64), Error> {
>          // unwrap: only `None` in unit tests
>          assert!(self.locker.is_some());
> @@ -628,8 +733,15 @@ fn test_chunk_store1() {
>      let user = nix::unistd::User::from_uid(nix::unistd::Uid::current())
>          .unwrap()
>          .unwrap();
> -    let chunk_store =
> -        ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None).unwrap();
> +    let chunk_store = ChunkStore::create(
> +        "test",
> +        &path,
> +        user.uid,
> +        user.gid,
> +        DatastoreFSyncLevel::None,
> +        true,
> +    )
> +    .unwrap();
>  
>      let (chunk, digest) = crate::data_blob::DataChunkBuilder::new(&[0u8, 1u8])
>          .build()
> @@ -641,8 +753,14 @@ fn test_chunk_store1() {
>      let (exists, _) = chunk_store.insert_chunk(&chunk, &digest).unwrap();
>      assert!(exists);
>  
> -    let chunk_store =
> -        ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None);
> +    let chunk_store = ChunkStore::create(
> +        "test",
> +        &path,
> +        user.uid,
> +        user.gid,
> +        DatastoreFSyncLevel::None,
> +        true,
> +    );
>      assert!(chunk_store.is_err());
>  
>      if let Err(_e) = std::fs::remove_dir_all(".testdir") { /* ignore */ }
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 75c0c16ab..ef932b47b 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1170,6 +1170,16 @@ impl DataStore {
>                  upid: Some(upid.to_string()),
>                  ..Default::default()
>              };
> +            let tuning: DatastoreTuning = serde_json::from_value(
> +                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.");
> +            } else {
> +                info!("Filesystem atime safety check disabled by datastore tuning options.");
> +            }
>  
>              info!("Start GC phase1 (mark used chunks)");
>  
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index fe3260f6d..35847fc45 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -119,6 +119,7 @@ pub(crate) fn do_create_datastore(
>                  backup_user.uid,
>                  backup_user.gid,
>                  tuning.sync_level.unwrap_or_default(),
> +                tuning.gc_atime_safety_check.unwrap_or(true),
>              )
>              .map(|_| ())
>          } else {
> -- 
> 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

[pbs-devel] [PATCH v3 proxmox-backup 5/9] ui: expose GC atime safety check flag in datastore tuning options

[Christian Ebner]

Allow to edit the atime safety check flag via the datastore tuning
options edit window.

Do not expose the flag for datastore creation as it is strongly
discouraged to create datastores on filesystems not correctly handling
atime updates as the garbage collection expects. It is nevertheless
still possible to create a datastore via the cli and pass in the
`--tuning gc-atime-safety-check=false` option.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 2:
- Adapt datastore tuning option names

 www/Utils.js                | 4 ++++
 www/datastore/OptionView.js | 8 ++++++++
 2 files changed, 12 insertions(+)

diff --git a/www/Utils.js b/www/Utils.js
index 2746ef0b5..9bd7e1615 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -846,6 +846,10 @@ Ext.define('PBS.Utils', {
 	sync = PBS.Utils.tuningOptions['sync-level'][sync ?? '__default__'];
 	options.push(`${gettext('Sync Level')}: ${sync}`);
 
+	let gc_atime_safety_check = tuning['gc-atime-safety-check'];
+	delete tuning['gc-atime-safety-check'];
+	options.push(`${gettext('GC Access Time Safety Check')}: ${gc_atime_safety_check ?? true}`);
+
 	for (const [k, v] of Object.entries(tuning)) {
 	    options.push(`${k}: ${v}`);
 	}
diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
index e1f38af6f..9ce3ce388 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -271,6 +271,14 @@ Ext.define('PBS.Datastore.Options', {
 			    deleteEmpty: true,
 			    value: '__default__',
 			},
+			{
+			    xtype: 'proxmoxcheckbox',
+			    name: 'gc-atime-safety-check',
+			    fieldLabel: gettext('GC Access Time Safety Check'),
+			    defaultValue: 1,
+			    uncheckedValue: 0,
+			    deleteDefaultValue: true,
+			},
 		    ],
 		},
 	    },
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v3 proxmox-backup 6/9] docs: mention GC atime update check for tuning options

[Christian Ebner]

Document the gc-atime-check flag and describe the behavior it
controls, by adding it as additional bullet point to the documented
datastore tuning options.

This also fixes the intendation for the cli example how to set the
sync level, to make it clear that still belongs to the previous
bullet point.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 2:
- Adapt datastore tuning option names

 docs/storage.rst | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/docs/storage.rst b/docs/storage.rst
index 490302955..46faed840 100644
--- a/docs/storage.rst
+++ b/docs/storage.rst
@@ -435,9 +435,16 @@ There are some tuning related options for the datastore that are more advanced:
 
   This can be set with:
 
-.. code-block:: console
+  .. code-block:: console
+
+    # proxmox-backup-manager datastore update <storename> --tuning 'sync-level=filesystem'
 
-  # proxmox-backup-manager datastore update <storename> --tuning 'sync-level=filesystem'
+* ``gc-atime-safety-check``: Datastore GC atime update safety check:
+  You can explicitly `enable` or `disable` the atime update safety check
+  performed on datastore creation and garbage collection. This checks if atime
+  updates are handled as expected by garbage collection and therefore avoids the
+  risk of data loss by unexpected filesystem behaviour. It is recommended to set
+  this to enabled, which is also the default value.
 
 If you want to set multiple tuning options simultaneously, you can separate them
 with a comma, like this:
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

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

[Christian Ebner]

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>,
         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
+        };
 
         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");
             }
 
             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

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

[Fabian Grünbichler]

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

[pbs-devel] [PATCH v3 proxmox-backup 8/9] ui: expose GC atime cutoff in datastore tuning option

[Christian Ebner]

Allows to set the atime cutoff for phase 2 of garbage collection in
the datastores tuning parameters. This value changes the time after
which a chunk is not considered in use anymore if it falls outside of
the cutoff window.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 2:
- Adapt datastore tuning option names
- Adapt allowed value range

 www/Utils.js                | 5 +++++
 www/datastore/OptionView.js | 9 +++++++++
 2 files changed, 14 insertions(+)

diff --git a/www/Utils.js b/www/Utils.js
index 9bd7e1615..13b5eceda 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -850,6 +850,11 @@ Ext.define('PBS.Utils', {
 	delete tuning['gc-atime-safety-check'];
 	options.push(`${gettext('GC Access Time Safety Check')}: ${gc_atime_safety_check ?? true}`);
 
+	let gc_atime_cutoff = tuning['gc-atime-cutoff'];
+	delete tuning['gc-atime-cutoff'];
+	gc_atime_cutoff = gc_atime_cutoff ?? '1445';
+	options.push(`${gettext('GC Access Time Cutoff')}: ${gc_atime_cutoff}m`);
+
 	for (const [k, v] of Object.entries(tuning)) {
 	    options.push(`${k}: ${v}`);
 	}
diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
index 9ce3ce388..9e8dcc4fb 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -279,6 +279,15 @@ Ext.define('PBS.Datastore.Options', {
 			    uncheckedValue: 0,
 			    deleteDefaultValue: true,
 			},
+			{
+			    xtype: 'proxmoxintegerfield',
+			    emptyText: Proxmox.Utils.defaultText,
+			    name: 'gc-atime-cutoff',
+			    minValue: 1,
+			    maxValue: 2880,
+			    fieldLabel: gettext('GC Access Time Cutoff (min)'),
+			    deleteEmpty: true,
+			},
 		    ],
 		},
 	    },
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v3 proxmox-backup 9/9] docs: mention gc-atime-cutoff as datastore tuning option

[Christian Ebner]

Document the gc-atime-cutoff option and describe the behavior it
controls, by adding it as additional bullet point to the
documented datastore tuning options.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 2:
- Adapt datastore tuning option names

 docs/storage.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/docs/storage.rst b/docs/storage.rst
index 46faed840..047cb3e59 100644
--- a/docs/storage.rst
+++ b/docs/storage.rst
@@ -446,6 +446,12 @@ There are some tuning related options for the datastore that are more advanced:
   risk of data loss by unexpected filesystem behaviour. It is recommended to set
   this to enabled, which is also the default value.
 
+* ``gc-atime-cutoff``: Datastore GC atime cutoff for chunk cleanup:
+  This allows to set the cutoff for which a chunk is still considered in-use
+  during phase 2 of garbage collection (given no older writers). If the
+  ``atime`` of the chunk is outside the range, it will be removed. This option
+  is only used in combination with the ``gc-atime-safety-check`` being enabled.
+
 If you want to set multiple tuning options simultaneously, you can separate them
 with a comma, like this:
 
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH v3 proxmox-backup 0/9] fix #5982: check atime update is honored

[Fabian Grünbichler]

On March 4, 2025 7:35 pm, Christian Ebner wrote:
> These patches add a check to phase 1 of garbage collection in order
> to detect when the filesystem backing the chunk store does not honor
> atime updates. This avoids possible data loss for situations where
> garbage collection could otherwise delete chunks still referenced by
> a backup snaphost's index file.
> 
> The check is performed by inserting a fixed size 4 MiB unencrypted and
> compressed chunk of all-zeros. The check is performed by setting the
> atime to 1 second in the past followed by setting it to now in order to
> test if atime updates are honored immediately. The test is performed
> on datastore creation for early detection and before garbage collection
> phase 1. The test is enabled by default, but an opt-out option can be
> set via the datastore tuning parameters for backwards compatibility.

this is outdated ;)

> 
> Further, add a datastore tuning parameter to reduce the wait period for
> chunk removal in phase 2 of garbage collection. Make this conditional on
> the atime update check being enabled and successful, to avoid possible
> data loss.
> 
> Most notable changes sice version 1 (thanks Fabian and Thomas for
> comments and suggestions):
> - Take into account Linux timestamp granularity, do not set timestamp
>   to the past, as that introduces other error paths such as lack of
>   permissions or fs limitations.
> - Check relatime behavior, if atime behaviour is not honored. Fallback
>   to original cutoff in that case.
> - Adapt tuning parameter names.

but here it is correct :)

> 
> Most notable changes sice version 1 (thanks Fabian and Thomas for
> comments and suggestions):
> - Optimize check by using the all zero chunk
> - Enable the check by default and fail GC job if not honored, but allow
>   to opt-out
> - Add GC wait period tuning option
> 
> Link to the issue in the bugtracker:
> https://bugzilla.proxmox.com/show_bug.cgi?id=5982
> 
> proxmox:
> 
> Christian Ebner (2):
>   pbs api types: add garbage collection atime safety check flag
>   pbs api types: add option to set GC chunk cleanup atime cutoff
> 
>  pbs-api-types/src/datastore.rs | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> proxmox-backup:
> 
> Christian Ebner (7):
>   datastore: use libc's timespec constants instead of redefinition
>   fix #5982: garbage collection: check atime updates are honored
>   ui: expose GC atime safety check flag in datastore tuning options
>   docs: mention GC atime update check for tuning options
>   datastore: conditionally use custom GC atime cutoff if set
>   ui: expose GC atime cutoff in datastore tuning option
>   docs: mention gc-atime-cutoff as datastore tuning option
> 
>  docs/storage.rst                 |  19 +++-
>  pbs-datastore/src/chunk_store.rs | 144 ++++++++++++++++++++++++++++---
>  pbs-datastore/src/datastore.rs   |  32 +++++++
>  src/api2/config/datastore.rs     |   1 +
>  www/Utils.js                     |   9 ++
>  www/datastore/OptionView.js      |  17 ++++
>  6 files changed, 208 insertions(+), 14 deletions(-)
> 
> -- 
> 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

Re: [pbs-devel] [PATCH v3 proxmox-backup 0/9] fix #5982: check atime update is honored

[Christian Ebner]

Superseded-by version 4:
https://lore.proxmox.com/pbs-devel/20250305151453.388817-1-c.ebner@proxmox.com/T/


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel