From: Christian Ebner <c.ebner@proxmox.com>
To: pbs-devel@lists.proxmox.com
Subject: [pbs-devel] [PATCH v5 proxmox-backup 3/8] fix #5982: garbage collection: check atime updates are honored
Date: Thu, 6 Mar 2025 15:52:47 +0100 [thread overview]
Message-ID: <20250306145252.565270-4-c.ebner@proxmox.com> (raw)
In-Reply-To: <20250306145252.565270-1-c.ebner@proxmox.com>
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 4:
- Improve logging of values if atime update fails.
- fix incorrect comment
pbs-datastore/src/chunk_store.rs | 101 ++++++++++++++++++++++++++++---
pbs-datastore/src/datastore.rs | 13 ++++
src/api2/config/datastore.rs | 1 +
3 files changed, 108 insertions(+), 7 deletions(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 5e02909a1..a8c826353 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -1,9 +1,11 @@
+use std::os::unix::fs::MetadataExt;
use std::os::unix::io::AsRawFd;
use std::path::{Path, PathBuf};
use std::sync::{Arc, Mutex};
+use std::time::{Duration, UNIX_EPOCH};
-use anyhow::{bail, format_err, Error};
-use tracing::info;
+use anyhow::{bail, format_err, Context, Error};
+use tracing::{info, warn};
use pbs_api_types::{DatastoreFSyncLevel, GarbageCollectionStatus};
use proxmox_io::ReadExt;
@@ -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,17 @@ impl ChunkStore {
}
}
- Self::open(name, base, sync_level)
+ let chunk_store = Self::open(name, base, sync_level)?;
+ if atime_safety_check {
+ chunk_store
+ .check_fs_atime_updates(true)
+ .map_err(|err| format_err!("access time safety check failed - {err:#}"))?;
+ info!("Access time safety check successful.");
+ } else {
+ info!("Access time safety check skipped.");
+ }
+
+ Ok(chunk_store)
}
fn lockfile_path<P: Into<PathBuf>>(base: P) -> PathBuf {
@@ -442,6 +456,66 @@ impl ChunkStore {
Ok(())
}
+ /// Check if atime updates are honored by the filesystem backing the chunk store.
+ ///
+ /// Checks if the atime is always updated by utimensat taking into consideration the Linux
+ /// kernel timestamp granularity.
+ /// If `retry_on_file_changed` is set to true, the check is performed again on the changed file
+ /// if a file change while testing is detected by differences in bith time or inode number.
+ /// Uses a 4 MiB fixed size, compressed but unencrypted chunk to test. The chunk is inserted in
+ /// the chunk store if not yet present.
+ /// Returns with error if the check could not be performed.
+ pub fn check_fs_atime_updates(&self, retry_on_file_changed: bool) -> Result<(), Error> {
+ let (zero_chunk, digest) = DataChunkBuilder::build_zero_chunk(None, 4096 * 1024, true)?;
+ let (pre_existing, _) = self.insert_chunk(&zero_chunk, &digest)?;
+ let (path, _digest) = self.chunk_path(&digest);
+
+ // Take into account timestamp update granularity in the kernel
+ std::thread::sleep(Duration::from_secs(1));
+
+ let metadata_before =
+ std::fs::metadata(&path).context(format!("failed to get metadata for {path:?}"))?;
+
+ // Second atime update if chunk pre-existed, insert_chunk already updates pre-existing ones
+ self.cond_touch_path(&path, true)?;
+
+ let metadata_now =
+ std::fs::metadata(&path).context(format!("failed to get metadata for {path:?}"))?;
+
+ // Check for the unlikely case that the file changed in-between the
+ // two metadata calls, try to check once again on changed file
+ if metadata_before.ino() != metadata_now.ino() {
+ if retry_on_file_changed {
+ return self.check_fs_atime_updates(false);
+ }
+ bail!("chunk {path:?} changed twice during access time safety check, cannot proceed.");
+ }
+
+ if metadata_before.accessed()? >= metadata_now.accessed()? {
+ let chunk_info_str = if pre_existing {
+ "pre-existing"
+ } else {
+ "newly inserted"
+ };
+ warn!("Chunk metadata was not correctly updated during access time safety check:");
+ info!(
+ "Timestamps before update: accessed {:?}, modified {:?}, created {:?}",
+ metadata_before.accessed().unwrap_or(UNIX_EPOCH),
+ metadata_before.modified().unwrap_or(UNIX_EPOCH),
+ metadata_before.created().unwrap_or(UNIX_EPOCH),
+ );
+ info!(
+ "Timestamps after update: accessed {:?}, modified {:?}, created {:?}",
+ metadata_now.accessed().unwrap_or(UNIX_EPOCH),
+ metadata_now.modified().unwrap_or(UNIX_EPOCH),
+ metadata_now.created().unwrap_or(UNIX_EPOCH),
+ );
+ bail!("access time safety check using {chunk_info_str} chunk failed, aborting GC!");
+ }
+
+ 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 +702,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 +722,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..5558bb1ac 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1170,6 +1170,19 @@ 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
+ .check_fs_atime_updates(true)
+ .map_err(|err| format_err!("atime safety check failed - {err:#}"))?;
+ info!("Access time safety check successful, proceeding with GC.");
+ } 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
next prev parent reply other threads:[~2025-03-06 14:53 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-06 14:52 [pbs-devel] [PATCH v5 proxmox-backup 0/8] fix #5982: check atime update is honored Christian Ebner
2025-03-06 14:52 ` [pbs-devel] [PATCH v5 proxmox 1/8] pbs api types: add garbage collection atime safety check flag Christian Ebner
2025-03-06 14:52 ` [pbs-devel] [PATCH v5 proxmox 2/8] pbs api types: add option to set GC chunk cleanup atime cutoff Christian Ebner
2025-03-06 14:52 ` Christian Ebner [this message]
2025-03-06 14:52 ` [pbs-devel] [PATCH v5 proxmox-backup 4/8] ui: expose GC atime safety check flag in datastore tuning options Christian Ebner
2025-03-06 14:52 ` [pbs-devel] [PATCH v5 proxmox-backup 5/8] docs: mention GC atime update check for " Christian Ebner
2025-03-06 14:52 ` [pbs-devel] [PATCH v5 proxmox-backup 6/8] datastore: use custom GC atime cutoff if set Christian Ebner
2025-03-06 14:52 ` [pbs-devel] [PATCH v5 proxmox-backup 7/8] ui: expose GC atime cutoff in datastore tuning option Christian Ebner
2025-03-06 14:52 ` [pbs-devel] [PATCH v5 proxmox-backup 8/8] docs: mention gc-atime-cutoff as " Christian Ebner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250306145252.565270-4-c.ebner@proxmox.com \
--to=c.ebner@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal