[pbs-devel] [PATCH proxmox-backup v3 0/3] reduce GC S3 locking

[pbs-devel] [PATCH proxmox-backup v3 0/3] reduce GC S3 locking

[Fabian Grünbichler]

this patch series tries to reduce the number of open locks held by GC,
in particular in case most objects returned by the S3 backend are
garbage that need deletion.

the first patch reduces the number of open locks by at least a factor of
10 in the worst case (from up to 1000 to up to 100).

the second patch just refactors some now common code.

the third patch tries to reduce the number of delete calls for regular
GC runs by batching deletes more efficiently, while still periodically
flushing the deferred deletes.

v2: force deletion after delete queue has been pending for a certain
amount of time
v3: fix third patch based on Chris feedback

Fabian Grünbichler (3):
  GC: S3: reduce number of open FDs for to-be-deleted objects
  GC: S3: factor out batch object deletion
  GC: S3: phase2: do not force delete for every list iteration

 pbs-datastore/src/datastore.rs | 63 +++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 17 deletions(-)

-- 
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 v3 1/3] GC: S3: reduce number of open FDs for to-be-deleted objects

[Fabian Grünbichler]

listing objects on the S3 side will return batches containing up to 1000
objects. previously, if all those objects were garbage, phase2 would open and
hold the lock file for each of them and delete them using a single call. this
can easily run afoul the maximum number of open files allowed by the default
process limits, which is 1024.

converting the code to instead delete batches of (at most) 100 objects should
alleviate this issue until bumping the limit is deemed safe, while (in the
worst case) causing 10x the number of delete requests.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>
---

Notes:
    v2: >= LIMIT instead of > LIMIT, thanks Chris

 pbs-datastore/src/datastore.rs | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 0a5179230..09ec23fc4 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -58,6 +58,8 @@ pub const S3_DATASTORE_IN_USE_MARKER: &str = ".in-use";
 const NAMESPACE_MARKER_FILENAME: &str = ".namespace";
 // s3 put request times out after upload_size / 1 Kib/s, so about 2.3 hours for 8 MiB
 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;
 
 /// checks if auth_id is owner, or, if owner is a token, if
 /// auth_id is the user of the token
@@ -1657,7 +1659,7 @@ 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(1000);
+            let mut delete_list = Vec::with_capacity(S3_DELETE_BATCH_LIMIT);
             loop {
                 for content in list_bucket_result.contents {
                     let (chunk_path, digest, bad) =
@@ -1716,8 +1718,29 @@ impl DataStore {
                     }
 
                     chunk_count += 1;
+
+                    // drop guard because of async S3 call below
+                    drop(_guard);
+
+                    // limit pending deletes to avoid holding too many chunk flocks
+                    if delete_list.len() >= S3_DELETE_BATCH_LIMIT {
+                        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");
+                        }
+                        // release all chunk guards
+                        delete_list.clear();
+                    }
                 }
 
