From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 64D701FF141 for ; Tue, 05 May 2026 14:15:11 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3F86226F7B; Tue, 5 May 2026 14:15:11 +0200 (CEST) Date: Tue, 05 May 2026 14:15:00 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= Subject: Re: [PATCH proxmox-backup 3/4] datastore: move file lock helper to centralized place To: Christian Ebner , pbs-devel@lists.proxmox.com References: <20260505081137.227901-1-c.ebner@proxmox.com> <20260505081137.227901-4-c.ebner@proxmox.com> In-Reply-To: <20260505081137.227901-4-c.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1777979120.4z0cb9ma00.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1777983198045 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.054 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: UT6ES6YNLCOUG52Q25WJNZXAWLG7PWT5 X-Message-ID-Hash: UT6ES6YNLCOUG52Q25WJNZXAWLG7PWT5 X-MailFrom: f.gruenbichler@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On May 5, 2026 10:11 am, Christian Ebner wrote: > Moves the implementation to a more central location, as it is not > only used by the datastore contents, but also by the chunk store. we could also consider moving it to its own place? we have: - lock_chunk in the chunk_store (has no dependency on the chunk/datastore other than its name) - lock_file_path_helper in backup_info - lock_helper - the DIR constant - lock_path implementations for -- chunk (digest) -- group -- snapshot -- manifests I've also wondered in the past a few times whether we'd not want to make the BackupLockGuard generic to encode *what* kind of entity is being locked, to improve function signatures.. though that might be a bigger change ;) >=20 > No functional changes. >=20 > Signed-off-by: Christian Ebner > --- > pbs-datastore/src/backup_info.rs | 35 ++------------------------------ > pbs-datastore/src/chunk_store.rs | 2 +- > pbs-datastore/src/lib.rs | 34 +++++++++++++++++++++++++++++++ > 3 files changed, 37 insertions(+), 34 deletions(-) >=20 > diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_= info.rs > index 9919b908a..bae136d3c 100644 > --- a/pbs-datastore/src/backup_info.rs > +++ b/pbs-datastore/src/backup_info.rs > @@ -1,5 +1,5 @@ > use std::fmt; > -use std::os::unix::io::{AsRawFd, RawFd}; > +use std::os::unix::io::RawFd; > use std::os::unix::prelude::OsStrExt; > use std::path::Path; > use std::path::PathBuf; > @@ -24,7 +24,7 @@ use crate::datastore::{GROUP_NOTES_FILE_NAME, GROUP_OWN= ER_FILE_NAME}; > use crate::manifest::{BackupManifest, MANIFEST_LOCK_NAME}; > use crate::move_journal; > use crate::s3::S3_CONTENT_PREFIX; > -use crate::{DataBlob, DataStore, DatastoreBackend, DATASTORE_LOCKS_DIR}; > +use crate::{lock_helper, DataBlob, DataStore, DatastoreBackend, DATASTOR= E_LOCKS_DIR}; > =20 > pub const PROTECTED_MARKER_FILENAME: &str =3D ".protected"; > =20 > @@ -1210,34 +1210,3 @@ fn lock_file_path_helper(ns: &BackupNamespace, pat= h: PathBuf) -> PathBuf { > =20 > to_return.join(format!("{first_eigthy}...{last_eighty}-{hash}")) > } > - > -/// Helps implement the double stat'ing procedure. It avoids certain rac= e conditions upon lock > -/// deletion. > -/// > -/// It also creates the base directory for lock files. > -pub(crate) fn lock_helper( > - store_name: &str, > - path: &std::path::Path, > - lock_fn: F, > -) -> Result > -where > - F: Fn(&std::path::Path) -> Result, > -{ > - let mut lock_dir =3D Path::new(DATASTORE_LOCKS_DIR).join(store_name)= ; > - > - if let Some(parent) =3D path.parent() { > - lock_dir =3D lock_dir.join(parent); > - }; > - > - std::fs::create_dir_all(&lock_dir)?; > - > - let lock =3D lock_fn(path)?; > - > - let inode =3D nix::sys::stat::fstat(lock.as_raw_fd())?.st_ino; > - > - if nix::sys::stat::stat(path).map_or(true, |st| inode !=3D st.st_ino= ) { > - bail!("could not acquire lock, another thread modified the lock = file"); > - } > - > - Ok(lock) > -} > diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_s= tore.rs > index 5b07ebdaa..778897974 100644 > --- a/pbs-datastore/src/chunk_store.rs > +++ b/pbs-datastore/src/chunk_store.rs > @@ -944,7 +944,7 @@ impl ChunkStore { > timeout: Duration, > ) -> Result { > let lock_path =3D self.chunk_lock_path(digest); > - let guard =3D crate::backup_info::lock_helper(self.name(), &lock= _path, |path| { > + let guard =3D crate::lock_helper(self.name(), &lock_path, |path|= { > pbs_config::open_backup_lockfile(path, Some(timeout), true) > })?; > Ok(guard) > diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs > index 29d203f87..48acba4c8 100644 > --- a/pbs-datastore/src/lib.rs > +++ b/pbs-datastore/src/lib.rs > @@ -157,6 +157,13 @@ > =20 > #![deny(unsafe_op_in_unsafe_fn)] > =20 > +use std::os::unix::io::AsRawFd; > +use std::path::Path; > + > +use anyhow::{bail, Error}; > + > +use pbs_config::BackupLockGuard; > + > /// Directory path where active operations counters are saved. > pub const ACTIVE_OPERATIONS_DIR: &str =3D concat!( > pbs_buildcfg::PROXMOX_BACKUP_RUN_DIR_M!(), > @@ -237,3 +244,30 @@ pub use local_chunk_reader::LocalChunkReader; > =20 > mod local_datastore_lru_cache; > pub use local_datastore_lru_cache::LocalDatastoreLruCache; > + > +/// Helps implement the double stat'ing procedure. It avoids certain rac= e conditions upon lock > +/// deletion. > +/// > +/// It also creates the base directory for lock files. > +pub fn lock_helper(store_name: &str, path: &Path, lock_fn: F) -> Resu= lt in any case, this does not need to be pub, `pub(crate)` is enough.. > +where > + F: Fn(&Path) -> Result, > +{ > + let mut lock_dir =3D Path::new(DATASTORE_LOCKS_DIR).join(store_name)= ; > + > + if let Some(parent) =3D path.parent() { > + lock_dir =3D lock_dir.join(parent); > + }; > + > + std::fs::create_dir_all(&lock_dir)?; > + > + let lock =3D lock_fn(path)?; > + > + let inode =3D nix::sys::stat::fstat(lock.as_raw_fd())?.st_ino; > + > + if nix::sys::stat::stat(path).map_or(true, |st| inode !=3D st.st_ino= ) { > + bail!("could not acquire lock, another thread modified the lock = file"); > + } > + > + Ok(lock) > +} > --=20 > 2.47.3 >=20 >=20 >=20 >=20 >=20 >=20