[pbs-devel] [PATCH proxmox-backup v3 4/6] datastore: move bad chunk touching logic to chunk store and lock it

[Christian Ebner]

Provide an dedicated method and encapsulate the bad chunk filename
generation and access time updates. This is an implementation detail
of the chunks store, the garbage collection should not be concerned
about this.

To assure exclusive access to the chunk store, acquire the chunk store
mutex guard before performing access time updates. While there is
currently no code path which would lead to races as the bad chunk
renaming (which generates the bad chunks) is concerned about file
existence only and the garbage collection cannot be executed
concurrently, locking makes access time updates future prove.

In preparation for making the bad filename generation a dedicated
helper as well, all contained within the chunk store module.

Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
---
changes since version 2:
- lock chunk store before performing atime updates

 pbs-datastore/src/chunk_store.rs | 21 ++++++++++++++++++++-
 pbs-datastore/src/datastore.rs   | 11 +----------
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 5148d6c46..8bc74faf7 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -242,7 +242,7 @@ impl ChunkStore {
         self.cond_touch_path(&chunk_path, assert_exists)
     }
 
-    pub(super) fn cond_touch_path(&self, path: &Path, assert_exists: bool) -> Result<bool, Error> {
+    fn cond_touch_path(&self, path: &Path, assert_exists: bool) -> Result<bool, Error> {
         // unwrap: only `None` in unit tests
         assert!(self.locker.is_some());
 
@@ -276,6 +276,25 @@ impl ChunkStore {
         Ok(true)
     }
 
+    /// Update access timestamp on all bad chunks for given digest
+    ///
+    /// Gets exclusive access by acquiring the chunk store mutex guard.
+    pub(super) fn cond_touch_bad_chunks(&self, digest: &[u8; 32]) -> Result<bool, Error> {
+        let _lock = self.mutex.lock();
+
+        let (chunk_path, _digest_str) = self.chunk_path(digest);
+        let mut is_bad = false;
+        for i in 0..=9 {
+            let bad_ext = format!("{i}.bad");
+            let mut bad_path = chunk_path.clone();
+            bad_path.set_extension(bad_ext);
+            if self.cond_touch_path(&bad_path, false)? {
+                is_bad = true;
+            }
+        }
+        Ok(is_bad)
+    }
+
     fn get_chunk_store_iterator(
         &self,
     ) -> Result<
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index c32f232d9..a84c6fe54 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1355,19 +1355,10 @@ impl DataStore {
             }
 
             if !self.inner.chunk_store.cond_touch_chunk(digest, false)? {
-                let (chunk_path, _digest_str) = self.chunk_path(digest);
                 // touch any corresponding .bad files to keep them around, meaning if a chunk is
                 // rewritten correctly they will be removed automatically, as well as if no index
                 // file requires the chunk anymore (won't get to this loop then)
-                let mut is_bad = false;
-                for i in 0..=9 {
-                    let bad_ext = format!("{i}.bad");
-                    let mut bad_path = chunk_path.clone();
-                    bad_path.set_extension(bad_ext);
-                    if self.inner.chunk_store.cond_touch_path(&bad_path, false)? {
-                        is_bad = true;
-                    }
-                }
+                let is_bad = self.inner.chunk_store.cond_touch_bad_chunks(digest)?;
 
                 if let Some(ref _s3_client) = s3_client {
                     // Do not retry here, this is very unlikely to happen as chunk markers will
-- 
2.47.3