[pbs-devel] [PATCH proxmox-backup v2 07/12] local store cache: rework access cache fetching and insert logic

[Christian Ebner]

The local datastore cache has both, an in-memory LRU cache only
storing the digests and the chunk marks on the filesystem.
Chunks in the LRU cache have recently been accessed, therefore the
chunk contents are expected to be present in the local chunk file,
while no payload is present for evicted ones.

The current implementation relied on the cacher to fetch the chunk
data on cache misses, but required to re-read the chunk file after
the download, as the cacher interface does not allow to return a
payload value other than the one defined for the LRU cache, which is
however none.

Therefore, instead of using the LRU cache access method and in turn
the S3Cacher, rather try to access the local filesystem chunks
directly. They need to be accessed anyways, and further this avoids
possible races with download and insert, as now the held filehandle
either has a chunk with valid content and can bypass the backend,
or the chunk must be downloaded, serving the chunk from the fetched
data instead after inserting into the cache.

By unconditional re-insertion, it is assured that the chunk will be
marked as recently used in all cases and the least recently used one
is evicted.

Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
---
 .../src/local_datastore_lru_cache.rs          | 50 ++++++++-----------
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs
index ea92bc9b3..f03265a5b 100644
--- a/pbs-datastore/src/local_datastore_lru_cache.rs
+++ b/pbs-datastore/src/local_datastore_lru_cache.rs
@@ -102,42 +102,36 @@ impl LocalDatastoreLruCache {
         digest: &[u8; 32],
         cacher: &mut S3Cacher,
     ) -> Result<Option<DataBlob>, Error> {
-        if self
-            .cache
-            .access(*digest, cacher, |digest| self.store.clear_chunk(&digest))
-            .await?
-            .is_some()
-        {
-            let (path, _digest_str) = self.store.chunk_path(digest);
-            let mut file = match std::fs::File::open(&path) {
-                Ok(file) => file,
-                Err(err) => {
-                    // Expected chunk to be present since LRU cache has it, but it is missing
-                    // locally, try to fetch again
-                    if err.kind() == std::io::ErrorKind::NotFound {
-                        let chunk = self.fetch_and_insert(cacher.client.clone(), digest).await?;
-                        return Ok(Some(chunk));
-                    } else {
-                        return Err(Error::from(err));
-                    }
+        let (path, _digest_str) = self.store.chunk_path(digest);
+        match std::fs::File::open(&path) {
+            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))?;
+                    Ok(Some(chunk))
                 }
-            };
-            let chunk = match DataBlob::load_from_reader(&mut file) {
-                Ok(chunk) => chunk,
+                // File was empty, might have been evicted since
                 Err(err) => {
                     use std::io::Seek;
                     // Check if file is empty marker file, try fetching content if so
                     if file.seek(std::io::SeekFrom::End(0))? == 0 {
                         let chunk = self.fetch_and_insert(cacher.client.clone(), digest).await?;
-                        return Ok(Some(chunk));
+                        Ok(Some(chunk))
                     } else {
-                        return Err(err);
+                        Err(err)
                     }
                 }
-            };
-            Ok(Some(chunk))
-        } else {
-            Ok(None)
+            },
+            Err(err) => {
+                // Failed to open file, missing
+                if err.kind() == std::io::ErrorKind::NotFound {
+                    let chunk = self.fetch_and_insert(cacher.client.clone(), digest).await?;
+                    Ok(Some(chunk))
+                } else {
+                    Err(Error::from(err))
+                }
+            }
         }
     }
 
@@ -159,7 +153,7 @@ impl LocalDatastoreLruCache {
             Some(response) => {
                 let bytes = response.content.collect().await?.to_bytes();
                 let chunk = DataBlob::from_raw(bytes.to_vec())?;
-                self.store.insert_chunk(&chunk, digest)?;
+                self.insert(digest, &chunk)?;
                 Ok(chunk)
             }
         }
-- 
2.47.3