[PATCH proxmox-backup 4/4] fix #7642: s3: avoid repeated user lookup for per-chunk file locking

[Christian Ebner <c.ebner@proxmox.com>]

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 <c.ebner@proxmox.com>
---
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<P: AsRef<std::path::Path>>(
     exclusive: bool,
 ) -> Result<BackupLockGuard, Error> {
     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<P: AsRef<std::path::Path>>(
+    path: P,
+    options: CreateOptions,
+    timeout: Option<std::time::Duration>,
+    exclusive: bool,
+) -> Result<BackupLockGuard, Error> {
     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<CreateOptions>,
     ) -> 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<BackupLockGuard, Error> {
         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