From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 63EF18B2CF for ; Wed, 24 Aug 2022 14:57:19 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 61EF61A245 for ; Wed, 24 Aug 2022 14:57:19 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 24 Aug 2022 14:57:15 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id DE28D4354C for ; Wed, 24 Aug 2022 14:48:44 +0200 (CEST) From: Stefan Sterz To: pbs-devel@lists.proxmox.com Date: Wed, 24 Aug 2022 14:48:26 +0200 Message-Id: <20220824124829.392189-3-s.sterz@proxmox.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20220824124829.392189-1-s.sterz@proxmox.com> References: <20220824124829.392189-1-s.sterz@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.046 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: [pbs-devel] [PATCH proxmox-backup v5 2/5] fix #3935: datastore/api/backup: add lock helpers to backup dir/group X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Aug 2022 12:57:19 -0000 to avoid duplicate code, add helpers for locking groups and snapshots to the BackupGroup and BackupDir traits respectively and refactor existing code to use them. Signed-off-by: Stefan Sterz --- pbs-datastore/src/backup_info.rs | 31 ++++++++++--- pbs-datastore/src/datastore.rs | 68 +++++++++------------------- pbs-datastore/src/snapshot_reader.rs | 8 +--- src/api2/backup/mod.rs | 8 +--- src/api2/reader/mod.rs | 7 +-- src/backup/verify.rs | 7 +-- 6 files changed, 52 insertions(+), 77 deletions(-) diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs index c3ce7a1d..52d927ed 100644 --- a/pbs-datastore/src/backup_info.rs +++ b/pbs-datastore/src/backup_info.rs @@ -6,7 +6,9 @@ use std::sync::Arc; use anyhow::{bail, format_err, Error}; -use proxmox_sys::fs::{lock_dir_noblock, replace_file, CreateOptions}; +use proxmox_sys::fs::{ + lock_dir_noblock, lock_dir_noblock_shared, replace_file, CreateOptions, DirLockGuard, +}; use pbs_api_types::{ Authid, BackupNamespace, BackupType, GroupFilter, BACKUP_DATE_REGEX, BACKUP_FILE_REGEX, @@ -200,9 +202,8 @@ impl BackupGroup { /// /// Returns true if all snapshots were removed, and false if some were protected pub fn destroy(&self) -> Result { + let _guard = self.lock()?; let path = self.full_group_path(); - let _guard = - proxmox_sys::fs::lock_dir_noblock(&path, "backup group", "possible running backup")?; log::info!("removing backup group {:?}", path); let mut removed_all_snaps = true; @@ -236,6 +237,15 @@ impl BackupGroup { self.store .set_owner(&self.ns, self.as_ref(), auth_id, force) } + + /// Lock a group exclusively + pub fn lock(&self) -> Result { + lock_dir_noblock( + &self.full_group_path(), + "backup group", + "possible running backup", + ) + } } impl AsRef for BackupGroup { @@ -439,15 +449,23 @@ impl BackupDir { .map_err(|err| format_err!("unable to acquire manifest lock {:?} - {}", &path, err)) } + /// Lock this snapshot exclusively + pub fn lock(&self) -> Result { + lock_dir_noblock(&self.full_path(), "snapshot", "possibly running or in use") + } + + /// Acquire a shared lock on this snapshot + pub fn lock_shared(&self) -> Result { + lock_dir_noblock_shared(&self.full_path(), "snapshot", "possibly running or in use") + } + /// Destroy the whole snapshot, bails if it's protected /// /// Setting `force` to true skips locking and thus ignores if the backup is currently in use. pub fn destroy(&self, force: bool) -> Result<(), Error> { - let full_path = self.full_path(); - let (_guard, _manifest_guard); if !force { - _guard = lock_dir_noblock(&full_path, "snapshot", "possibly running or in use")?; + _guard = self.lock()?; _manifest_guard = self.lock_manifest()?; } @@ -455,6 +473,7 @@ impl BackupDir { bail!("cannot remove protected snapshot"); // use special error type? } + let full_path = self.full_path(); log::info!("removing backup snapshot {:?}", full_path); std::fs::remove_dir_all(&full_path).map_err(|err| { format_err!("removing backup snapshot {:?} failed - {}", full_path, err,) diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs index a539ddc5..52a5f079 100644 --- a/pbs-datastore/src/datastore.rs +++ b/pbs-datastore/src/datastore.rs @@ -11,8 +11,8 @@ use nix::unistd::{unlinkat, UnlinkatFlags}; use proxmox_schema::ApiType; +use proxmox_sys::fs::DirLockGuard; use proxmox_sys::fs::{file_read_optional_string, replace_file, CreateOptions}; -use proxmox_sys::fs::{lock_dir_noblock, DirLockGuard}; use proxmox_sys::process_locker::ProcessLockSharedGuard; use proxmox_sys::WorkerTaskContext; use proxmox_sys::{task_log, task_warn}; @@ -630,41 +630,31 @@ impl DataStore { /// /// This also acquires an exclusive lock on the directory and returns the lock guard. pub fn create_locked_backup_group( - &self, + self: &Arc, ns: &BackupNamespace, backup_group: &pbs_api_types::BackupGroup, auth_id: &Authid, ) -> Result<(Authid, DirLockGuard), Error> { - // create intermediate path first: - let mut full_path = self.base_path(); - for ns in ns.components() { - full_path.push("ns"); - full_path.push(ns); - } - full_path.push(backup_group.ty.as_str()); - std::fs::create_dir_all(&full_path)?; + let backup_group = self.backup_group(ns.clone(), backup_group.clone()); - full_path.push(&backup_group.id); + // create intermediate path first + let full_path = backup_group.full_group_path(); - // create the last component now + std::fs::create_dir_all(&full_path.parent().ok_or(format_err!( + "could not construct parent path for group {backup_group:?}" + ))?)?; + + // now create the group, this allows us to check whether it existed before match std::fs::create_dir(&full_path) { Ok(_) => { - let guard = lock_dir_noblock( - &full_path, - "backup group", - "another backup is already running", - )?; - self.set_owner(ns, backup_group, auth_id, false)?; - let owner = self.get_owner(ns, backup_group)?; // just to be sure + let guard = backup_group.lock()?; + self.set_owner(ns, backup_group.group(), auth_id, false)?; + let owner = self.get_owner(ns, backup_group.group())?; // just to be sure Ok((owner, guard)) } Err(ref err) if err.kind() == io::ErrorKind::AlreadyExists => { - let guard = lock_dir_noblock( - &full_path, - "backup group", - "another backup is already running", - )?; - let owner = self.get_owner(ns, backup_group)?; // just to be sure + let guard = backup_group.lock()?; + let owner = self.get_owner(ns, backup_group.group())?; // just to be sure Ok((owner, guard)) } Err(err) => bail!("unable to create backup group {:?} - {}", full_path, err), @@ -675,29 +665,17 @@ impl DataStore { /// /// The BackupGroup directory needs to exist. pub fn create_locked_backup_dir( - &self, + self: &Arc, ns: &BackupNamespace, backup_dir: &pbs_api_types::BackupDir, ) -> Result<(PathBuf, bool, DirLockGuard), Error> { - let full_path = self.snapshot_path(ns, backup_dir); - let relative_path = full_path.strip_prefix(self.base_path()).map_err(|err| { - format_err!( - "failed to produce correct path for backup {backup_dir} in namespace {ns}: {err}" - ) - })?; - - let lock = || { - lock_dir_noblock( - &full_path, - "snapshot", - "internal error - tried creating snapshot that's already in use", - ) - }; + let backup_dir = self.backup_dir(ns.clone(), backup_dir.clone())?; + let relative_path = backup_dir.relative_path(); - match std::fs::create_dir(&full_path) { - Ok(_) => Ok((relative_path.to_owned(), true, lock()?)), + match std::fs::create_dir(&backup_dir.full_path()) { + Ok(_) => Ok((relative_path, true, backup_dir.lock()?)), Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists => { - Ok((relative_path.to_owned(), false, lock()?)) + Ok((relative_path, false, backup_dir.lock()?)) } Err(e) => Err(e.into()), } @@ -1155,9 +1133,7 @@ impl DataStore { /// Updates the protection status of the specified snapshot. pub fn update_protection(&self, backup_dir: &BackupDir, protection: bool) -> Result<(), Error> { - let full_path = backup_dir.full_path(); - - let _guard = lock_dir_noblock(&full_path, "snapshot", "possibly running or in use")?; + let _guard = backup_dir.lock()?; let protected_path = backup_dir.protected_file(); if protection { diff --git a/pbs-datastore/src/snapshot_reader.rs b/pbs-datastore/src/snapshot_reader.rs index 899c3bce..08b2b66e 100644 --- a/pbs-datastore/src/snapshot_reader.rs +++ b/pbs-datastore/src/snapshot_reader.rs @@ -6,8 +6,6 @@ use std::sync::Arc; use anyhow::{bail, Error}; use nix::dir::Dir; -use proxmox_sys::fs::lock_dir_noblock_shared; - use pbs_api_types::{print_store_and_ns, BackupNamespace, Operation}; use crate::backup_info::BackupDir; @@ -39,10 +37,8 @@ impl SnapshotReader { pub(crate) fn new_do(snapshot: BackupDir) -> Result { let datastore = snapshot.datastore(); - let snapshot_path = snapshot.full_path(); - let locked_dir = - lock_dir_noblock_shared(&snapshot_path, "snapshot", "locked by another operation")?; + let locked_dir = snapshot.lock_shared()?; let datastore_name = datastore.name().to_string(); let manifest = match snapshot.load_manifest() { @@ -57,7 +53,7 @@ impl SnapshotReader { } }; - let mut client_log_path = snapshot_path; + let mut client_log_path = snapshot.full_path(); client_log_path.push(CLIENT_LOG_BLOB_NAME); let mut file_list = vec![MANIFEST_BLOB_NAME.to_string()]; diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs index e519a200..077d70f9 100644 --- a/src/api2/backup/mod.rs +++ b/src/api2/backup/mod.rs @@ -27,7 +27,6 @@ use pbs_datastore::manifest::{archive_type, ArchiveType}; use pbs_datastore::{DataStore, PROXMOX_BACKUP_PROTOCOL_ID_V1}; use pbs_tools::json::{required_array_param, required_integer_param, required_string_param}; use proxmox_rest_server::{H2Service, WorkerTask}; -use proxmox_sys::fs::lock_dir_noblock_shared; mod environment; use environment::*; @@ -181,12 +180,7 @@ fn upgrade_to_backup_protocol( } // lock last snapshot to prevent forgetting/pruning it during backup - let full_path = last.backup_dir.full_path(); - Some(lock_dir_noblock_shared( - &full_path, - "snapshot", - "base snapshot is already locked by another operation", - )?) + Some(last.backup_dir.lock_shared()?) } else { None }; diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs index e2a10da3..ea7c4aa7 100644 --- a/src/api2/reader/mod.rs +++ b/src/api2/reader/mod.rs @@ -27,7 +27,6 @@ use pbs_datastore::manifest::{archive_type, ArchiveType}; use pbs_datastore::{DataStore, PROXMOX_BACKUP_READER_PROTOCOL_ID_V1}; use pbs_tools::json::required_string_param; use proxmox_rest_server::{H2Service, WorkerTask}; -use proxmox_sys::fs::lock_dir_noblock_shared; use crate::api2::backup::optional_ns_param; use crate::api2::helpers; @@ -125,11 +124,7 @@ fn upgrade_to_backup_reader_protocol( } } - let _guard = lock_dir_noblock_shared( - &backup_dir.full_path(), - "snapshot", - "locked by another operation", - )?; + let _guard = backup_dir.lock_shared()?; let path = datastore.base_path(); diff --git a/src/backup/verify.rs b/src/backup/verify.rs index 3984e28d..6c4acf3a 100644 --- a/src/backup/verify.rs +++ b/src/backup/verify.rs @@ -16,7 +16,6 @@ use pbs_datastore::backup_info::{BackupDir, BackupGroup, BackupInfo}; use pbs_datastore::index::IndexFile; use pbs_datastore::manifest::{archive_type, ArchiveType, BackupManifest, FileInfo}; use pbs_datastore::{DataBlob, DataStore, StoreProgress}; -use proxmox_sys::fs::lock_dir_noblock_shared; use crate::tools::parallel_handler::ParallelHandler; @@ -328,11 +327,7 @@ pub fn verify_backup_dir( upid: UPID, filter: Option<&dyn Fn(&BackupManifest) -> bool>, ) -> Result { - let snap_lock = lock_dir_noblock_shared( - &backup_dir.full_path(), - "snapshot", - "locked by another operation", - ); + let snap_lock = backup_dir.lock_shared(); match snap_lock { Ok(snap_lock) => { verify_backup_dir_with_lock(verify_worker, backup_dir, upid, filter, snap_lock) -- 2.30.2