[pbs-devel] [PATCH proxmox-backup 16/17] GC: lock chunk marker before cleanup in phase 3 on s3 backends

[Christian Ebner]

To make sure there is no race between atime check and deletion with
possible re-insertion.

By only acquiring the file lock if the chunk marker would be removed
and double stating, the file locking penalty is avoided for the other
cases.

Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 28 +++++++++++++++++++++++++++-
 pbs-datastore/src/datastore.rs   |  2 ++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 49687b2fa..08519fe2b 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -5,6 +5,7 @@ use std::sync::{Arc, Mutex};
 use std::time::Duration;
 
 use anyhow::{bail, format_err, Context, Error};
+use hex::FromHex;
 use tracing::{info, warn};
 
 use pbs_api_types::{DatastoreFSyncLevel, GarbageCollectionStatus};
@@ -22,7 +23,7 @@ use crate::data_blob::DataChunkBuilder;
 use crate::file_formats::{
     COMPRESSED_BLOB_MAGIC_1_0, ENCRYPTED_BLOB_MAGIC_1_0, UNCOMPRESSED_BLOB_MAGIC_1_0,
 };
-use crate::DataBlob;
+use crate::{DataBlob, LocalDatastoreLruCache};
 
 /// File system based chunk store
 pub struct ChunkStore {
@@ -366,6 +367,7 @@ impl ChunkStore {
         min_atime: i64,
         status: &mut GarbageCollectionStatus,
         worker: &dyn WorkerTaskContext,
+        cache: Option<&LocalDatastoreLruCache>,
     ) -> Result<(), Error> {
         // unwrap: only `None` in unit tests
         assert!(self.locker.is_some());
@@ -419,6 +421,30 @@ impl ChunkStore {
                         bad,
                         status,
                         || {
+                            if let Some(cache) = cache {
+                                // never lock bad chunks
+                                if filename.to_bytes().len() == 64 {
+                                    let digest = <[u8; 32]>::from_hex(filename.to_bytes())?;
+                                    match self.lock_chunk(&digest, Duration::from_secs(0)) {
+                                        Ok(_guard) => {
+                                            // don't remove if changed since locking
+                                            match fstatat(
+                                                Some(dirfd),
+                                                filename,
+                                                nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
+                                            ) {
+                                                Ok(stat) if stat.st_atime < min_atime => {
+                                                    let _ = cache.remove(&digest);
+                                                    return Ok(());
+                                                }
+                                                _ => return Ok(()),
+                                            }
+                                        }
+                                        Err(_) => return Ok(()),
+                                    }
+                                }
+                            }
+
                             unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir).map_err(
                                 |err| {
                                     format_err!(
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index beca72e9a..33d589b67 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1733,6 +1733,7 @@ impl DataStore {
                 min_atime,
                 &mut tmp_gc_status,
                 worker,
+                self.cache(),
             )?;
         } else {
             self.inner.chunk_store.sweep_unused_chunks(
@@ -1740,6 +1741,7 @@ impl DataStore {
                 min_atime,
                 &mut gc_status,
                 worker,
+                None,
             )?;
         }
 
-- 
2.47.3