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

Fabian Grünbichler
2026-05-05T12:15:11Z

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
>
>
>
>
>
>