* [PATCH proxmox-backup 1/2] api: datastore: add option to run garbage collection before unmount
2026-04-17 11:27 [PATCH proxmox-backup 0/2] fixes #7465: add gc-on-unmount flag Hannes Laimer
@ 2026-04-17 11:27 ` Hannes Laimer
2026-04-17 14:35 ` Christian Ebner
2026-04-17 11:27 ` [PATCH proxmox-backup 2/2] ui: datastore: expose gc-on-unmount setting Hannes Laimer
2026-04-17 14:56 ` [PATCH proxmox-backup 0/2] fixes #7465: add gc-on-unmount flag Christian Ebner
2 siblings, 1 reply; 5+ messages in thread
From: Hannes Laimer @ 2026-04-17 11:27 UTC (permalink / raw)
To: pbs-devel
Removable datastores set up for auto-unmount have no natural point at
which to run garbage collection, since the drive is unmounted right
after jobs finish. Expose a gc-on-unmount option so GC can be triggered
as part of the unmount for those setups.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
src/api2/admin/datastore.rs | 33 +++++++++++++++++++++++++++------
src/api2/config/datastore.rs | 9 +++++++++
2 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 757b3114..5ce30c57 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -2641,15 +2641,36 @@ fn do_unmount_device(
}
async fn do_unmount(store: String, auth_id: Authid, to_stdout: bool) -> Result<Value, Error> {
- let _lock = pbs_config::datastore::lock_config()?;
- let (mut section_config, _digest) = pbs_config::datastore::config()?;
- let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
+ let gc_on_unmount = {
+ let _lock = pbs_config::datastore::lock_config()?;
+ let (section_config, _digest) = pbs_config::datastore::config()?;
+ let datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
- if datastore.backing_device.is_none() {
- bail!("datastore '{store}' is not removable");
+ if datastore.backing_device.is_none() {
+ bail!("datastore '{store}' is not removable");
+ }
+
+ ensure_datastore_is_mounted(&datastore)?;
+
+ datastore.gc_on_unmount.unwrap_or(false)
+ };
+
+ // Must happen outside the config lock, the GC handler acquires it too.
+ if gc_on_unmount {
+ let client = crate::client_helpers::connect_to_localhost()
+ .context("failed to connect to localhost for starting GC")?;
+ match client
+ .post(&format!("api2/json/admin/datastore/{store}/gc"), None)
+ .await
+ {
+ Ok(_) => info!("started garbage collection, unmount will wait for it to finish"),
+ Err(err) => warn!("unable to start garbage collection before unmount: {err}"),
+ }
}
- ensure_datastore_is_mounted(&datastore)?;
+ let _lock = pbs_config::datastore::lock_config()?;
+ let (mut section_config, _digest) = pbs_config::datastore::config()?;
+ let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
datastore.set_maintenance_mode(Some(MaintenanceMode {
ty: MaintenanceType::Unmount,
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index c44d50d1..c0be0296 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -409,6 +409,8 @@ pub enum DeletableProperty {
Comment,
/// Delete the garbage collection schedule.
GcSchedule,
+ /// Delete the gc-on-unmount property.
+ GcOnUnmount,
/// Delete the prune job schedule.
PruneSchedule,
/// Delete the keep-last property
@@ -495,6 +497,9 @@ pub fn update_datastore(
DeletableProperty::GcSchedule => {
data.gc_schedule = None;
}
+ DeletableProperty::GcOnUnmount => {
+ data.gc_on_unmount = None;
+ }
DeletableProperty::PruneSchedule => {
data.prune_schedule = None;
}
@@ -560,6 +565,10 @@ pub fn update_datastore(
data.gc_schedule = update.gc_schedule;
}
+ if update.gc_on_unmount.is_some() {
+ data.gc_on_unmount = update.gc_on_unmount;
+ }
+
macro_rules! prune_disabled {
($(($param:literal, $($member:tt)+)),+) => {
$(
--
2.47.3
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH proxmox-backup 1/2] api: datastore: add option to run garbage collection before unmount
2026-04-17 11:27 ` [PATCH proxmox-backup 1/2] api: datastore: add option to run garbage collection before unmount Hannes Laimer
@ 2026-04-17 14:35 ` Christian Ebner
0 siblings, 0 replies; 5+ messages in thread
From: Christian Ebner @ 2026-04-17 14:35 UTC (permalink / raw)
To: Hannes Laimer, pbs-devel
On 4/17/26 1:26 PM, Hannes Laimer wrote:
> Removable datastores set up for auto-unmount have no natural point at
> which to run garbage collection, since the drive is unmounted right
> after jobs finish. Expose a gc-on-unmount option so GC can be triggered
> as part of the unmount for those setups.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> src/api2/admin/datastore.rs | 33 +++++++++++++++++++++++++++------
> src/api2/config/datastore.rs | 9 +++++++++
> 2 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 757b3114..5ce30c57 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -2641,15 +2641,36 @@ fn do_unmount_device(
> }
>
> async fn do_unmount(store: String, auth_id: Authid, to_stdout: bool) -> Result<Value, Error> {
> - let _lock = pbs_config::datastore::lock_config()?;
> - let (mut section_config, _digest) = pbs_config::datastore::config()?;
> - let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
> + let gc_on_unmount = {
> + let _lock = pbs_config::datastore::lock_config()?;
question: Is the exclusive lock on the datastore config really required
here? After all this only reads the config and checks it's parameters.
> + let (section_config, _digest) = pbs_config::datastore::config()?;
> + let datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
>
> - if datastore.backing_device.is_none() {
> - bail!("datastore '{store}' is not removable");
> + if datastore.backing_device.is_none() {
> + bail!("datastore '{store}' is not removable");
> + }
> +
> + ensure_datastore_is_mounted(&datastore)?;
> +
> + datastore.gc_on_unmount.unwrap_or(false)
> + };
> +
> + // Must happen outside the config lock, the GC handler acquires it too.
> + if gc_on_unmount {
> + let client = crate::client_helpers::connect_to_localhost()
> + .context("failed to connect to localhost for starting GC")?;
> + match client
> + .post(&format!("api2/json/admin/datastore/{store}/gc"), None)
question: What about permissions here? Running garbage collection
normally requires Datastore.Modify, but since a datastore config edit
requires Datastore.Modify or creation via Datastore.Allocate I guess
this is okay. Might however warrant a comment.
> + .await
> + {
> + Ok(_) => info!("started garbage collection, unmount will wait for it to finish"),
comment: Why this works should be outlined in the commit message IMO.
Maybe something along the lines of:
```
Relies on the active write operation by garbage collection to block the
unmount task until GC completes (among possible other active
operations). Further, no other active operation can occur in the mean
time, since the datastore remains in maintenance mode `unmount`.
```
> + Err(err) => warn!("unable to start garbage collection before unmount: {err}"),
> + }
> }
>
> - ensure_datastore_is_mounted(&datastore)?;
> + let _lock = pbs_config::datastore::lock_config()?;
> + let (mut section_config, _digest) = pbs_config::datastore::config()?;
> + let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
>
> datastore.set_maintenance_mode(Some(MaintenanceMode {
> ty: MaintenanceType::Unmount,
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index c44d50d1..c0be0296 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -409,6 +409,8 @@ pub enum DeletableProperty {
> Comment,
> /// Delete the garbage collection schedule.
> GcSchedule,
> + /// Delete the gc-on-unmount property.
> + GcOnUnmount,
> /// Delete the prune job schedule.
> PruneSchedule,
> /// Delete the keep-last property
> @@ -495,6 +497,9 @@ pub fn update_datastore(
> DeletableProperty::GcSchedule => {
> data.gc_schedule = None;
> }
> + DeletableProperty::GcOnUnmount => {
> + data.gc_on_unmount = None;
> + }
> DeletableProperty::PruneSchedule => {
> data.prune_schedule = None;
> }
> @@ -560,6 +565,10 @@ pub fn update_datastore(
> data.gc_schedule = update.gc_schedule;
> }
>
> + if update.gc_on_unmount.is_some() {
> + data.gc_on_unmount = update.gc_on_unmount;
> + }
> +
> macro_rules! prune_disabled {
> ($(($param:literal, $($member:tt)+)),+) => {
> $(
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH proxmox-backup 2/2] ui: datastore: expose gc-on-unmount setting
2026-04-17 11:27 [PATCH proxmox-backup 0/2] fixes #7465: add gc-on-unmount flag Hannes Laimer
2026-04-17 11:27 ` [PATCH proxmox-backup 1/2] api: datastore: add option to run garbage collection before unmount Hannes Laimer
@ 2026-04-17 11:27 ` Hannes Laimer
2026-04-17 14:56 ` [PATCH proxmox-backup 0/2] fixes #7465: add gc-on-unmount flag Christian Ebner
2 siblings, 0 replies; 5+ messages in thread
From: Hannes Laimer @ 2026-04-17 11:27 UTC (permalink / raw)
To: pbs-devel
Shows the gc-on-unmount option in the datastore options and allows
editing for removable datastores.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
www/datastore/OptionView.js | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
index 160edd44..2b35355a 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -104,6 +104,7 @@ Ext.define('PBS.Datastore.Options', {
let record = listStore.findRecord('store', me.datastore, 0, false, true, true);
return {
s3Visible: record?.get('backend-type') === 's3' ? undefined : false,
+ removableVisible: record?.get('mount-status') !== 'nonremovable' ? undefined : false,
};
},
@@ -226,6 +227,28 @@ Ext.define('PBS.Datastore.Options', {
},
},
},
+ 'gc-on-unmount': {
+ required: true,
+ header: gettext('GC on Unmount'),
+ defaultValue: false,
+ renderer: Proxmox.Utils.format_boolean,
+ cbind: {
+ visible: '{removableVisible}',
+ },
+ editor: {
+ xtype: 'proxmoxWindowEdit',
+ title: gettext('GC on Unmount'),
+ width: 350,
+ items: {
+ xtype: 'proxmoxcheckbox',
+ name: 'gc-on-unmount',
+ boxLabel: gettext('Run garbage collection before unmounting'),
+ defaultValue: false,
+ deleteDefaultValue: true,
+ deleteEmpty: true,
+ },
+ },
+ },
'maintenance-mode': {
required: true,
header: gettext('Maintenance mode'),
--
2.47.3
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH proxmox-backup 0/2] fixes #7465: add gc-on-unmount flag
2026-04-17 11:27 [PATCH proxmox-backup 0/2] fixes #7465: add gc-on-unmount flag Hannes Laimer
2026-04-17 11:27 ` [PATCH proxmox-backup 1/2] api: datastore: add option to run garbage collection before unmount Hannes Laimer
2026-04-17 11:27 ` [PATCH proxmox-backup 2/2] ui: datastore: expose gc-on-unmount setting Hannes Laimer
@ 2026-04-17 14:56 ` Christian Ebner
2 siblings, 0 replies; 5+ messages in thread
From: Christian Ebner @ 2026-04-17 14:56 UTC (permalink / raw)
To: Hannes Laimer, pbs-devel
On 4/17/26 1:26 PM, Hannes Laimer wrote:
> So GC can run before unmount, without this its somewhat complicated to
> run gc on a datastore that is configured to auto unmount after jobs
> finish.
>
>
> Hannes Laimer (2):
> api: datastore: add option to run garbage collection before unmount
> ui: datastore: expose gc-on-unmount setting
>
> src/api2/admin/datastore.rs | 33 +++++++++++++++++++++++++++------
> src/api2/config/datastore.rs | 9 +++++++++
> www/datastore/OptionView.js | 23 +++++++++++++++++++++++
> 3 files changed, 59 insertions(+), 6 deletions(-)
>
Looks good apart from a few smaller comments and questions.
I'm missing the patch for pbs-api-types though? (Adapted myself for
review since it is a trivial adaption).
^ permalink raw reply [flat|nested] 5+ messages in thread