[pbs-devel] [PATCH proxmox-backup v5 11/19] store: cache: move Mutex acquire to cache insertion

[Christian Ebner]

From: Fabian Grünbichler <f.gruenbichler at proxmox.com>

to avoid the lock ordering issue between the cache implemention's internal
Mutex, and the chunk store Mutex, refactor the interface so that any cache
actions that modify the chunk store can acquire the chunk store Mutex first,
before locking the cache.

Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
---
 pbs-datastore/src/chunk_store.rs              |  6 +++---
 .../src/local_datastore_lru_cache.rs          | 20 ++++++++++++++-----
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 085816f42..a17c258a7 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -697,7 +697,9 @@ impl ChunkStore {
     ///
     /// Used to evict chunks from the local datastore cache, while keeping them as in-use markers
     /// for garbage collection. Returns with success also if chunk file is not pre-existing.
-    pub(crate) fn clear_chunk(&self, digest: &[u8; 32]) -> Result<(), Error> {
+    ///
+    /// Safety: chunk store mutex must be held!
+    pub(crate) unsafe fn clear_chunk(&self, digest: &[u8; 32]) -> Result<(), Error> {
         let (chunk_path, digest_str) = self.chunk_path(digest);
         let mut create_options = CreateOptions::new();
         if nix::unistd::Uid::effective().is_root() {
@@ -706,8 +708,6 @@ impl ChunkStore {
             create_options = create_options.owner(uid).group(gid);
         }
 
-        let _lock = self.mutex.lock();
-
         proxmox_sys::fs::replace_file(&chunk_path, &[], create_options, false)
             .map_err(|err| format_err!("clear chunk failed for {digest_str} - {err}"))?;
         Ok(())
diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs
index 7b9d8caae..8b2dbedfd 100644
--- a/pbs-datastore/src/local_datastore_lru_cache.rs
+++ b/pbs-datastore/src/local_datastore_lru_cache.rs
@@ -34,9 +34,16 @@ impl LocalDatastoreLruCache {
     ///
     /// Fails if the chunk cannot be inserted successfully.
     pub fn insert(&self, digest: &[u8; 32], chunk: &DataBlob) -> Result<(), Error> {
-        self.store.insert_chunk(chunk, digest)?;
-        self.cache
-            .insert(*digest, (), |digest| self.store.clear_chunk(&digest))
+        let _lock = self.store.mutex().lock().unwrap();
+
+        // Safety: lock acquire above
+        unsafe {
+            self.store.insert_chunk_nolock(chunk, digest)?;
+        }
+        self.cache.insert(*digest, (), |digest| {
+            // Safety: lock acquired above, this is executed inline!
+            unsafe { self.store.clear_chunk(&digest) }
+        })
     }
 
     /// Remove a chunk from the local datastore cache.
@@ -70,8 +77,11 @@ impl LocalDatastoreLruCache {
             Ok(mut file) => match DataBlob::load_from_reader(&mut file) {
                 // File was still cached with contents, load response from file
                 Ok(chunk) => {
-                    self.cache
-                        .insert(*digest, (), |digest| self.store.clear_chunk(&digest))?;
+                    let _lock = self.store.mutex().lock().unwrap();
+                    self.cache.insert(*digest, (), |digest| {
+                        // Safety: lock acquired above, this is executed inline
+                        unsafe { self.store.clear_chunk(&digest) }
+                    })?;
                     Ok(Some(chunk))
                 }
                 // File was empty, might have been evicted since
-- 
2.47.3