[pbs-devel] [PATCH proxmox-backup 0/4] followups for garbage collection

[pbs-devel] [PATCH proxmox-backup 0/4] followups for garbage collection

[Christian Ebner]

This patch series contains some followup to the recently introduced
changes to phase 2 of garbage collection on s3 backed datastores [0]
and clenups for marker files during phase 3 [1].

The first patch introduces a dedicated struct for tracking objects
to be deleted from the s3 object store during phase 2, bundling the
length and age based deletion threshold logic to the list state.

Subsequent patches rename and adapt the chunk store iterator to
optimize for regular chunk filenames and clearly distinguish chunks
by directory entry filename extension.

[0] https://lore.proxmox.com/pbs-devel/2b637884-f6bd-434d-be00-fa4e9d9e2dcb@proxmox.com/T/
[1] https://lore.proxmox.com/pbs-devel/1764145262.ycdoq9dzrx.astroid@yuna.none/T/

proxmox-backup:

Christian Ebner (4):
  GC: Move S3 delete list state and logic to a dedicated struct
  chunk store: rename and limit scope for chunk store iterator
  chunk store: invert chunk filename checks in chunk store iterator
  chunk store: return chunk extension and check for used marker

 pbs-datastore/src/chunk_store.rs |  61 ++++++++++----
 pbs-datastore/src/datastore.rs   | 132 ++++++++++++++++++-------------
 2 files changed, 121 insertions(+), 72 deletions(-)


Summary over all repositories:
  2 files changed, 121 insertions(+), 72 deletions(-)

-- 
Generated by git-murpp 0.8.1


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 1/4] GC: Move S3 delete list state and logic to a dedicated struct

[Christian Ebner]

To better keep track of the state and declutter the code on the
callsites, bundle the S3 delete list and it's logic by a dedicated
struct. Since the check for empty lists is now performed as part
of the deletion related methods, the callsites can get rid of that.

Further, avoid the proxmox_time::epoch_i64() and use SystemTime and
Duration with their methods directly.

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 132 +++++++++++++++++++--------------
 1 file changed, 75 insertions(+), 57 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 36550ff63..a0890340b 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -24,7 +24,7 @@ use proxmox_sys::error::SysError;
 use proxmox_sys::fs::{file_read_optional_string, replace_file, CreateOptions};
 use proxmox_sys::linux::procfs::MountInfo;
 use proxmox_sys::process_locker::{ProcessLockExclusiveGuard, ProcessLockSharedGuard};
