From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 525551FF138 for ; Mon, 01 Jun 2026 14:32:27 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B40D01C4F4; Mon, 1 Jun 2026 14:32:26 +0200 (CEST) From: Christian Ebner To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox-backup 4/4] fix #7642: s3: avoid repeated user lookup for per-chunk file locking Date: Mon, 1 Jun 2026 14:31:24 +0200 Message-ID: <20260601123124.461765-5-c.ebner@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260601123124.461765-1-c.ebner@proxmox.com> References: <20260601123124.461765-1-c.ebner@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1780317075685 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.063 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 KAM_SHORT 0.001 Use of a URL Shortener for very short URL SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record T_SPF_PERMERROR 0.01 SPF: test of record failed (permerror) Message-ID-Hash: JKMGD3CKILRWQZ7SCY4NLZBLSJJ3HRKR X-Message-ID-Hash: JKMGD3CKILRWQZ7SCY4NLZBLSJJ3HRKR X-MailFrom: c.ebner@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: In contrast to regular datastores which use a mutex lock to avoid concurrent metadata updates/access on chunks, datastores backed by S3 use per-chunk file locks for this, so the mutex does not need to be held across async/await boundaries and per-chunk operations can also happen in parallel. pbs_config::open_backup_lockfile() helper is used for this, which does however perform a username to uid/gid lookup via pbs_config::backup_user() and therefore getpwnam_r(3) [0], which does read the /etc/nsswitch.conf and /etc/passwd as shown in the strace outputs provided in the bugtacker issue linked below. This causes avoidable syscalls especially for phase 2 of garbage collection and chunk insertion on datastores backed by S3. Fix by providing barebone helper implementations for the file locking without user lookup and create option instantiation, and wrap the pre-existing helper around it, so the callsites can be adapted accordingly. For GC the create options are now only instantiated at the start of GC, for backups they are created and stored with the backend in the backup environment. [0] https://docs.rs/nix/0.29.0/nix/unistd/struct.User.html#method.from_name Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7642 Signed-off-by: Christian Ebner --- Alternatively this could also be instantiated once inside a LazyCell and shared, since in practice this should never change. Other call sites would benefit from that as well. pbs-config/src/lib.rs | 30 ++++++++++--- pbs-datastore/src/chunk_store.rs | 14 +++++- pbs-datastore/src/datastore.rs | 74 +++++++++++++++++++------------- 3 files changed, 82 insertions(+), 36 deletions(-) diff --git a/pbs-config/src/lib.rs b/pbs-config/src/lib.rs index a194d70ac..07adb3391 100644 --- a/pbs-config/src/lib.rs +++ b/pbs-config/src/lib.rs @@ -4,7 +4,7 @@ use anyhow::{Error, bail, format_err}; use hex::FromHex; use nix::unistd::{Gid, Group, Uid, User}; -use proxmox_sys::fs::DirLockGuard; +use proxmox_sys::fs::{CreateOptions, DirLockGuard}; pub use pbs_buildcfg::{BACKUP_GROUP_NAME, BACKUP_USER_NAME}; @@ -94,6 +94,14 @@ pub unsafe fn create_mocked_lock() -> BackupLockGuard { } } +/// Create default file lock create options from user +pub fn create_options_for_user(user: &User) -> CreateOptions { + CreateOptions::new() + .perm(nix::sys::stat::Mode::from_bits_truncate(0o660)) + .owner(user.uid) + .group(user.gid) +} + /// Open or create a lock file owned by user "backup" and lock it. /// /// Owner/Group of the file is set to backup/backup. @@ -107,11 +115,23 @@ pub fn open_backup_lockfile>( exclusive: bool, ) -> Result { let user = backup_user()?; - let options = proxmox_sys::fs::CreateOptions::new() - .perm(nix::sys::stat::Mode::from_bits_truncate(0o660)) - .owner(user.uid) - .group(user.gid); + let options = create_options_for_user(&user); + open_backup_lockfile_with_create_options(path, options, timeout, exclusive) +} +/// Open or create a lock file using provided create options and lock it. +/// +/// File mode is 0660. +/// Default timeout is 10 seconds. +/// +/// Note: This method needs to be called by user "root" or owner of the file +/// matching create options. +pub fn open_backup_lockfile_with_create_options>( + path: P, + options: CreateOptions, + timeout: Option, + exclusive: bool, +) -> Result { let timeout = timeout.unwrap_or(std::time::Duration::new(10, 0)); let file = proxmox_sys::fs::open_file_locked(&path, timeout, exclusive, options)?; diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs index f8a2696ce..50690b7dd 100644 --- a/pbs-datastore/src/chunk_store.rs +++ b/pbs-datastore/src/chunk_store.rs @@ -446,6 +446,7 @@ impl ChunkStore { status: &mut GarbageCollectionStatus, worker: &dyn WorkerTaskContext, cache: Option<&LocalDatastoreLruCache>, + lock_create_options: Option, ) -> Result<(), Error> { // unwrap: only `None` in unit tests assert!(self.locker.is_some()); @@ -512,10 +513,18 @@ impl ChunkStore { if !bad { let digest = <[u8; 32]>::from_hex(filename.to_bytes())?; + let options = match lock_create_options { + Some(options) => options, + None => { + let user = pbs_config::backup_user()?; + pbs_config::create_options_for_user(&user) + } + }; + // unless there is a concurrent upload pending, // must never block due to required locking order if let Ok(_guard) = - self.lock_chunk(&digest, Duration::from_secs(0)) + self.lock_chunk(&digest, Duration::from_secs(0), options) { cache.remove(&digest)?; } @@ -964,10 +973,11 @@ impl ChunkStore { &self, digest: &[u8], timeout: Duration, + options: CreateOptions, ) -> Result { let lock_path = self.chunk_lock_path(digest); let guard = crate::backup_info::lock_helper(self.name(), &lock_path, |path| { - pbs_config::open_backup_lockfile(path, Some(timeout), true) + pbs_config::open_backup_lockfile_with_create_options(path, options, Some(timeout), true) })?; Ok(guard) } diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs index 988cda833..9dbf610a4 100644 --- a/pbs-datastore/src/datastore.rs +++ b/pbs-datastore/src/datastore.rs @@ -11,7 +11,7 @@ use anyhow::{Context, Error, bail, format_err}; use http_body_util::BodyExt; use hyper::Method; use hyper::body::Bytes; -use nix::unistd::{UnlinkatFlags, unlinkat}; +use nix::unistd::{UnlinkatFlags, User, unlinkat}; use pbs_tools::lru_cache::LruCache; use tokio::io::AsyncWriteExt; use tracing::{info, warn}; @@ -344,6 +344,7 @@ pub enum DatastoreBackend { pub struct S3Backend { // S3 client instance for current S3 backend pub client: S3Client, + backup_user: User, } impl DatastoreBackend { @@ -526,7 +527,10 @@ impl DataStore { })); } let s3_client = S3Client::new(options)?; - let s3_backend = S3Backend { client: s3_client }; + let s3_backend = S3Backend { + client: s3_client, + backup_user: user, + }; DatastoreBackend::S3(Arc::new(s3_backend)) } }; @@ -2154,13 +2158,14 @@ impl DataStore { // file requires the chunk anymore (won't get to this loop then) let is_bad = self.inner.chunk_store.cond_touch_bad_chunks(digest)?; - if let Some(_s3_backend) = &s3_backend { + if let Some(s3_backend) = &s3_backend { + let options = pbs_config::create_options_for_user(&s3_backend.backup_user); // Do not retry here, this is very unlikely to happen as chunk markers will // most likely only be missing if the local cache store was recreated. - let _guard = self - .inner - .chunk_store - .lock_chunk(digest, CHUNK_LOCK_TIMEOUT)?; + let _guard = + self.inner + .chunk_store + .lock_chunk(digest, CHUNK_LOCK_TIMEOUT, options)?; if !self.inner.chunk_store.cond_touch_chunk(digest, false)? && !is_bad { // Insert empty file as marker to tell GC phase2 that this is // a chunk still in-use, so to keep in the S3 object store. @@ -2481,6 +2486,7 @@ impl DataStore { if let Some(s3_backend) = &s3_backend { let mut chunk_count = 0; + let options = pbs_config::create_options_for_user(&s3_backend.backup_user); let prefix = S3PathPrefix::Some(".chunks/".to_string()); // Operates in batches of 1000 objects max per request let mut list_bucket_result = @@ -2500,10 +2506,11 @@ impl DataStore { }; let timeout = std::time::Duration::from_secs(0); - let _chunk_guard = match self.inner.chunk_store.lock_chunk(&digest, timeout) { - Ok(guard) => guard, - Err(_) => continue, - }; + let _chunk_guard = + match self.inner.chunk_store.lock_chunk(&digest, timeout, options) { + Ok(guard) => guard, + Err(_) => continue, + }; let _guard = self.inner.chunk_store.mutex().lock().unwrap(); // Check local markers (created or atime updated during phase1) and @@ -2595,6 +2602,7 @@ impl DataStore { &mut tmp_gc_status, worker, self.cache(), + Some(options), )?; } else { self.inner.chunk_store.sweep_unused_chunks( @@ -2603,6 +2611,7 @@ impl DataStore { &mut gc_status, worker, None, + None, )?; } @@ -2776,10 +2785,11 @@ impl DataStore { match backend { DatastoreBackend::Filesystem => self.inner.chunk_store.insert_chunk(chunk, digest), DatastoreBackend::S3(s3_backend) => { - let _chunk_guard = self - .inner - .chunk_store - .lock_chunk(digest, CHUNK_LOCK_TIMEOUT)?; + let options = pbs_config::create_options_for_user(&s3_backend.backup_user); + let _chunk_guard = + self.inner + .chunk_store + .lock_chunk(digest, CHUNK_LOCK_TIMEOUT, options)?; let chunk_data: Bytes = chunk.raw_data().to_vec().into(); let chunk_size = chunk_data.len() as u64; let object_key = crate::s3::object_key_from_digest(digest)?; @@ -2802,10 +2812,12 @@ impl DataStore { ) -> Result<(bool, u64), Error> { let chunk_data = chunk.raw_data(); let chunk_size = chunk_data.len() as u64; - let _chunk_guard = self - .inner - .chunk_store - .lock_chunk(digest, CHUNK_LOCK_TIMEOUT)?; + let options = pbs_config::create_options_for_user(&s3_backend.backup_user); + + let _chunk_guard = + self.inner + .chunk_store + .lock_chunk(digest, CHUNK_LOCK_TIMEOUT, options)?; // Avoid re-upload to S3 if the chunk is either present in the in-memory LRU cache // or the chunk marker file exists on filesystem. The latter means the chunk has @@ -3208,17 +3220,16 @@ impl DataStore { tmp_base: &Path, s3_backend: &S3Backend, ) -> Result<(), Error> { - let backup_user = pbs_config::backup_user().context("failed to get backup user")?; let mode = nix::sys::stat::Mode::from_bits_truncate(0o0644); let file_create_options = CreateOptions::new() .perm(mode) - .owner(backup_user.uid) - .group(backup_user.gid); + .owner(s3_backend.backup_user.uid) + .group(s3_backend.backup_user.gid); let mode = nix::sys::stat::Mode::from_bits_truncate(0o0755); let dir_create_options = CreateOptions::new() .perm(mode) - .owner(backup_user.uid) - .group(backup_user.gid); + .owner(s3_backend.backup_user.uid) + .group(s3_backend.backup_user.gid); let list_prefix = S3PathPrefix::Some(S3_CONTENT_PREFIX.to_string()); let store_prefix = format!("{}/{S3_CONTENT_PREFIX}/", self.name()); @@ -3388,7 +3399,10 @@ impl DataStore { let s3_client = S3Client::new(options) .context("failed to create s3 client") .map_err(|err| format_err!("{err:#}"))?; - let s3_backend = S3Backend { client: s3_client }; + let s3_backend = S3Backend { + client: s3_client, + backup_user: user, + }; Ok((backend_type, Some(s3_backend))) } @@ -3474,10 +3488,12 @@ impl DataStore { let _chunk_guard; if let DatastoreBackendType::S3 = self.inner.backend_config.ty.unwrap_or_default() { - _chunk_guard = self - .inner - .chunk_store - .lock_chunk(digest, CHUNK_LOCK_TIMEOUT)?; + let user = pbs_config::backup_user()?; + let options = pbs_config::create_options_for_user(&user); + _chunk_guard = + self.inner + .chunk_store + .lock_chunk(digest, CHUNK_LOCK_TIMEOUT, options)?; } let _lock = self.inner.chunk_store.mutex().lock().unwrap(); -- 2.47.3