Re: [PATCH proxmox-backup v3 05/10] datastore: move lock files base path constant to central location

[Robert Obkircher <r.obkircher@proxmox.com>]

> Moves the implementation to a more central location, as it is not
> only used by the datastore contents, but also by the chunk store.
> 
> No functional changes.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>
> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
> index fbef8e85a..e01ae8ab2 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::{DATASTORE_LOCKS_DIR, DataBlob, DataStore, DatastoreBackend};
> +use crate::{DATASTORE_LOCKS_DIR, DataBlob, DataStore, DatastoreBackend, lock_helper};
>  
>  pub const PROTECTED_MARKER_FILENAME: &str = ".protected";
>  
> @@ -1212,34 +1212,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 cfd920dea..c06b7f29b 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -947,7 +947,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 0904d7da2..c17f9591b 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::{Error, bail};
> +
> +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!(),
> @@ -236,3 +243,34 @@ 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(crate) fn lock_helper<F>(
> +    store_name: &str,
> +    path: &Path,
> +    lock_fn: F,
> +) -> Result<BackupLockGuard, Error>
> +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) {
Unrelated to the move, but this should technically also compare st_dev
because the inode number is not unique.

-- 
Robert Obkircher <r.obkircher@proxmox.com>