+                // delete the last batch of objects, if there are any remaining
                 if !delete_list.is_empty() {
                     let delete_objects_result = proxmox_async::runtime::block_on(
                         s3_client.delete_objects(
-- 
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 v3 2/3] GC: S3: factor out batch object deletion

[Fabian Grünbichler]

since we do it twice with identical code, move that code to a closure.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>
---

Notes:
    v1->v2: don't drop comment when extracting helper

 pbs-datastore/src/datastore.rs | 47 +++++++++++++++-------------------
 1 file changed, 21 insertions(+), 26 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 09ec23fc4..e9d6b46f3 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1660,6 +1660,25 @@ impl DataStore {
                     .context("failed to list chunk in s3 object store")?;
 
             let mut delete_list = Vec::with_capacity(S3_DELETE_BATCH_LIMIT);
+
+            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(())
+            };
             loop {
                 for content in list_bucket_result.contents {
                     let (chunk_path, digest, bad) =
@@ -1724,37 +1743,13 @@ impl DataStore {
 
                     // limit pending deletes to avoid holding too many chunk flocks
                     if delete_list.len() >= S3_DELETE_BATCH_LIMIT {
-                        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");
-                        }
-                        // release all chunk guards
-                        delete_list.clear();
+                        s3_delete_batch(&mut delete_list, s3_client)?;
                     }
                 }
 
                 // delete the last batch of objects, if there are any remaining
                 if !delete_list.is_empty() {
-                    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");
-                    }
-                    // release all chunk guards
-                    delete_list.clear();
+                    s3_delete_batch(&mut delete_list, s3_client)?;
                 }
 
                 // Process next batch of chunks if there is more
-- 
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 v3 3/3] GC: S3: phase2: do not force delete for every list iteration

[Fabian Grünbichler]

force delete if the first pending delete was inserted more than 5
minutes ago, and at the very end after the last iteration, instead of
after every processing every batch of 1000 listed objects. this reduces
the number of delete calls made to the backend, making regular garbage
collections that do not delete most objects cheaper, but means holding
the flocks for garbage chunks/objects longer.

Suggested-by: Christian Ebner <c.ebner@proxmox.com>
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    v1->v2: reworked to use age-based cutoff
    v2->v3:
    - prevent delete calls for empty lists (thanks Chris)
    - reset age correctly on first insert into delete list (thanks Chris)

 pbs-datastore/src/datastore.rs | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index e9d6b46f3..bc6f6b3ba 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -22,7 +22,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::TimeSpan;
+use proxmox_time::{epoch_i64, TimeSpan};
 use proxmox_worker_task::WorkerTaskContext;
 
 use pbs_api_types::{
@@ -60,6 +60,8 @@ const NAMESPACE_MARKER_FILENAME: &str = ".namespace";
 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;
 
 /// checks if auth_id is owner, or, if owner is a token, if
 /// auth_id is the user of the token
@@ -1660,6 +1662,7 @@ impl DataStore {
                     .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>|
@@ -1730,6 +1733,11 @@ impl DataStore {
                                         std::fs::remove_file(chunk_path)?;
                                     }
                                 }
+
+                                // set age based on first insertion
+                                if delete_list.is_empty() {
+                                    delete_list_age = epoch_i64();
+                                }
                                 delete_list.push((content.key, _chunk_guard));
                                 Ok(())
                             },
@@ -1742,16 +1750,13 @@ impl DataStore {
                     drop(_guard);
 
                     // limit pending deletes to avoid holding too many chunk flocks
-                    if delete_list.len() >= S3_DELETE_BATCH_LIMIT {
+                    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 the last batch of objects, if there are any remaining
-                if !delete_list.is_empty() {
-                    s3_delete_batch(&mut delete_list, s3_client)?;
-                }
-
                 // Process next batch of chunks if there is more
                 if list_bucket_result.is_truncated {
                     list_bucket_result =
@@ -1764,6 +1769,12 @@ impl DataStore {
 
                 break;
             }
+
+            // delete the last batch of objects, if there are any remaining
+            if !delete_list.is_empty() {
+                s3_delete_batch(&mut delete_list, s3_client)?;
+            }
+
             info!("processed {chunk_count} total chunks");
 
             // Phase 2 GC of Filesystem backed storage is phase 3 for S3 backed GC
-- 
2.47.3



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

[pbs-devel] applied: [PATCH proxmox-backup v3 0/3] reduce GC S3 locking

[Thomas Lamprecht]

On Fri, 21 Nov 2025 13:38:22 +0100, Fabian Grünbichler wrote:
> this patch series tries to reduce the number of open locks held by GC,
> in particular in case most objects returned by the S3 backend are
> garbage that need deletion.
> 
> the first patch reduces the number of open locks by at least a factor of
> 10 in the worst case (from up to 1000 to up to 100).
> 
> [...]

Applied, thanks!

[1/3] GC: S3: reduce number of open FDs for to-be-deleted objects
      commit: 99a35fc5214b8a7a8277849496acabcc911549e7
[2/3] GC: S3: factor out batch object deletion
      commit: e70b7535a87b038e265224ae89557da6aa845445
[3/3] GC: S3: phase2: do not force delete for every list iteration
      commit: 4e2b819061f1369ffa73fc34844379a2560bdd17


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