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

[Hannes Laimer <h.laimer@proxmox.com>]

On 2026-04-20 10:31, Shannon Sterz wrote:
> 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?
> 

the reason is that the unmounting is running in the api process, so as
root. if we don't go through the api we would have the gc also running
in the privileged process. general datastore operations, like gc, do
assume they run as the `backup` user

hitting the (proxy) api endpoint is the simplest way to have the gc run
with the correct permissions

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