-use proxmox_time::{epoch_i64, TimeSpan};
+use proxmox_time::TimeSpan;
 use proxmox_worker_task::WorkerTaskContext;
 
 use pbs_api_types::{
@@ -64,7 +64,7 @@ const CHUNK_LOCK_TIMEOUT: Duration = Duration::from_secs(3 * 60 * 60);
 // s3 deletion batch size to avoid 1024 open files soft limit
 const S3_DELETE_BATCH_LIMIT: usize = 100;
 // max defer time for s3 batch deletions
-const S3_DELETE_DEFER_LIMIT_SECONDS: i64 = 60 * 5;
+const S3_DELETE_DEFER_LIMIT_SECONDS: Duration = Duration::from_secs(60 * 5);
 
 /// checks if auth_id is owner, or, if owner is a token, if
 /// auth_id is the user of the token
@@ -1704,40 +1704,8 @@ impl DataStore {
                 proxmox_async::runtime::block_on(s3_client.list_objects_v2(&prefix, None))
                     .context("failed to list chunk in s3 object store")?;
 
-            let mut delete_list = Vec::with_capacity(S3_DELETE_BATCH_LIMIT);
-            let mut delete_list_age = epoch_i64();
-
-            let s3_delete_batch = |delete_list: &mut Vec<(S3ObjectKey, BackupLockGuard)>,
-                                   s3_client: &Arc<S3Client>|
-             -> Result<(), Error> {
-                let delete_objects_result = proxmox_async::runtime::block_on(
-                    s3_client.delete_objects(
-                        &delete_list
-                            .iter()
-                            .map(|(key, _)| key.clone())
-                            .collect::<Vec<S3ObjectKey>>(),
-                    ),
-                )?;
-                if let Some(_err) = delete_objects_result.error {
-                    bail!("failed to delete some objects");
-                }
-                // drops all chunk flock guards
-                delete_list.clear();
-                Ok(())
-            };
-
-            let add_to_delete_list =
-                |delete_list: &mut Vec<(S3ObjectKey, BackupLockGuard)>,
-                 delete_list_age: &mut i64,
-                 key: S3ObjectKey,
-                 _chunk_guard: BackupLockGuard| {
-                    // set age based on first insertion
-                    if delete_list.is_empty() {
-                        *delete_list_age = epoch_i64();
-                    }
-                    delete_list.push((key, _chunk_guard));
-                };
-
+            let mut delete_list =
+                S3DeleteList::with_thresholds(S3_DELETE_BATCH_LIMIT, S3_DELETE_DEFER_LIMIT_SECONDS);
             loop {
                 for content in list_bucket_result.contents {
                     let (chunk_path, digest, bad) =
@@ -1792,12 +1760,7 @@ impl DataStore {
                                             std::fs::remove_file(chunk_path)?;
                                         }
                                     }
-                                    add_to_delete_list(
-                                        &mut delete_list,
-                                        &mut delete_list_age,
-                                        content.key,
-                                        _chunk_guard,
-                                    );
+                                    delete_list.push(content.key, _chunk_guard);
                                     Ok(())
                                 },
                             )?;
@@ -1805,12 +1768,7 @@ impl DataStore {
                     } else {
                         gc_status.removed_chunks += 1;
                         gc_status.removed_bytes += content.size;
-                        add_to_delete_list(
-                            &mut delete_list,
-                            &mut delete_list_age,
-                            content.key,
-                            _chunk_guard,
-                        );
+                        delete_list.push(content.key, _chunk_guard);
                     }
 
                     chunk_count += 1;
@@ -1819,12 +1777,7 @@ impl DataStore {
                     drop(_guard);
 
                     // limit pending deletes to avoid holding too many chunk flocks
-                    if delete_list.len() >= S3_DELETE_BATCH_LIMIT
-                        || (!delete_list.is_empty()
-                            && epoch_i64() - delete_list_age > S3_DELETE_DEFER_LIMIT_SECONDS)
-                    {
-                        s3_delete_batch(&mut delete_list, s3_client)?;
-                    }
+                    delete_list.conditional_delete_and_drop_locks(s3_client)?;
                 }
                 // Process next batch of chunks if there is more
                 if list_bucket_result.is_truncated {
@@ -1840,9 +1793,7 @@ impl DataStore {
             }
 
             // delete the last batch of objects, if there are any remaining
-            if !delete_list.is_empty() {
-                s3_delete_batch(&mut delete_list, s3_client)?;
-            }
+            delete_list.delete_and_drop_locks(s3_client)?;
 
             info!("processed {chunk_count} total chunks");
 
@@ -2781,3 +2732,70 @@ impl DataStore {
         result
     }
 }
+
+/// Track S3 object keys to be deleted by garbage collection while holding their file lock.
+struct S3DeleteList {
+    list: Vec<(S3ObjectKey, BackupLockGuard)>,
+    first_entry_added: SystemTime,
+    age_threshold: Duration,
+    capacity_threshold: usize,
+}
+
+impl S3DeleteList {
+    /// Create a new list instance with given capacity and age thresholds.
+    fn with_thresholds(capacity_threshold: usize, age_threshold: Duration) -> Self {
+        Self {
+            first_entry_added: SystemTime::now(), // init only, updated once added
+            list: Vec::with_capacity(capacity_threshold),
+            age_threshold,
+            capacity_threshold,
+        }
+    }
+
+    /// 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) {
+        // set age based on first insertion
+        if self.list.is_empty() {
+            self.first_entry_added = SystemTime::now();
+        }
+        self.list.push((key, guard));
+    }
+
+    /// Delete the objects in the list via the provided S3 client instance.
+    /// Clears the list contents and frees the per-chunk file locks.
+    fn delete_and_drop_locks(&mut self, s3_client: &Arc<S3Client>) -> Result<(), Error> {
+        if self.list.is_empty() {
+            return Ok(());
+        }
+        let delete_objects_result = proxmox_async::runtime::block_on(
+            s3_client.delete_objects(
+                &self
+                    .list
+                    .iter()
+                    .map(|(key, _)| key.clone())
+                    .collect::<Vec<S3ObjectKey>>(),
+            ),
+        )?;
+        if delete_objects_result.error.is_some() {
+            bail!("failed to delete some objects");
+        }
+        // drops all chunk flock guards
+        self.list.clear();
+        Ok(())
+    }
+
+    /// Delete the object stored in the list if either the list exceeds the capacity threshold or
+    /// the delete list age threshold.
+    fn conditional_delete_and_drop_locks(
+        &mut self,
+        s3_client: &Arc<S3Client>,
+    ) -> Result<(), Error> {
+        if self.list.len() >= self.capacity_threshold
+            || (!self.list.is_empty() && self.first_entry_added.elapsed()? > self.age_threshold)
+        {
+            self.delete_and_drop_locks(s3_client)?;
+        }
+        Ok(())
+    }
+}
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 2/4] chunk store: rename and limit scope for chunk store iterator

