* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal