Re: [PATCH proxmox-backup 1/2] api: datastore: add option to run garbage collection before unmount

[Christian Ebner <c.ebner@proxmox.com>]

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)+)),+) => {
>               $(