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

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

On 2026-04-17 16:33, Christian Ebner wrote:
> 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.
> 

not really, will drop for v2

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