[pbs-devel] [PATCH proxmox-backup] garbage-collect: switch to tokio Mutex

[pbs-devel] [PATCH proxmox-backup] garbage-collect: switch to tokio Mutex

[Fabian Grünbichler]

this Mutex is held for the duration of the whole garbage collection run, which
now includes async calls into S3 code. this is not a highly contested lock, so
switching to the more expensive tokio variant shouldn't cause noticeable
overhead, and fix potential deadlock problems that could occur because of
holding the lock guard across await points.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
also found while going through std::sync::Mutex usage with S3

 pbs-datastore/src/datastore.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 7cf020fc0..68043e7a5 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -136,7 +136,7 @@ pub fn ensure_datastore_is_mounted(config: &DataStoreConfig) -> Result<(), Error
 /// management interface for backup.
 pub struct DataStoreImpl {
     chunk_store: Arc<ChunkStore>,
-    gc_mutex: Mutex<()>,
+    gc_mutex: tokio::sync::Mutex<()>,
     last_gc_status: Mutex<GarbageCollectionStatus>,
     verify_new: bool,
     chunk_order: ChunkOrder,
@@ -152,7 +152,7 @@ impl DataStoreImpl {
     pub(crate) unsafe fn new_test() -> Arc<Self> {
         Arc::new(Self {
             chunk_store: Arc::new(unsafe { ChunkStore::panic_store() }),
-            gc_mutex: Mutex::new(()),
+            gc_mutex: tokio::sync::Mutex::new(()),
             last_gc_status: Mutex::new(GarbageCollectionStatus::default()),
             verify_new: false,
             chunk_order: Default::default(),
@@ -513,7 +513,7 @@ impl DataStore {
 
         Ok(DataStoreImpl {
             chunk_store,
-            gc_mutex: Mutex::new(()),
+            gc_mutex: tokio::sync::Mutex::new(()),
             last_gc_status: Mutex::new(gc_status),
             verify_new: config.verify_new.unwrap_or(false),
             chunk_order: tuning.chunk_order.unwrap_or_default(),
-- 
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] garbage-collect: switch to tokio Mutex

[Christian Ebner]

On 9/29/25 11:32 AM, Fabian Grünbichler wrote:
> this Mutex is held for the duration of the whole garbage collection run, which
> now includes async calls into S3 code. this is not a highly contested lock, so
> switching to the more expensive tokio variant shouldn't cause noticeable
> overhead, and fix potential deadlock problems that could occur because of
> holding the lock guard across await points.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> also found while going through std::sync::Mutex usage with S3
> 
>   pbs-datastore/src/datastore.rs | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 7cf020fc0..68043e7a5 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -136,7 +136,7 @@ pub fn ensure_datastore_is_mounted(config: &DataStoreConfig) -> Result<(), Error
>   /// management interface for backup.
>   pub struct DataStoreImpl {
>       chunk_store: Arc<ChunkStore>,
> -    gc_mutex: Mutex<()>,
> +    gc_mutex: tokio::sync::Mutex<()>,
>       last_gc_status: Mutex<GarbageCollectionStatus>,
>       verify_new: bool,
>       chunk_order: ChunkOrder,
> @@ -152,7 +152,7 @@ impl DataStoreImpl {
>       pub(crate) unsafe fn new_test() -> Arc<Self> {
>           Arc::new(Self {
>               chunk_store: Arc::new(unsafe { ChunkStore::panic_store() }),
> -            gc_mutex: Mutex::new(()),
> +            gc_mutex: tokio::sync::Mutex::new(()),
>               last_gc_status: Mutex::new(GarbageCollectionStatus::default()),
>               verify_new: false,
>               chunk_order: Default::default(),
> @@ -513,7 +513,7 @@ impl DataStore {
>   
>           Ok(DataStoreImpl {
>               chunk_store,
> -            gc_mutex: Mutex::new(()),
> +            gc_mutex: tokio::sync::Mutex::new(()),
>               last_gc_status: Mutex::new(gc_status),
>               verify_new: config.verify_new.unwrap_or(false),
>               chunk_order: tuning.chunk_order.unwrap_or_default(),

This patch breaks the PBS build for me. Also, not sure if just replacing 
the type is enough, as the tokio::sync::Mutex::lock() is now async, so 
must be awaited afaiu from 
https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html#method.lock


_______________________________________________
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] garbage-collect: switch to tokio Mutex

[Fabian Grünbichler]

On September 29, 2025 6:07 pm, Christian Ebner wrote:
> On 9/29/25 11:32 AM, Fabian Grünbichler wrote:
>> this Mutex is held for the duration of the whole garbage collection run, which
>> now includes async calls into S3 code. this is not a highly contested lock, so
>> switching to the more expensive tokio variant shouldn't cause noticeable
>> overhead, and fix potential deadlock problems that could occur because of
>> holding the lock guard across await points.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> also found while going through std::sync::Mutex usage with S3
>> 
>>   pbs-datastore/src/datastore.rs | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>> index 7cf020fc0..68043e7a5 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -136,7 +136,7 @@ pub fn ensure_datastore_is_mounted(config: &DataStoreConfig) -> Result<(), Error
>>   /// management interface for backup.
>>   pub struct DataStoreImpl {
>>       chunk_store: Arc<ChunkStore>,
>> -    gc_mutex: Mutex<()>,
>> +    gc_mutex: tokio::sync::Mutex<()>,
>>       last_gc_status: Mutex<GarbageCollectionStatus>,
>>       verify_new: bool,
>>       chunk_order: ChunkOrder,
>> @@ -152,7 +152,7 @@ impl DataStoreImpl {
>>       pub(crate) unsafe fn new_test() -> Arc<Self> {
>>           Arc::new(Self {
>>               chunk_store: Arc::new(unsafe { ChunkStore::panic_store() }),
>> -            gc_mutex: Mutex::new(()),
>> +            gc_mutex: tokio::sync::Mutex::new(()),
>>               last_gc_status: Mutex::new(GarbageCollectionStatus::default()),
>>               verify_new: false,
>>               chunk_order: Default::default(),
>> @@ -513,7 +513,7 @@ impl DataStore {
>>   
>>           Ok(DataStoreImpl {
>>               chunk_store,
>> -            gc_mutex: Mutex::new(()),
>> +            gc_mutex: tokio::sync::Mutex::new(()),
>>               last_gc_status: Mutex::new(gc_status),
>>               verify_new: config.verify_new.unwrap_or(false),
>>               chunk_order: tuning.chunk_order.unwrap_or_default(),
> 
> This patch breaks the PBS build for me. Also, not sure if just replacing 
> the type is enough, as the tokio::sync::Mutex::lock() is now async, so 
> must be awaited afaiu from 
> https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html#method.lock

yes, I must have run my test build in the wrong work tree :-/

given that this also needs unwind safety by virtue of being handled
across worker task boundaries, it might be easiest to switch to an flock
instead..


_______________________________________________
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] garbage-collect: switch to tokio Mutex

[Fabian Grünbichler]

by https://lore.proxmox.com/pbs-devel/20251001111915.2001026-2-f.gruenbichler@proxmox.com/T/#u

On September 29, 2025 11:32 am, Fabian Grünbichler wrote:
> this Mutex is held for the duration of the whole garbage collection run, which
> now includes async calls into S3 code. this is not a highly contested lock, so
> switching to the more expensive tokio variant shouldn't cause noticeable
> overhead, and fix potential deadlock problems that could occur because of
> holding the lock guard across await points.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> also found while going through std::sync::Mutex usage with S3
> 
>  pbs-datastore/src/datastore.rs | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 7cf020fc0..68043e7a5 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -136,7 +136,7 @@ pub fn ensure_datastore_is_mounted(config: &DataStoreConfig) -> Result<(), Error
>  /// management interface for backup.
>  pub struct DataStoreImpl {
>      chunk_store: Arc<ChunkStore>,
> -    gc_mutex: Mutex<()>,
> +    gc_mutex: tokio::sync::Mutex<()>,
>      last_gc_status: Mutex<GarbageCollectionStatus>,
>      verify_new: bool,
>      chunk_order: ChunkOrder,
> @@ -152,7 +152,7 @@ impl DataStoreImpl {
>      pub(crate) unsafe fn new_test() -> Arc<Self> {
>          Arc::new(Self {
>              chunk_store: Arc::new(unsafe { ChunkStore::panic_store() }),
> -            gc_mutex: Mutex::new(()),
> +            gc_mutex: tokio::sync::Mutex::new(()),
>              last_gc_status: Mutex::new(GarbageCollectionStatus::default()),
>              verify_new: false,
>              chunk_order: Default::default(),
> @@ -513,7 +513,7 @@ impl DataStore {
>  
>          Ok(DataStoreImpl {
>              chunk_store,
> -            gc_mutex: Mutex::new(()),
> +            gc_mutex: tokio::sync::Mutex::new(()),
>              last_gc_status: Mutex::new(gc_status),
>              verify_new: config.verify_new.unwrap_or(false),
>              chunk_order: tuning.chunk_order.unwrap_or_default(),
> -- 
> 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