[pbs-devel] [PATCH v3 proxmox-backup 4/9] fix #5982: garbage collection: check atime updates are honored
Fabian Grünbichler
f.gruenbichler at proxmox.com
Wed Mar 5 10:41:39 CET 2025
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 at 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(), ×[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 at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
More information about the pbs-devel
mailing list