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 8110D72B20 for ; Tue, 24 May 2022 10:28:55 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 778011C35A for ; Tue, 24 May 2022 10:28:25 +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 id 636641C001 for ; Tue, 24 May 2022 10:28:22 +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 3B9D44381B for ; Tue, 24 May 2022 10:28:22 +0200 (CEST) From: Stefan Sterz To: pbs-devel@lists.proxmox.com Date: Tue, 24 May 2022 10:28:14 +0200 Message-Id: <20220524082816.97270-4-s.sterz@proxmox.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20220524082816.97270-1-s.sterz@proxmox.com> References: <20220524082816.97270-1-s.sterz@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.042 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 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [datastore.rs, verify.rs, environment.rs, lib.rs] Subject: [pbs-devel] [PATCH proxmox-backup v3 3/5] fix #3935: datastore/api/backup: move datastore locking to '/run' 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: Tue, 24 May 2022 08:28:55 -0000 to avoid issues when removing a group or snapshot directory where two threads hold a lock to the same directory, move locking to the tmpfs backed '/run' directory and use a flat file structure. also adds double stat'ing to make it possible to remove locks without certain race condition issues. Signed-off-by: Stefan Sterz --- pbs-config/src/lib.rs | 7 ++ pbs-datastore/src/backup_info.rs | 121 +++++++++++++++++++++++---- pbs-datastore/src/datastore.rs | 7 +- pbs-datastore/src/snapshot_reader.rs | 17 +++- src/api2/backup/environment.rs | 5 +- src/backup/verify.rs | 4 +- 6 files changed, 133 insertions(+), 28 deletions(-) diff --git a/pbs-config/src/lib.rs b/pbs-config/src/lib.rs index f9b63922..81f106c4 100644 --- a/pbs-config/src/lib.rs +++ b/pbs-config/src/lib.rs @@ -21,6 +21,7 @@ pub use config_version_cache::ConfigVersionCache; use anyhow::{format_err, Error}; use nix::unistd::{Gid, Group, Uid, User}; +use std::os::unix::prelude::AsRawFd; pub use pbs_buildcfg::{BACKUP_GROUP_NAME, BACKUP_USER_NAME}; @@ -46,6 +47,12 @@ pub fn backup_group() -> Result { pub struct BackupLockGuard(Option); +impl AsRawFd for BackupLockGuard { + fn as_raw_fd(&self) -> i32 { + self.0.as_ref().map_or(-1, |f| f.as_raw_fd()) + } +} + #[doc(hidden)] /// Note: do not use for production code, this is only intended for tests pub unsafe fn create_mocked_lock() -> BackupLockGuard { diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs index 0711fcfa..9bbe68ac 100644 --- a/pbs-datastore/src/backup_info.rs +++ b/pbs-datastore/src/backup_info.rs @@ -1,14 +1,15 @@ use std::convert::TryFrom; use std::fmt; -use std::os::unix::io::RawFd; +use std::os::unix::io::{AsRawFd, RawFd}; +use std::path::Path; use std::path::PathBuf; use std::sync::Arc; +use std::time::Duration; use anyhow::{bail, format_err, Error}; -use proxmox_sys::fs::{ - lock_dir_noblock, lock_dir_noblock_shared, replace_file, CreateOptions, DirLockGuard, -}; +use proxmox_sys::fs::{replace_file, CreateOptions}; +use proxmox_sys::systemd::escape_unit; use pbs_api_types::{ Authid, BackupNamespace, BackupType, GroupFilter, BACKUP_DATE_REGEX, BACKUP_FILE_REGEX, @@ -20,6 +21,8 @@ use crate::manifest::{ }; use crate::{DataBlob, DataStore}; +pub const DATASTORE_LOCKS_DIR: &str = "/run/proxmox-backup/locks"; + /// BackupGroup is a directory containing a list of BackupDir #[derive(Clone)] pub struct BackupGroup { @@ -216,6 +219,8 @@ impl BackupGroup { })?; } + let _ = std::fs::remove_file(self.lock_path()); + Ok(removed_all_snaps) } @@ -232,13 +237,29 @@ impl BackupGroup { .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", - ) + /// Returns a file name for locking a group. + /// + /// The lock file will be located in: + /// `${DATASTORE_LOCKS_DIR}/${datastore name}/${lock_file_name_helper(rpath)}` + /// where `rpath` is the relative path of the group. + /// + /// If the group's relative path contains non-Unicode sequences they will be replaced via + /// [std::ffi::OsStr::to_string_lossy()]. + fn lock_path(&self) -> PathBuf { + let path = Path::new(DATASTORE_LOCKS_DIR).join(self.store.name()); + + let rpath = self.relative_group_path(); + let rpath = rpath.as_os_str().to_string_lossy(); + + path.join(lock_file_name_helper(&rpath)) + } + + /// Locks a group exclusively. + pub fn lock(&self) -> Result { + lock_helper(self.store.name(), &self.lock_path(), |p| { + open_backup_lockfile(p, Some(Duration::from_secs(0)), true) + .map_err(|err| format_err!("unable to acquire backup group lock {p:?} - {err}")) + }) } } @@ -443,14 +464,37 @@ 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") + /// Returns a file name for locking a snapshot. + /// + /// The lock file will be located in: + /// `${DATASTORE_LOCKS_DIR}/${datastore name}/${lock_file_name_helper(rpath)}` + /// where `rpath` is the relative path of the snapshot. + /// + /// If the snapshot's relative path contains non-Unicode sequences they will be replaced via + /// [std::ffi::OsStr::to_string_lossy()]. + fn lock_path(&self) -> PathBuf { + let path = Path::new(DATASTORE_LOCKS_DIR).join(self.store.name()); + + let rpath = self.relative_path(); + let rpath = rpath.as_os_str().to_string_lossy(); + + path.join(lock_file_name_helper(&rpath)) + } + + /// Locks a snapshot exclusively. + pub fn lock(&self) -> Result { + lock_helper(self.store.name(), &self.lock_path(), |p| { + open_backup_lockfile(p, Some(Duration::from_secs(0)), true) + .map_err(|err| format_err!("unable to acquire snapshot lock {p:?} - {err}")) + }) } - /// 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") + /// Acquires a shared lock on a snapshot. + pub fn lock_shared(&self) -> Result { + lock_helper(self.store.name(), &self.lock_path(), |p| { + open_backup_lockfile(p, Some(Duration::from_secs(0)), false) + .map_err(|err| format_err!("unable to acquire shared snapshot lock {p:?} - {err}")) + }) } /// Destroy the whole snapshot, bails if it's protected @@ -473,11 +517,13 @@ impl BackupDir { format_err!("removing backup snapshot {:?} failed - {}", full_path, err,) })?; - // the manifest doesn't exist anymore, no need to keep the lock (already done by guard?) + // remove no longer needed lock files if let Ok(path) = self.manifest_lock_path() { let _ = std::fs::remove_file(path); // ignore errors } + let _ = std::fs::remove_file(self.lock_path()); // ignore errors + Ok(()) } @@ -664,3 +710,42 @@ fn list_backup_files( Ok(files) } + +/// Encodes a string so it can be used as a lock file name. +/// +/// The first 64 characters will be the sha256 hash of `path` then a hyphen followed by up to 100 +/// characters of the unit encoded version of `path`. +fn lock_file_name_helper(path: &str) -> String { + let enc = escape_unit(path, true); + let from = enc.len().checked_sub(100).unwrap_or(0); + let enc = enc[from..].to_string(); + let hash = hex::encode(openssl::sha::sha256(path.as_bytes())); + + format!("{hash}-{enc}") +} + +/// Helps implement the double stat'ing procedure. It avoids certain race conditions upon lock +/// deletion. +/// +/// It also creates the base directory for lock files. +fn lock_helper( + store_name: &str, + path: &std::path::Path, + lock_fn: F, +) -> Result +where + F: Fn(&std::path::Path) -> Result, +{ + let lock_dir = Path::new(DATASTORE_LOCKS_DIR).join(store_name); + std::fs::create_dir_all(&lock_dir)?; + + let lock = lock_fn(path)?; + + let stat = nix::sys::stat::fstat(lock.as_raw_fd())?.st_ino; + + if nix::sys::stat::stat(path).map_or(true, |st| stat != st.st_ino) { + bail!("could not acquire lock, another thread modified the lock file"); + } + + Ok(lock) +} diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs index f74ea04b..1fc7efa7 100644 --- a/pbs-datastore/src/datastore.rs +++ b/pbs-datastore/src/datastore.rs @@ -11,7 +11,6 @@ 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::process_locker::ProcessLockSharedGuard; use proxmox_sys::WorkerTaskContext; @@ -21,7 +20,7 @@ use pbs_api_types::{ Authid, BackupNamespace, BackupType, ChunkOrder, DataStoreConfig, DatastoreTuning, GarbageCollectionStatus, HumanByte, Operation, UPID, }; -use pbs_config::ConfigVersionCache; +use pbs_config::{BackupLockGuard, ConfigVersionCache}; use crate::backup_info::{BackupDir, BackupGroup}; use crate::chunk_store::ChunkStore; @@ -615,7 +614,7 @@ impl DataStore { ns: &BackupNamespace, backup_group: &pbs_api_types::BackupGroup, auth_id: &Authid, - ) -> Result<(Authid, DirLockGuard), Error> { + ) -> Result<(Authid, BackupLockGuard), Error> { let backup_group = BackupGroup::new(Arc::new(self.clone()), ns.clone(), backup_group.clone()); @@ -650,7 +649,7 @@ impl DataStore { &self, ns: &BackupNamespace, backup_dir: &pbs_api_types::BackupDir, - ) -> Result<(PathBuf, bool, DirLockGuard), Error> { + ) -> Result<(PathBuf, bool, BackupLockGuard), Error> { let backup_dir = Arc::new(self.clone()).backup_dir(ns.clone(), backup_dir.clone())?; let relative_path = backup_dir.relative_path(); diff --git a/pbs-datastore/src/snapshot_reader.rs b/pbs-datastore/src/snapshot_reader.rs index 8c7eefe1..40083900 100644 --- a/pbs-datastore/src/snapshot_reader.rs +++ b/pbs-datastore/src/snapshot_reader.rs @@ -3,8 +3,11 @@ use std::os::unix::io::{AsRawFd, FromRawFd}; use std::path::Path; use std::sync::Arc; -use anyhow::{bail, Error}; +use anyhow::{bail, format_err, Error}; use nix::dir::Dir; +use nix::fcntl::OFlag; +use nix::sys::stat::Mode; +use pbs_config::BackupLockGuard; use pbs_api_types::{BackupNamespace, DatastoreWithNamespace, Operation}; @@ -23,6 +26,10 @@ pub struct SnapshotReader { datastore_name: String, file_list: Vec, locked_dir: Dir, + + // while this is never read, the lock needs to be kept until the + // reader is dropped to ensure valid locking semantics + _lock: BackupLockGuard, } impl SnapshotReader { @@ -42,7 +49,12 @@ impl SnapshotReader { ns: snapshot.backup_ns().clone(), }; - let locked_dir = snapshot.lock_shared()?; + let lock = snapshot.lock_shared()?; + + let path = snapshot.full_path(); + + let locked_dir = Dir::open(&path, OFlag::O_RDONLY, Mode::empty()) + .map_err(|err| format_err!("unable to open snapshot directory {path:?} - {err}"))?; let datastore_name = datastore.name().to_string(); let manifest = match snapshot.load_manifest() { @@ -74,6 +86,7 @@ impl SnapshotReader { datastore_name, file_list, locked_dir, + _lock: lock, }) } diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs index 8c1c42db..c4280b3f 100644 --- a/src/api2/backup/environment.rs +++ b/src/api2/backup/environment.rs @@ -1,5 +1,6 @@ use anyhow::{bail, format_err, Error}; -use nix::dir::Dir; + +use pbs_config::BackupLockGuard; use std::collections::HashMap; use std::sync::{Arc, Mutex}; @@ -632,7 +633,7 @@ impl BackupEnvironment { /// If verify-new is set on the datastore, this will run a new verify task /// for the backup. If not, this will return and also drop the passed lock /// immediately. - pub fn verify_after_complete(&self, snap_lock: Dir) -> Result<(), Error> { + pub fn verify_after_complete(&self, snap_lock: BackupLockGuard) -> Result<(), Error> { self.ensure_finished()?; if !self.datastore.verify_new() { diff --git a/src/backup/verify.rs b/src/backup/verify.rs index dfe1dc56..2a89ab87 100644 --- a/src/backup/verify.rs +++ b/src/backup/verify.rs @@ -1,4 +1,4 @@ -use nix::dir::Dir; +use pbs_config::BackupLockGuard; use std::collections::HashSet; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; @@ -351,7 +351,7 @@ pub fn verify_backup_dir_with_lock( backup_dir: &BackupDir, upid: UPID, filter: Option<&dyn Fn(&BackupManifest) -> bool>, - _snap_lock: Dir, + _snap_lock: BackupLockGuard, ) -> Result { let manifest = match backup_dir.load_manifest() { Ok((manifest, _)) => manifest, -- 2.30.2