[RFC PATCH proxmox-backup] fix #7670: datastore: s3: allow for per-chunk file lock cleanup

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

Per-chunk file locks are located on a tmpfs but never cleaned up to
avoid TOCTOU race conditions. Therefore lock files can accumulate
over time, the memory required to store the inodes finally lead to
OOM conditions if the system is not rebooted for a long time or a
high number of different chunks is written to the s3 backed
datastore.

To fix this, use the double stating strategy already implemented by
pbs_datastore::backup_info::lock_helper(), but adapt it so that the
lock file is cleaned up before unlocking. Since after file removal
the lock can be acquired by a different thread/process, the file lock
must also be dropped immediately without performing any other
critical operation. To assure this, a ChunkLockGuard is implemented
which removes the file and drops the file descriptor by implementing
the Drop trait.

After each flock() call, which is performed as part of
proxmox_sys::fs::open_file_locked(), stating the file and comparing
locked files inode from the open file handle to the one currently
present on the filesystem is performed. By this possible races are
detected, resulting in missing or newly create lock files. In that
case locking must be retried.

Note that this locking mechanism is not fair, the first caller might
end up being the last to actually acquire the lock. This is however
not problematic for the intended use case of per-chunk file locking,
with limited lock contention.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7670
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
Sending this as RFC in case there are ideas for a different solution
to the problem at hand which might be preferable.

Currently the only reason for this to fail as far as I can see is if
external tooling prematurely removes the lock file. The retry
mechanism could further be limited to a fixed number of retries to
protect against infinite loops. This should also be forwards
compatible with the previous implementation, the previous one might
hover fail if a still ongoing job tries to lock the per-chunk file,
looses however to the new implementation getting there first, which
will then remove the file and the subsequent stat of the old
implementation after getting the lock will fail.
Such a race is however rather unlikely to happen and would not lead
to an inconsistent state.

 pbs-datastore/src/chunk_store.rs | 69 +++++++++++++++++++++++++++++---
 pbs-datastore/src/datastore.rs   |  6 +--
 2 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index a936f5034..020d5ff2b 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -9,7 +9,6 @@ use hex::FromHex;
 use tracing::{info, warn};
 
 use pbs_api_types::{DatastoreFSyncLevel, GarbageCollectionStatus};
-use pbs_config::BackupLockGuard;
 use proxmox_io::ReadExt;
 use proxmox_s3_client::S3Client;
 use proxmox_sys::fs::{CreateOptions, create_dir, create_path, file_type_from_file_stat};
@@ -43,6 +42,32 @@ pub struct ChunkStore {
     locker: Option<Arc<Mutex<ProcessLocker>>>,
 }
 
+pub(crate) struct ChunkLockGuard {
+    file: std::fs::File,
+    path: PathBuf,
+}
+
+impl AsRawFd for ChunkLockGuard {
+    fn as_raw_fd(&self) -> i32 {
+        self.file.as_raw_fd()
+    }
+}
+
+impl Drop for ChunkLockGuard {
+    fn drop(&mut self) {
+        // After unlink lock can be acquired, local lock is invalid
+        if let Err(err) = nix::unistd::unlink(&self.path) {
+            tracing::error!(
+                "Failed to unlink chunk lock guard {:?}: {err}",
+                self.path,
+            );
+        }
+        // implicit drop of lock guard file descriptor which unblocks flock() waiters for it,
+        // they can however not use it and must retry from scratch (this is checked by stat after
+        // lock).
+    }
+}
+
 // TODO: what about sysctl setting vm.vfs_cache_pressure (0 - 100) ?
 
 pub fn verify_chunk_size(size: usize) -> Result<(), Error> {
@@ -963,12 +988,44 @@ impl ChunkStore {
         &self,
         digest: &[u8],
         timeout: Duration,
-    ) -> Result<BackupLockGuard, Error> {
+    ) -> Result<ChunkLockGuard, 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)
-        })?;
-        Ok(guard)
+
+        let mut lock_dir = Path::new(DATASTORE_LOCKS_DIR).join(self.name.clone());
+
+        if let Some(parent) = lock_path.parent() {
+            lock_dir = lock_dir.join(parent);
+        };
+
+        std::fs::create_dir_all(&lock_dir)?;
+
+        let user = pbs_config::backup_user()?;
+        let options = proxmox_sys::fs::CreateOptions::new()
+            .perm(nix::sys::stat::Mode::from_bits_truncate(0o660))
+            .owner(user.uid)
+            .group(user.gid);
+
+        loop {
+            let file = proxmox_sys::fs::open_file_locked(&lock_path, timeout, true, options)?;
+
+            // if stat fails here, file might have been tampered with from unrelated third party or
+            // due to a bug, always fail with error.
+            let inode = nix::sys::stat::fstat(file.as_raw_fd())?.st_ino;
+
+            match nix::sys::stat::stat(&lock_path) {
+                Ok(stat) => if inode == stat.st_ino {
+                    return Ok(ChunkLockGuard {
+                        file,
+                        path: lock_path.clone(),
+                    });
+                }
+                Err(err) => if err != nix::errno::Errno::ENOENT {
+                    bail!("failed to stat chunk lock file {lock_path:?}: {err}");
+                }
+            }
+            // neither matching inode after lock, nor unrelated error on stat
+            // spin retry
+        }
     }
 
     /// Generate the next bad chunk file path for given digest. Returns the path as well as the bad
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index e2d1ae67c..e930a3b95 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -43,7 +43,7 @@ use proxmox_section_config::SectionConfigData;
 use crate::backup_info::{
     BackupDir, BackupGroup, BackupInfo, OLD_LOCKING, PROTECTED_MARKER_FILENAME,
 };
-use crate::chunk_store::ChunkStore;
+use crate::chunk_store::{ChunkStore, ChunkLockGuard};
 use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
 use crate::fixed_index::{FixedIndexReader, FixedIndexWriter};
 use crate::hierarchy::{ListGroups, ListGroupsType, ListNamespaces, ListNamespacesRecursive};
@@ -3497,7 +3497,7 @@ impl DataStore {
 
 /// Track S3 object keys to be deleted by garbage collection while holding their file lock.
 struct S3DeleteList {
-    list: Vec<(S3ObjectKey, BackupLockGuard)>,
+    list: Vec<(S3ObjectKey, ChunkLockGuard)>,
     first_entry_added: SystemTime,
     age_threshold: Duration,
     capacity_threshold: usize,
@@ -3516,7 +3516,7 @@ impl S3DeleteList {
 
     /// Pushes the current key and backup lock guard to the list, updating the delete list age if
     /// the list was empty before the insert.
-    fn push(&mut self, key: S3ObjectKey, guard: BackupLockGuard) {
+    fn push(&mut self, key: S3ObjectKey, guard: ChunkLockGuard) {
         // set age based on first insertion
         if self.list.is_empty() {
             self.first_entry_added = SystemTime::now();
-- 
2.47.3