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

[pbs-devel] [PATCH proxmox-backup 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.

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: delete last partial batch of objects at the very end

 pbs-datastore/src/datastore.rs | 49 +++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 16 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 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>
---
 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..1afcef53a 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

Re: [pbs-devel] [PATCH proxmox-backup 1/3] GC: S3: reduce number of open FDs for to-be-deleted objects

[Christian Ebner]

one comment inline

On 11/21/25 10:06 AM, Fabian Grünbichler wrote:
> 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>
> ---
>   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..1afcef53a 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 {

off by one error: this should never exceed the limit but rather delete 
once it is reached.

> +                        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(



_______________________________________________
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/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>
---
 pbs-datastore/src/datastore.rs | 46 +++++++++++++++-------------------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 1afcef53a..f89fd28b0 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1660,6 +1660,24 @@ 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");
+                }
+                delete_list.clear();
+                Ok(())
+            };
             loop {
                 for content in list_bucket_result.contents {
                     let (chunk_path, digest, bad) =
@@ -1724,37 +1742,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 3/3] GC: S3: phase2: delete last partial batch of objects at the very end

[Fabian Grünbichler]

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.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index f89fd28b0..f26de672a 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1745,12 +1745,6 @@ impl DataStore {
                         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 =
@@ -1763,6 +1757,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

Re: [pbs-devel] [PATCH proxmox-backup 3/3] GC: S3: phase2: delete last partial batch of objects at the very end

[Christian Ebner]

While going trough the rest of the series in detail now, one idea right 
away.

On 11/21/25 10:06 AM, Fabian Grünbichler wrote:
> 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.

We could avoid holding the flock for to long (e.g. GC over several days 
because of super slow local datastore cache, S3 backend, ...) by setting 
(or resetting) a timer on each last delete list insert, and not only 
using the batch size to decide if to perform the deleteObjects() call, 
but rather compare if a timeout has been elapsed.

This would safeguard us from locking some chunks way to long, causing 
potential issues with concurrent backups, but not trow out all the 
benefits this patch brings.

What do you think? I could send that as followup if you like.


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

Re: [pbs-devel] [PATCH proxmox-backup 3/3] GC: S3: phase2: delete last partial batch of objects at the very end

[Fabian Grünbichler]

On November 21, 2025 10:31 am, Christian Ebner wrote:
> While going trough the rest of the series in detail now, one idea right 
> away.
> 
> On 11/21/25 10:06 AM, Fabian Grünbichler wrote:
>> 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.
> 
> We could avoid holding the flock for to long (e.g. GC over several days 
> because of super slow local datastore cache, S3 backend, ...) by setting 
> (or resetting) a timer on each last delete list insert, and not only 
> using the batch size to decide if to perform the deleteObjects() call, 
> but rather compare if a timeout has been elapsed.
> 
> This would safeguard us from locking some chunks way to long, causing 
> potential issues with concurrent backups, but not trow out all the 
> benefits this patch brings.
> 
> What do you think? I could send that as followup if you like.

considerations like this were why I split this out as separate patch ;)

the loop here basically does:
- one S3 call to (continue to) list objects in the bucket
  (potentially expensive), then for each object:
-- maps each object back to a chunk (free)
-- does some local operations
   (stat, empty marker handling - these should not take too long?)
-- remove from cache if garbage
   (might take a bit if local storage is very slow?)

so yeah, we should probably cap the max. number of list calls before we
trigger the deletion, to avoid locking a garbage chunk in the first 1000
objects until the end of GC, if there is no further garbage to fill up
the batch of 100.. either by number of iterations since the first
not-yet-process delete insertion, or via timestamp?



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

Re: [pbs-devel] [PATCH proxmox-backup 3/3] GC: S3: phase2: delete last partial batch of objects at the very end

[Christian Ebner]

On 11/21/25 10:45 AM, Fabian Grünbichler wrote:
> On November 21, 2025 10:31 am, Christian Ebner wrote:
>> While going trough the rest of the series in detail now, one idea right
>> away.
>>
>> On 11/21/25 10:06 AM, Fabian Grünbichler wrote:
>>> 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.
>>
>> We could avoid holding the flock for to long (e.g. GC over several days
>> because of super slow local datastore cache, S3 backend, ...) by setting
>> (or resetting) a timer on each last delete list insert, and not only
>> using the batch size to decide if to perform the deleteObjects() call,
>> but rather compare if a timeout has been elapsed.
>>
>> This would safeguard us from locking some chunks way to long, causing
>> potential issues with concurrent backups, but not trow out all the
>> benefits this patch brings.
>>
>> What do you think? I could send that as followup if you like.
> 
> considerations like this were why I split this out as separate patch ;)
> 
> the loop here basically does:
> - one S3 call to (continue to) list objects in the bucket
>    (potentially expensive), then for each object:
> -- maps each object back to a chunk (free)
> -- does some local operations
>     (stat, empty marker handling - these should not take too long?)
> -- remove from cache if garbage
>     (might take a bit if local storage is very slow?)
> 
> so yeah, we should probably cap the max. number of list calls before we
> trigger the deletion, to avoid locking a garbage chunk in the first 1000
> objects until the end of GC, if there is no further garbage to fill up
> the batch of 100.. either by number of iterations since the first
> not-yet-process delete insertion, or via timestamp?

Maybe best to reuse a fraction of the already defined 
CHUNK_LOCK_TIMEOUT, so even if there are a lot of list objects 
iterations (many chunks), we don't loose out on the delete list collections?


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

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

[Christian Ebner]

On 11/21/25 10:06 AM, 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).
> 
> 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.
> 
> 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: delete last partial batch of objects at the very end
> 
>   pbs-datastore/src/datastore.rs | 49 +++++++++++++++++++++++-----------
>   1 file changed, 33 insertions(+), 16 deletions(-)
> 

Apart from the off by one error the patches look good to me and 
significantly reduce the likelihood to run into the soft limit for open 
files. And as a positive side effect, this makes garbage collection even 
more efficient when only a limited number of chunks per list batch has 
to be removed, collecting them into a reduced number of API calls.

With the one issue addressed, consider:

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>


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

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

[Fabian Grünbichler]

by https://lore.proxmox.com/pbs-devel/20251121101849.463119-1-f.gruenbichler@proxmox.com

thanks Chris for the quick review and testing!

On November 21, 2025 10:05 am, 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).
> 
> 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.
> 
> 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: delete last partial batch of objects at the very end
> 
>  pbs-datastore/src/datastore.rs | 49 +++++++++++++++++++++++-----------
>  1 file changed, 33 insertions(+), 16 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 mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel