[pbs-devel] [PATCH v6 proxmox-backup 3/8] fix #5982: garbage collection: check atime updates are honored
Wolfgang Bumiller
w.bumiller at proxmox.com
Thu Mar 20 09:36:35 CET 2025
On Wed, Mar 19, 2025 at 06:24:27PM +0100, 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. To perform the check also when
> reusing an existing datastore, open the chunks store also on reuse.
>
> 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 chunk insert
> (including an atime update if pre-existing) and the subsequent
> stating + 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 5:
> - Also perform check when reusing datastores, not just new one.
> - Expand on comment for sleep, fix incorrect wording in commit message.
> - Add additional error context to before/after stat calls.
> - Soften wording of error messages if check is skipped because disabled.
>
> pbs-datastore/src/chunk_store.rs | 72 ++++++++++++++++++++++++++++++--
> pbs-datastore/src/datastore.rs | 13 ++++++
> src/api2/config/datastore.rs | 35 ++++++++++++----
> 3 files changed, 109 insertions(+), 11 deletions(-)
>
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index 5e02909a1..dcb499426 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,
> };
> @@ -177,7 +180,7 @@ impl ChunkStore {
> /// Note that this must be used with care, as it's dangerous to create two instances on the
> /// same base path, as closing the underlying ProcessLocker drops all locks from this process
> /// on the lockfile (even if separate FDs)
> - pub(crate) fn open<P: Into<PathBuf>>(
> + pub fn open<P: Into<PathBuf>>(
> name: &str,
> base: P,
> sync_level: DatastoreFSyncLevel,
> @@ -442,6 +445,69 @@ 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
> + // Blocking the thread is fine here since this runs in a worker.
> + std::thread::sleep(Duration::from_secs(1));
> +
> + let metadata_before = std::fs::metadata(&path).context(format!(
> + "failed to get metadata for {path:?} before atime update"
> + ))?;
> +
> + // 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:?} after atime update"
> + ))?;
> +
> + // 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),
I wonder if rendering `UNIX_EPOCH` makes sense for failed values, we
could just use `.ok()` and have it debug-render the `Option`?
> + );
> + 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());
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index a6a91ca79..09b5808e0 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:#}"))?;
^ I see we have `{:#}` only exactly once in our code base from one of
your patches. Do we really want to use this form?
>From what I can tell, this chains contexts with colons.
I think we mostly use `.context`/`.with_context` nowadays instead?
That way the choice of formatting is left to the caller and they can
choose between `{}`, `{:#}` and `{:?}` and `{:#?}`...
> + info!("Access time update check successful, proceeding with GC.");
> + } else {
> + info!("Access time update 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..7e97a7de3 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -4,7 +4,7 @@ use ::serde::{Deserialize, Serialize};
> use anyhow::{bail, format_err, Error};
> use hex::FromHex;
> use serde_json::Value;
> -use tracing::warn;
> +use tracing::{info, warn};
>
> use proxmox_router::{http_bail, Permission, Router, RpcEnvironment, RpcEnvironmentType};
> use proxmox_schema::{api, param_bail, ApiType};
> @@ -98,7 +98,15 @@ pub(crate) fn do_create_datastore(
> )?;
>
> let res = if reuse_datastore {
> - ChunkStore::verify_chunkstore(&path)
> + ChunkStore::verify_chunkstore(&path).and_then(|_| {
> + // Must be the only instance accessing and locking the chunk store,
> + // dropping will close all other locks from this process on the lockfile as well.
> + ChunkStore::open(
> + &datastore.name,
> + &path,
> + tuning.sync_level.unwrap_or_default(),
> + )
> + })
> } else {
> let mut is_empty = true;
> if let Ok(dir) = std::fs::read_dir(&path) {
> @@ -120,19 +128,30 @@ pub(crate) fn do_create_datastore(
> backup_user.gid,
> tuning.sync_level.unwrap_or_default(),
> )
> - .map(|_| ())
> } else {
> Err(format_err!("datastore path not empty"))
> }
> };
>
> - if res.is_err() {
> - if need_unmount {
> - if let Err(e) = unmount_by_mountpoint(&path) {
> - warn!("could not unmount device: {e}");
> + match res {
> + Ok(chunk_store) => {
> + if tuning.gc_atime_safety_check.unwrap_or(true) {
> + chunk_store
> + .check_fs_atime_updates(true)
> + .map_err(|err| format_err!("access time safety check failed - {err:#}"))?;
^ same here
> + info!("Access time update check successful.");
> + } else {
> + info!("Access time update check skipped.");
> + }
> + }
> + Err(err) => {
> + if need_unmount {
> + if let Err(e) = unmount_by_mountpoint(&path) {
> + warn!("could not unmount device: {e}");
> + }
> }
> + return Err(err);
> }
> - return res;
> }
>
> config.set_data(&datastore.name, "datastore", &datastore)?;
> --
> 2.39.5
More information about the pbs-devel
mailing list