[pbs-devel] [PATCH proxmox-backup 0/2] fix possible deadlock for S3 garbage

[pbs-devel] [PATCH proxmox-backup 0/2] fix possible deadlock for S3 garbage

[Christian Ebner]

The first patch fixes a possible deadlock situation:
Holding a std::sync::Mutex guard across .await boundaries can lead to
deadlock situations and must be avoided by all costs. Fix one such
ocurence when deleting objects from S3 object stores while still
holding the lock.

The second patch adapts an ill-chosen method name and incorrect
comment.

These patches are followups to be applied on top of:
https://lore.proxmox.com/pbs-devel/20251001111915.2001026-1-f.gruenbichler@proxmox.com/T/

Christian Ebner (2):
  datastore: gc: drop mutex lock before entering async context
  datastore: gc: rename method and adapt incorrect comments

 pbs-datastore/src/datastore.rs | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 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/2] datastore: gc: drop mutex lock before entering async context

[Christian Ebner]

The list of objects to be deleted from the S3 backend is already
generated at this point, so no need to hold the lock any longer.
Avoids holding the lock while entering an async context, which
can lead to deadlocks when held across await boundaries.

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

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 8d6aea9d1..2e62590f9 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1670,6 +1670,8 @@ impl DataStore {
                     }
                 }
 
+                drop(lock);
+
                 if !delete_list.is_empty() {
                     let delete_objects_result =
                         proxmox_async::runtime::block_on(s3_client.delete_objects(&delete_list))?;
@@ -1679,8 +1681,6 @@ impl DataStore {
                     delete_list.clear();
                 }
 
-                drop(lock);
-
                 // Process next batch of chunks if there is more
                 if list_bucket_result.is_truncated {
                     list_bucket_result =
-- 
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/2] datastore: gc: rename method and adapt incorrect comments

[Christian Ebner]

ss the helper does not really mark the chunks but rather checks
if it should be added to the delete list.

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

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 2e62590f9..b76bb1987 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1654,7 +1654,7 @@ impl DataStore {
 
                 for content in list_bucket_result.contents {
                     if self
-                        .mark_chunk_for_object_key(
+                        .check_chunk_marker_for_object_key(
                             &content.key,
                             content.size,
                             min_atime,
@@ -1796,10 +1796,9 @@ impl DataStore {
         Ok(())
     }
 
-    // Mark the chunk marker in the local cache store for the given object key as in use
-    // by updating it's atime.
-    // Returns Ok(true) if the chunk was updated and Ok(false) if the object was not a chunk.
-    fn mark_chunk_for_object_key(
+    // Check if the chunk marker in the local cache store for the given object key marks the chunk
+    // object as in-use or adds it to the list of objects to delete.
+    fn check_chunk_marker_for_object_key(
         &self,
         object_key: &S3ObjectKey,
         size: u64,
-- 
2.47.3



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

[pbs-devel] partially-applied: [PATCH proxmox-backup 0/2] fix possible deadlock for S3 garbage

[Fabian Grünbichler]

On Thu, 02 Oct 2025 11:11:40 +0200, Christian Ebner wrote:
> The first patch fixes a possible deadlock situation:
> Holding a std::sync::Mutex guard across .await boundaries can lead to
> deadlock situations and must be avoided by all costs. Fix one such
> ocurence when deleting objects from S3 object stores while still
> holding the lock.
> 
> The second patch adapts an ill-chosen method name and incorrect
> comment.
> 
> [...]

Applied, thanks!

[1/2] datastore: gc: drop mutex lock before entering async context
      commit: fa1c2021bbdfc264f1be9e97e5745d9c6a54cd18

skipped the second patch for now, see reply there!

Best regards,
-- 
Fabian Grünbichler <f.gruenbichler@proxmox.com>


_______________________________________________
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 2/2] datastore: gc: rename method and adapt incorrect comments

[Fabian Grünbichler]

On October 2, 2025 11:11 am, Christian Ebner wrote:
> ss the helper does not really mark the chunks but rather checks
> if it should be added to the delete list.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 2e62590f9..b76bb1987 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1654,7 +1654,7 @@ impl DataStore {
>  
>                  for content in list_bucket_result.contents {
>                      if self
> -                        .mark_chunk_for_object_key(
> +                        .check_chunk_marker_for_object_key(
>                              &content.key,
>                              content.size,
>                              min_atime,
> @@ -1796,10 +1796,9 @@ impl DataStore {
>          Ok(())
>      }
>  
> -    // Mark the chunk marker in the local cache store for the given object key as in use
> -    // by updating it's atime.
> -    // Returns Ok(true) if the chunk was updated and Ok(false) if the object was not a chunk.
> -    fn mark_chunk_for_object_key(
> +    // Check if the chunk marker in the local cache store for the given object key marks the chunk
> +    // object as in-use or adds it to the list of objects to delete.
> +    fn check_chunk_marker_for_object_key(

this is still wrong - this method doesn't mark anything as in-use, but
it does remove it from the cache (if eligible), which will also remove
the cache marker!

so either we really make this a pure check helper (and let it return
whether the corresponding digest should be removed from the cache?) or
we find some better name ;)

>          &self,
>          object_key: &S3ObjectKey,
>          size: u64,
> -- 
> 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

Re: [pbs-devel] [PATCH proxmox-backup 2/2] datastore: gc: rename method and adapt incorrect comments

[Christian Ebner]

On 10/2/25 2:18 PM, Fabian Grünbichler wrote:
> On October 2, 2025 11:11 am, Christian Ebner wrote:
>> ss the helper does not really mark the chunks but rather checks
>> if it should be added to the delete list.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   pbs-datastore/src/datastore.rs | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>> index 2e62590f9..b76bb1987 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -1654,7 +1654,7 @@ impl DataStore {
>>   
>>                   for content in list_bucket_result.contents {
>>                       if self
>> -                        .mark_chunk_for_object_key(
>> +                        .check_chunk_marker_for_object_key(
>>                               &content.key,
>>                               content.size,
>>                               min_atime,
>> @@ -1796,10 +1796,9 @@ impl DataStore {
>>           Ok(())
>>       }
>>   
>> -    // Mark the chunk marker in the local cache store for the given object key as in use
>> -    // by updating it's atime.
>> -    // Returns Ok(true) if the chunk was updated and Ok(false) if the object was not a chunk.
>> -    fn mark_chunk_for_object_key(
>> +    // Check if the chunk marker in the local cache store for the given object key marks the chunk
>> +    // object as in-use or adds it to the list of objects to delete.
>> +    fn check_chunk_marker_for_object_key(
> 
> this is still wrong - this method doesn't mark anything as in-use, but
> it does remove it from the cache (if eligible), which will also remove
> the cache marker!

Well... it states that it checks whether the marker file marks the chunk 
as in-use (by it's presence). but yeah, I agree that it is not worded to 
be easily comprehended.

Also forgot to adapt the info message just below, so this needs to be 
included as well anyways.

I will improve wording and send this as part of the S3 upload race 
patches I'm working on.


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