Re: [PATCH proxmox-backup 3/4] datastore: move file lock helper to centralized place

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

On May 5, 2026 10:11 am, Christian Ebner wrote:
> Moves the implementation to a more central location, as it is not
> only used by the datastore contents, but also by the chunk store.

we could also consider moving it to its own place?

we have:
- lock_chunk in the chunk_store (has no dependency on the
  chunk/datastore other than its name)
- lock_file_path_helper in backup_info
- lock_helper
- the DIR constant
- lock_path implementations for
-- chunk (digest)
-- group
-- snapshot
-- manifests

I've also wondered in the past a few times whether we'd not want to make
the BackupLockGuard generic to encode *what* kind of entity is being
locked, to improve function signatures.. though that might be a bigger
change ;)

> 
> No functional changes.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-datastore/src/backup_info.rs | 35 ++------------------------------
>  pbs-datastore/src/chunk_store.rs |  2 +-
>  pbs-datastore/src/lib.rs         | 34 +++++++++++++++++++++++++++++++
>  3 files changed, 37 insertions(+), 34 deletions(-)
> 
> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
> index 9919b908a..bae136d3c 100644
> --- a/pbs-datastore/src/backup_info.rs
> +++ b/pbs-datastore/src/backup_info.rs
> @@ -1,5 +1,5 @@
>  use std::fmt;
> -use std::os::unix::io::{AsRawFd, RawFd};
> +use std::os::unix::io::RawFd;
>  use std::os::unix::prelude::OsStrExt;
>  use std::path::Path;
>  use std::path::PathBuf;
> @@ -24,7 +24,7 @@ use crate::datastore::{GROUP_NOTES_FILE_NAME, GROUP_OWNER_FILE_NAME};
>  use crate::manifest::{BackupManifest, MANIFEST_LOCK_NAME};
>  use crate::move_journal;
>  use crate::s3::S3_CONTENT_PREFIX;
> -use crate::{DataBlob, DataStore, DatastoreBackend, DATASTORE_LOCKS_DIR};
> +use crate::{lock_helper, DataBlob, DataStore, DatastoreBackend, DATASTORE_LOCKS_DIR};
>  
>  pub const PROTECTED_MARKER_FILENAME: &str = ".protected";
>  
> @@ -1210,34 +1210,3 @@ fn lock_file_path_helper(ns: &BackupNamespace, path: PathBuf) -> PathBuf {
>  
>      to_return.join(format!("{first_eigthy}...{last_eighty}-{hash}"))
>  }
> -
> -/// Helps implement the double stat'ing procedure. It avoids certain race conditions upon lock
> -/// deletion.
> -///
> -/// It also creates the base directory for lock files.
> -pub(crate) fn lock_helper<F>(
> -    store_name: &str,
> -    path: &std::path::Path,
> -    lock_fn: F,
> -) -> Result<BackupLockGuard, Error>
> -where
> -    F: Fn(&std::path::Path) -> Result<BackupLockGuard, Error>,
> -{
> -    let mut lock_dir = Path::new(DATASTORE_LOCKS_DIR).join(store_name);
> -
> -    if let Some(parent) = path.parent() {
> -        lock_dir = lock_dir.join(parent);
> -    };
> -
> -    std::fs::create_dir_all(&lock_dir)?;
> -
> -    let lock = lock_fn(path)?;
> -
> -    let inode = nix::sys::stat::fstat(lock.as_raw_fd())?.st_ino;
> -
> -    if nix::sys::stat::stat(path).map_or(true, |st| inode != st.st_ino) {
> -        bail!("could not acquire lock, another thread modified the lock file");
> -    }
> -
> -    Ok(lock)
> -}
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index 5b07ebdaa..778897974 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -944,7 +944,7 @@ impl ChunkStore {
>          timeout: Duration,
>      ) -> Result<BackupLockGuard, Error> {
>          let lock_path = self.chunk_lock_path(digest);
> -        let guard = crate::backup_info::lock_helper(self.name(), &lock_path, |path| {
> +        let guard = crate::lock_helper(self.name(), &lock_path, |path| {
>              pbs_config::open_backup_lockfile(path, Some(timeout), true)
>          })?;
>          Ok(guard)
> diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
> index 29d203f87..48acba4c8 100644
> --- a/pbs-datastore/src/lib.rs
> +++ b/pbs-datastore/src/lib.rs
> @@ -157,6 +157,13 @@
>  
>  #![deny(unsafe_op_in_unsafe_fn)]
>  
> +use std::os::unix::io::AsRawFd;
> +use std::path::Path;
> +
> +use anyhow::{bail, Error};
> +
> +use pbs_config::BackupLockGuard;
> +
>  /// Directory path where active operations counters are saved.
>  pub const ACTIVE_OPERATIONS_DIR: &str = concat!(
>      pbs_buildcfg::PROXMOX_BACKUP_RUN_DIR_M!(),
> @@ -237,3 +244,30 @@ pub use local_chunk_reader::LocalChunkReader;
>  
>  mod local_datastore_lru_cache;
>  pub use local_datastore_lru_cache::LocalDatastoreLruCache;
> +
> +/// Helps implement the double stat'ing procedure. It avoids certain race conditions upon lock
> +/// deletion.
> +///
> +/// It also creates the base directory for lock files.
> +pub fn lock_helper<F>(store_name: &str, path: &Path, lock_fn: F) -> Result<BackupLockGuard, Error>

in any case, this does not need to be pub, `pub(crate)` is enough..

> +where
> +    F: Fn(&Path) -> Result<BackupLockGuard, Error>,
> +{
> +    let mut lock_dir = Path::new(DATASTORE_LOCKS_DIR).join(store_name);
> +
> +    if let Some(parent) = path.parent() {
> +        lock_dir = lock_dir.join(parent);
> +    };
> +
> +    std::fs::create_dir_all(&lock_dir)?;
> +
> +    let lock = lock_fn(path)?;
> +
> +    let inode = nix::sys::stat::fstat(lock.as_raw_fd())?.st_ino;
> +
> +    if nix::sys::stat::stat(path).map_or(true, |st| inode != st.st_ino) {
> +        bail!("could not acquire lock, another thread modified the lock file");
> +    }
> +
> +    Ok(lock)
> +}
> -- 
> 2.47.3
> 
> 
> 
> 
> 
>