* [pbs-devel] [PATCH proxmox-backup] garbage-collect: switch to tokio Mutex
@ 2025-09-29 9:32 Fabian Grünbichler
2025-09-29 16:07 ` Christian Ebner
2025-10-01 11:20 ` [pbs-devel] superseded: " Fabian Grünbichler
0 siblings, 2 replies; 4+ messages in thread
From: Fabian Grünbichler @ 2025-09-29 9:32 UTC (permalink / raw)
To: pbs-devel; +Cc: w.bumiller
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] garbage-collect: switch to tokio Mutex
2025-09-29 9:32 [pbs-devel] [PATCH proxmox-backup] garbage-collect: switch to tokio Mutex Fabian Grünbichler
@ 2025-09-29 16:07 ` Christian Ebner
2025-09-30 6:24 ` Fabian Grünbichler
2025-10-01 11:20 ` [pbs-devel] superseded: " Fabian Grünbichler
1 sibling, 1 reply; 4+ messages in thread
From: Christian Ebner @ 2025-09-29 16:07 UTC (permalink / raw)
To: Fabian Grünbichler, pbs-devel; +Cc: w.bumiller
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] garbage-collect: switch to tokio Mutex
2025-09-29 16:07 ` Christian Ebner
@ 2025-09-30 6:24 ` Fabian Grünbichler
0 siblings, 0 replies; 4+ messages in thread
From: Fabian Grünbichler @ 2025-09-30 6:24 UTC (permalink / raw)
To: Christian Ebner, pbs-devel; +Cc: w.bumiller
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [pbs-devel] superseded: [PATCH proxmox-backup] garbage-collect: switch to tokio Mutex
2025-09-29 9:32 [pbs-devel] [PATCH proxmox-backup] garbage-collect: switch to tokio Mutex Fabian Grünbichler
2025-09-29 16:07 ` Christian Ebner
@ 2025-10-01 11:20 ` Fabian Grünbichler
1 sibling, 0 replies; 4+ messages in thread
From: Fabian Grünbichler @ 2025-10-01 11:20 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-10-01 11:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-09-29 9:32 [pbs-devel] [PATCH proxmox-backup] garbage-collect: switch to tokio Mutex Fabian Grünbichler
2025-09-29 16:07 ` Christian Ebner
2025-09-30 6:24 ` Fabian Grünbichler
2025-10-01 11:20 ` [pbs-devel] superseded: " Fabian Grünbichler
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.