[Christian Ebner]

Since the introduction of marker files for datastores backed by s3
object stores, the iterator can also include the <digest>.using file
entries and chunk marker files which have 0 size. Also, the returned
entries for regular datastores are not guaranteed be of type file, as
that is only checked by stating the entry afterwards, so there is no
guarantee to only get chunks by the iterator.

Therefore, rename the method to get_chunk_store_iterator(), which is
more generic and does not imply the returned entries are chunks.

While at it, also limit the scope for the method as this is not used
outside the module.

No functional changes intended.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 7fe09b914..a5e5f6261 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -276,7 +276,7 @@ impl ChunkStore {
         Ok(true)
     }
 
-    pub fn get_chunk_iterator(
+    fn get_chunk_store_iterator(
         &self,
     ) -> Result<
         impl std::iter::FusedIterator<
@@ -397,7 +397,7 @@ impl ChunkStore {
         let mut last_percentage = 0;
         let mut chunk_count = 0;
 
-        for (entry, percentage, bad) in self.get_chunk_iterator()? {
+        for (entry, percentage, bad) in self.get_chunk_store_iterator()? {
             if last_percentage != percentage {
                 last_percentage = percentage;
                 info!("processed {percentage}% ({chunk_count} chunks)");
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 3/4] chunk store: invert chunk filename checks in chunk store iterator

[Christian Ebner]

Optimizes the chunk filename check towards regular chunk files by
explicitley checking for the correct length.

While the check for ascii hexdigits needs to be stated twice, this
avoids to check for the `.bad` extension if the chunk filename did
already match the expected length.

This will also help to better distinguish bad chunks and chunks
used markers for s3 datastores in subsequent changes.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index a5e5f6261..7980938ad 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -315,15 +315,20 @@ impl ChunkStore {
                         Some(Ok(entry)) => {
                             // skip files if they're not a hash
                             let bytes = entry.file_name().to_bytes();
-                            if bytes.len() != 64 && bytes.len() != 64 + ".0.bad".len() {
-                                continue;
+
+                            if bytes.len() == 64 && bytes.iter().take(64).all(u8::is_ascii_hexdigit)
+                            {
+                                return Some((Ok(entry), percentage, false));
                             }
-                            if !bytes.iter().take(64).all(u8::is_ascii_hexdigit) {
-                                continue;
+
+                            if bytes.len() == 64 + ".0.bad".len()
+                                && bytes.iter().take(64).all(u8::is_ascii_hexdigit)
+                            {
+                                let bad = bytes.ends_with(b".bad");
+                                return Some((Ok(entry), percentage, bad));
                             }
 
-                            let bad = bytes.ends_with(b".bad");
-                            return Some((Ok(entry), percentage, bad));
+                            continue;
                         }
                         Some(Err(err)) => {
                             // stop after first error
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 4/4] chunk store: return chunk extension and check for used marker

[Christian Ebner]

Clearly distinguish the cases for `bad` and `using` extensions for
items returned by the chunk iterator. Directory entries with
filenames which matched the expected length but not the extension
are now skipped over.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 48 ++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 11 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 7980938ad..ad517391d 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -280,7 +280,11 @@ impl ChunkStore {
         &self,
     ) -> Result<
         impl std::iter::FusedIterator<
-            Item = (Result<proxmox_sys::fs::ReadDirEntry, Error>, usize, bool),
+            Item = (
+                Result<proxmox_sys::fs::ReadDirEntry, Error>,
+                usize,
+                ChunkExt,
+            ),
         >,
         Error,
     > {
@@ -318,14 +322,20 @@ impl ChunkStore {
 
                             if bytes.len() == 64 && bytes.iter().take(64).all(u8::is_ascii_hexdigit)
                             {
-                                return Some((Ok(entry), percentage, false));
+                                return Some((Ok(entry), percentage, ChunkExt::None));
                             }
 
                             if bytes.len() == 64 + ".0.bad".len()
                                 && bytes.iter().take(64).all(u8::is_ascii_hexdigit)
                             {
-                                let bad = bytes.ends_with(b".bad");
-                                return Some((Ok(entry), percentage, bad));
+                                let chunk_ext = if bytes.ends_with(b".bad") {
+                                    ChunkExt::Bad
+                                } else if bytes.ends_with(USING_MARKER_FILENAME_EXT.as_bytes()) {
+                                    ChunkExt::UsedMarker
+                                } else {
+                                    continue;
+                                };
+                                return Some((Ok(entry), percentage, chunk_ext));
                             }
 
                             continue;
@@ -334,7 +344,7 @@ impl ChunkStore {
                             // stop after first error
                             done = true;
                             // and pass the error through:
-                            return Some((Err(err), percentage, false));
+                            return Some((Err(err), percentage, ChunkExt::None));
                         }
                         None => (), // open next directory
                     }
@@ -367,7 +377,7 @@ impl ChunkStore {
                         return Some((
                             Err(format_err!("unable to read subdir '{subdir}' - {err}")),
                             percentage,
-                            false,
+                            ChunkExt::None,
                         ));
                     }
                 }
@@ -402,7 +412,8 @@ impl ChunkStore {
         let mut last_percentage = 0;
         let mut chunk_count = 0;
 
-        for (entry, percentage, bad) in self.get_chunk_store_iterator()? {
+        for (entry, percentage, chunk_ext) in self.get_chunk_store_iterator()? {
+            let bad = chunk_ext.is_bad();
             if last_percentage != percentage {
                 last_percentage = percentage;
                 info!("processed {percentage}% ({chunk_count} chunks)");
@@ -433,10 +444,7 @@ impl ChunkStore {
                     drop(lock);
                     continue;
                 }
-                if filename
-                    .to_bytes()
-                    .ends_with(USING_MARKER_FILENAME_EXT.as_bytes())
-                {
+                if chunk_ext.is_used_marker() {
                     unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir).map_err(|err| {
                         format_err!("unlinking chunk using marker {filename:?} failed - {err}")
                     })?;
@@ -908,6 +916,24 @@ impl ChunkStore {
     }
 }
 
+#[derive(PartialEq)]
+/// Chunk iterator directory entry filename extension
+enum ChunkExt {
+    None,
+    Bad,
+    UsedMarker,
+}
+
+impl ChunkExt {
+    fn is_bad(&self) -> bool {
+        *self == Self::Bad
+    }
+
+    fn is_used_marker(&self) -> bool {
+        *self == Self::UsedMarker
+    }
+}
+
 #[test]
 fn test_chunk_store1() {
     let mut path = std::fs::canonicalize(".").unwrap(); // we need absolute path
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel