* [pbs-devel] [PATCH proxmox-backup] tape: move media catalog locking path and interface
@ 2025-06-16 13:55 Dominik Csapak
0 siblings, 0 replies; only message in thread
From: Dominik Csapak @ 2025-06-16 13:55 UTC (permalink / raw)
To: pbs-devel
nix's flock interface is deprecated in newer versions, so let's use our
helper version with a guard instead. Since we can't use the file
descriptor for that, take that chance to move the locks off from the
files itself to their own lockfile in '/run/proxmox-backup/tape-lock'.
(For consistency and robustness, should we ever decide to atomically
replace the files instead of doing append + flush)
To handle in place upgrades, use the same 'old locking' mechanism as in
27dd73777 (fix #3935: datastore/api/backup: move datastore locking to '/run')
With the same version fixme in debian/rules.
This old locking method should be dropped after the next major release,
so with 5.x. Alternatively we could fail with a message to reboot, if we
want to remove the deprecated code earlier.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
debian/postinst | 5 +++
debian/rules | 2 +
src/bin/proxmox-backup-api.rs | 1 +
src/tape/media_catalog.rs | 84 ++++++++++++++++++++++++++++-------
src/tape/mod.rs | 22 +++++++++
5 files changed, 99 insertions(+), 15 deletions(-)
diff --git a/debian/postinst b/debian/postinst
index f38a8c66a..051636cde 100644
--- a/debian/postinst
+++ b/debian/postinst
@@ -77,6 +77,11 @@ EOF
# ensure old locking is used by the daemon until a reboot happened
touch "/run/proxmox-backup/old-locking"
fi
+
+ if dpkg --compare-versions "$2" 'lt' '3.4.2~'; then
+ # ensure old tape locking is used by the daemon until a reboot happened
+ touch "/run/proxmox-backup/old-tape-locking"
+ fi
fi
;;
diff --git a/debian/rules b/debian/rules
index 70cec4754..4bf0e22b9 100755
--- a/debian/rules
+++ b/debian/rules
@@ -22,6 +22,8 @@ ifneq ("$(wildcard .repoid)","")
endif
%:
+ @echo "fix tape locking version in postinst"
+ false
dh $@ --with=bash-completion
override_dh_auto_configure:
diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs
index b1abf99c3..91a53b6e2 100644
--- a/src/bin/proxmox-backup-api.rs
+++ b/src/bin/proxmox-backup-api.rs
@@ -59,6 +59,7 @@ async fn run() -> Result<(), Error> {
proxmox_backup::tape::create_drive_state_dir()?;
proxmox_backup::tape::create_changer_state_dir()?;
proxmox_backup::tape::create_drive_lock_dir()?;
+ proxmox_backup::tape::create_tape_lock_dir()?;
if let Err(err) = generate_auth_key() {
bail!("unable to generate auth key - {}", err);
diff --git a/src/tape/media_catalog.rs b/src/tape/media_catalog.rs
index fa8ff4058..0c042943f 100644
--- a/src/tape/media_catalog.rs
+++ b/src/tape/media_catalog.rs
@@ -3,6 +3,8 @@ use std::fs::File;
use std::io::{BufReader, Read, Seek, Write};
use std::os::unix::io::AsRawFd;
use std::path::{Path, PathBuf};
+use std::sync::LazyLock;
+use std::time::Duration;
use anyhow::{bail, format_err, Error};
use endian_trait::Endian;
@@ -15,6 +17,19 @@ use proxmox_uuid::Uuid;
use pbs_api_types::{parse_ns_and_snapshot, print_ns_and_snapshot, BackupDir, BackupNamespace};
+#[cfg(not(test))]
+use pbs_config::open_backup_lockfile;
+
+#[cfg(test)]
+fn open_backup_lockfile<P: AsRef<std::path::Path>>(
+ _path: P,
+ _timeout: Option<std::time::Duration>,
+ _exclusive: bool,
+) -> Result<pbs_config::BackupLockGuard, anyhow::Error> {
+ Ok(unsafe { pbs_config::create_mocked_lock() })
+}
+
+use crate::tape::TAPE_LOCK_DIR;
use crate::tape::{file_formats::MediaSetLabel, MediaId};
#[derive(Default)]
@@ -32,6 +47,15 @@ impl DatastoreContent {
}
}
+static OLD_TAPE_LOCKING: LazyLock<bool> = LazyLock::new(|| {
+ std::fs::exists("/run/proxmox-backup/old-tape-locking")
+ .expect("cannot read /run/proxmox-backup, please check permissions")
+});
+
+fn old_tape_locking() -> bool {
+ *OLD_TAPE_LOCKING
+}
+
/// The Media Catalog
///
/// Stores what chunks and snapshots are stored on a specific media,
@@ -89,6 +113,13 @@ impl MediaCatalog {
path
}
+ fn catalog_lock_path<P: AsRef<Path>>(base_path: P, uuid: &Uuid) -> PathBuf {
+ let mut path = base_path.as_ref().to_owned();
+ path.push(uuid.to_string());
+ path.set_extension("lck");
+ path
+ }
+
fn tmp_catalog_path<P: AsRef<Path>>(base_path: P, uuid: &Uuid) -> PathBuf {
let mut path = base_path.as_ref().to_owned();
path.push(uuid.to_string());
@@ -192,7 +223,7 @@ impl MediaCatalog {
let path = Self::catalog_path(&base_path, uuid);
let me = proxmox_lang::try_block!({
- Self::create_basedir(base_path)?;
+ Self::create_basedir(&base_path)?;
let mut file = std::fs::OpenOptions::new()
.read(true)
@@ -219,9 +250,19 @@ impl MediaCatalog {
};
// Note: lock file, to get a consistent view with load_catalog
- nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::LockExclusive)?;
- let result = me.load_catalog(&mut file, media_id.media_set_label.as_ref());
- nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::Unlock)?;
+ // FIXME: remove old locking code with 5.0
+ let result = if old_tape_locking() {
+ nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::LockExclusive)?;
+ let result = me.load_catalog(&mut file, media_id.media_set_label.as_ref());
+ nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::Unlock)?;
+ result
+ } else {
+ // use longer timeout to mimic infinite locking from old locking method, but don't
+ // block the code indefinitely
+ let lock_path = Self::catalog_lock_path(TAPE_LOCK_DIR, uuid);
+ let _guard = open_backup_lockfile(lock_path, Some(Duration::from_secs(300)), true)?;
+ me.load_catalog(&mut file, media_id.media_set_label.as_ref())
+ };
let (found_magic_number, _) = result?;
@@ -285,7 +326,7 @@ impl MediaCatalog {
let tmp_path = Self::tmp_catalog_path(&base_path, uuid);
let me = proxmox_lang::try_block!({
- let file = Self::create_temporary_database_file(base_path, uuid)?;
+ let file = Self::create_temporary_database_file(&base_path, uuid)?;
let mut me = Self {
uuid: uuid.clone(),
@@ -371,16 +412,29 @@ impl MediaCatalog {
Some(ref mut file) => {
let pending = &self.pending;
// Note: lock file, to get a consistent view with load_catalog
- nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::LockExclusive)?;
- let result: Result<(), Error> = proxmox_lang::try_block!({
- file.write_all(pending)?;
- file.flush()?;
- file.sync_data()?;
- Ok(())
- });
- nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::Unlock)?;
-
- result?;
+ let write_data = |file: &mut File| -> Result<(), Error> {
+ proxmox_lang::try_block!({
+ file.write_all(pending)?;
+ file.flush()?;
+ file.sync_data()?;
+ Ok(())
+ })
+ };
+ // FIXME: remove old locking code with 5.0
+ if old_tape_locking() {
+ nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::LockExclusive)?;
+ let result = write_data(file);
+ nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::Unlock)?;
+ result?;
+ } else {
+ // use longer timeout to mimic infinite locking from old locking method, but don't
+ // block the code indefinitely
+ let lock_path = Self::catalog_lock_path(TAPE_LOCK_DIR, &self.uuid);
+ let _guard =
+ open_backup_lockfile(lock_path, Some(Duration::from_secs(300)), true)?;
+
+ write_data(file)?;
+ }
}
None => bail!("media catalog not writable (opened read only)"),
}
diff --git a/src/tape/mod.rs b/src/tape/mod.rs
index f276f9482..096e851a7 100644
--- a/src/tape/mod.rs
+++ b/src/tape/mod.rs
@@ -38,6 +38,9 @@ pub use pool_writer::*;
/// Directory path where we store all tape status information
pub const TAPE_STATUS_DIR: &str = concat!(PROXMOX_BACKUP_STATE_DIR_M!(), "/tape");
+/// Directory path where we store tape lock files
+pub const TAPE_LOCK_DIR: &str = concat!(PROXMOX_BACKUP_RUN_DIR_M!(), "/tape-lock");
+
/// Directory path where we store drive lock file
pub const DRIVE_LOCK_DIR: &str = concat!(PROXMOX_BACKUP_RUN_DIR_M!(), "/drive-lock");
@@ -93,6 +96,25 @@ pub fn create_drive_lock_dir() -> Result<(), Error> {
Ok(())
}
+/// Create tape lock dir with correct permission
+pub fn create_tape_lock_dir() -> Result<(), Error> {
+ let backup_user = pbs_config::backup_user()?;
+ let mode = nix::sys::stat::Mode::from_bits_truncate(0o0750);
+ let options = CreateOptions::new()
+ .perm(mode)
+ .owner(backup_user.uid)
+ .group(backup_user.gid);
+
+ let parent_opts = CreateOptions::new()
+ .owner(backup_user.uid)
+ .group(backup_user.gid);
+
+ create_path(TAPE_LOCK_DIR, Some(parent_opts), Some(options))
+ .map_err(|err: Error| format_err!("unable to create tape lock dir - {}", err))?;
+
+ Ok(())
+}
+
/// Create drive state dir with correct permission
pub fn create_drive_state_dir() -> Result<(), Error> {
let backup_user = pbs_config::backup_user()?;
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2025-06-16 13:55 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-06-16 13:55 [pbs-devel] [PATCH proxmox-backup] tape: move media catalog locking path and interface Dominik Csapak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.