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

["Shannon Sterz" <s.sterz@proxmox.com>]

On Mon Apr 20, 2026 at 9:42 AM CEST, 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.
>
> Relies on the active write operation by garbage collection to block
> the unmount task until GC completes (among possible other active
> operations). No other active operation can occur in the meantime
> since the datastore remains in 'unmount' maintenance mode.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/api2/admin/datastore.rs  | 23 ++++++++++++++++++++---
>  src/api2/config/datastore.rs |  9 +++++++++
>  2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 757b3114..212a48da 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -2641,9 +2641,8 @@ 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 (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");
> @@ -2651,6 +2650,24 @@ async fn do_unmount(store: String, auth_id: Authid, to_stdout: bool) -> Result<V
>
>      ensure_datastore_is_mounted(&datastore)?;
>
> +    // Setting gc-on-unmount requires Datastore.Modify (or Datastore.Allocate at creation), the
> +    // same level needed to start GC directly, so no privilege escalation from triggering it here.
> +    if datastore.gc_on_unmount.unwrap_or(false) {
> +        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}"),

small question, any reason to do a round trip across the api here
instead of factoring out the logic needed here from the
`start_garbage_collection` function below and calling that directly?
something like this should to the trick:

```

fn init_garbage_collection_job(
    store: String,
    auth_id: &Authid,
    to_stdout: bool,
) -> Result<Value, Error> {
    let datastore = DataStore::lookup_datastore(lookup_with(&store, Operation::Write))?;

    let job = Job::new("garbage_collection", &store)
        .map_err(|_| format_err!("garbage collection already running"))?;

    let upid_str =
        crate::server::do_garbage_collection_job(job, datastore, &auth_id, None, to_stdout)
            .map_err(|err| {
                format_err!("unable to start garbage collection job on datastore {store} - {err:#}")
            })?;

    Ok(json!(upid_str))
}

you can then call that in `do_unmount` and `start_garbage_collection`.
or am i missing something? would also associate the gc task with user
starting the unmount operation instead of root@pam if im not mistaken?

> +        }
> +    }
> +
> +    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,
>          message: None,
> 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)+)),+) => {
>              $(