[pbs-devel] [PATCH proxmox-backup] tape: move media catalog locking path and interface

[Dominik Csapak <d.csapak@proxmox.com>]

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