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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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