Re: [pbs-devel] [PATCH proxmox-backup v9 2/4] fix #3935: datastore/api/backup: move datastore locking to '/run'

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 26.03.25 um 12:44 schrieb Shannon Sterz:
> to avoid issues when removing a group or snapshot directory where two
> threads hold a lock to the same directory, move locking to the tmpfs
> backed '/run' directory. also adds double stat'ing to make it possible
> to remove locks without certain race condition issues.
> 
> this new mechanism is only employed when we can be sure, that a reboot
> has occured so that all processes are using the new locking mechanism.
> otherwise, two separate process could assume they have exclusive
> rights to a group or snapshot.
> 
> bumps the rust version to 1.81 so we can use `std::fs::exists` without
> issue.
> 
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
>  Cargo.toml                           |   2 +-
>  debian/postinst                      |   5 +
>  debian/rules                         |   2 +
>  pbs-config/src/lib.rs                |  32 ++++-
>  pbs-datastore/Cargo.toml             |   1 +
>  pbs-datastore/src/backup_info.rs     | 195 +++++++++++++++++++++++----
>  pbs-datastore/src/datastore.rs       |   6 +-
>  pbs-datastore/src/snapshot_reader.rs |  15 ++-
>  src/api2/backup/environment.rs       |   5 +-
>  src/backup/verify.rs                 |   4 +-
>  src/server/sync.rs                   |   4 +-
>  11 files changed, 229 insertions(+), 42 deletions(-)
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index 306d50e9..645f2d81 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -13,7 +13,7 @@ authors = [
>  edition = "2021"
>  license = "AGPL-3"
>  repository = "https://git.proxmox.com/?p=proxmox-backup.git"
> -rust-version = "1.80"
> +rust-version = "1.81"
>  
>  [package]
>  name = "proxmox-backup"
> diff --git a/debian/postinst b/debian/postinst
> index 0485ca34..81c15826 100644
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -80,6 +80,11 @@ EOF
>  				update_sync_job "$prev_job"
>  			fi
>  		fi
> +
> +		if dpkg --compare-versions "$2" 'lt' '3.3.4~'; then
> +			# ensure old locking is used by the daemon until a reboot happened
> +			touch "/run/proxmox-backup/old-locking"
> +		fi
>  	fi
>      ;;
>  
> diff --git a/debian/rules b/debian/rules
> index a03fe11b..6e1844e1 100755
> --- a/debian/rules
> +++ b/debian/rules
> @@ -22,6 +22,8 @@ ifneq ("$(wildcard .repoid)","")
>  endif
>  
>  %:
> +	@echo "fix locking version in postinst"
> +	false
>  	dh $@ --with=bash-completion
>  
>  override_dh_auto_configure:
> diff --git a/pbs-config/src/lib.rs b/pbs-config/src/lib.rs
> index 20a8238d..9c4d77c2 100644
> --- a/pbs-config/src/lib.rs
> +++ b/pbs-config/src/lib.rs
> @@ -22,6 +22,8 @@ pub use config_version_cache::ConfigVersionCache;
>  
>  use anyhow::{format_err, Error};
>  use nix::unistd::{Gid, Group, Uid, User};
> +use proxmox_sys::fs::DirLockGuard;
> +use std::os::unix::prelude::AsRawFd;
>  
>  pub use pbs_buildcfg::{BACKUP_GROUP_NAME, BACKUP_USER_NAME};
>  
> @@ -46,13 +48,34 @@ pub fn backup_group() -> Result<nix::unistd::Group, Error> {
>  }
>  
>  pub struct BackupLockGuard {
> -    _file: Option<std::fs::File>,
> +    file: Option<std::fs::File>,
> +    // TODO: Remove `_legacy_dir` with PBS 5
> +    _legacy_dir: Option<DirLockGuard>,
> +}
> +
> +impl AsRawFd for BackupLockGuard {
> +    fn as_raw_fd(&self) -> i32 {
> +        self.file.as_ref().map_or(-1, |f| f.as_raw_fd())
> +    }
> +}
> +
> +// TODO: Remove with PBS 5
> +impl From<DirLockGuard> for BackupLockGuard {
> +    fn from(value: DirLockGuard) -> Self {
> +        Self {
> +            file: None,
> +            _legacy_dir: Some(value),
> +        }
> +    }
>  }
>  
>  #[doc(hidden)]
>  /// Note: do not use for production code, this is only intended for tests
>  pub unsafe fn create_mocked_lock() -> BackupLockGuard {
> -    BackupLockGuard { _file: None }
> +    BackupLockGuard {
> +        file: None,
> +        _legacy_dir: None,
> +    }
>  }
>  
>  /// Open or create a lock file owned by user "backup" and lock it.
> @@ -76,7 +99,10 @@ pub fn open_backup_lockfile<P: AsRef<std::path::Path>>(
>      let timeout = timeout.unwrap_or(std::time::Duration::new(10, 0));
>  
>      let file = proxmox_sys::fs::open_file_locked(&path, timeout, exclusive, options)?;
> -    Ok(BackupLockGuard { _file: Some(file) })
> +    Ok(BackupLockGuard {
> +        file: Some(file),
> +        _legacy_dir: None,
> +    })
>  }
>  
>  /// Atomically write data to file owned by "root:backup" with permission "0640"
> diff --git a/pbs-datastore/Cargo.toml b/pbs-datastore/Cargo.toml
> index 4ebc5fdc..7623adc2 100644
> --- a/pbs-datastore/Cargo.toml
> +++ b/pbs-datastore/Cargo.toml
> @@ -35,6 +35,7 @@ proxmox-lang.workspace=true
>  proxmox-schema = { workspace = true, features = [ "api-macro" ] }
>  proxmox-serde = { workspace = true, features = [ "serde_json" ] }
>  proxmox-sys.workspace = true
> +proxmox-systemd.workspace = true
>  proxmox-time.workspace = true
>  proxmox-uuid.workspace = true
>  proxmox-worker-task.workspace = true
> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
> index c7f7ed53..4e381142 100644
> --- a/pbs-datastore/src/backup_info.rs
> +++ b/pbs-datastore/src/backup_info.rs
> @@ -1,13 +1,15 @@
>  use std::fmt;
> -use std::os::unix::io::RawFd;
> +use std::os::unix::io::{AsRawFd, RawFd};
> +use std::os::unix::prelude::OsStrExt;
> +use std::path::Path;
>  use std::path::PathBuf;
> -use std::sync::Arc;
> +use std::sync::{Arc, LazyLock};
> +use std::time::Duration;
>  
>  use anyhow::{bail, format_err, Context, Error};
>  
> -use proxmox_sys::fs::{
> -    lock_dir_noblock, lock_dir_noblock_shared, replace_file, CreateOptions, DirLockGuard,
> -};
> +use proxmox_sys::fs::{lock_dir_noblock, lock_dir_noblock_shared, replace_file, CreateOptions};
> +use proxmox_systemd::escape_unit;
>  
>  use pbs_api_types::{
>      Authid, BackupGroupDeleteStats, BackupNamespace, BackupType, GroupFilter, VerifyState,
> @@ -18,6 +20,18 @@ use pbs_config::{open_backup_lockfile, BackupLockGuard};
>  use crate::manifest::{BackupManifest, MANIFEST_LOCK_NAME};
>  use crate::{DataBlob, DataStore};
>  
> +pub const DATASTORE_LOCKS_DIR: &str = "/run/proxmox-backup/locks";

I was shortly tempted to question if this should be in /run/lock/proxmox-backup,
but we already keep the index manifest locks in there, so it's probably fine
and might be even stranger if we would move just these new lock paths to
/run/lock/proxmox-backup I think. We could always create a symlink that points
/run/lock/proxmox-backup -> /run/proxmox-backup/locks if one really questions this.

So just to vent this thought out, it's fine as is.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel