From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id B00311FF191
for <inbox@lore.proxmox.com>; Mon, 16 Jun 2025 15:55:52 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 0557C926B;
Mon, 16 Jun 2025 15:56:21 +0200 (CEST)
From: Dominik Csapak <d.csapak@proxmox.com>
To: pbs-devel@lists.proxmox.com
Date: Mon, 16 Jun 2025 15:55:46 +0200
Message-Id: <20250616135546.3196306-1-d.csapak@proxmox.com>
X-Mailer: git-send-email 2.39.5
MIME-Version: 1.0
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.021 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [proxmox-backup-api.rs, mod.rs]
Subject: [pbs-devel] [PATCH proxmox-backup] tape: move media catalog locking
path and interface
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.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