Re: [pbs-devel] [PATCH proxmox-backup v2 4/6] api: admin: factor out locking and maintenance mode clearing

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

On 11/13/25 9:18 AM, Fabian Grünbichler wrote:
> On November 12, 2025 5:36 pm, Christian Ebner wrote:
>> Provide a helper which allows to either clear the maintenance mode if
>> the worker was aborted, or call the provided callback while holding
>> the datastore config lock.
>>
>> In preparation for reusing the same logic for the s3 refresh.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>> changes since version 1:
>> - not present in previous version
>>
>>   src/api2/admin/datastore.rs | 50 +++++++++++++++++++++++++------------
>>   1 file changed, 34 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> index 7daccf9fd..8d58b5059 100644
>> --- a/src/api2/admin/datastore.rs
>> +++ b/src/api2/admin/datastore.rs
>> @@ -2584,7 +2584,6 @@ fn do_unmount_device(
>>       if datastore.backing_device.is_none() {
>>           bail!("can't unmount non-removable datastore");
>>       }
>> -    let mount_point = datastore.absolute_path();
>>   
>>       let mut old_status = String::new();
>>       let aborted = wait_on_active_operations(
>> @@ -2602,21 +2601,14 @@ fn do_unmount_device(
>>           },
>>       )?;
>>   
>> -    if aborted || worker.is_some_and(|w| w.abort_requested()) {
>> -        let _ = expect_maintenance_type(&datastore.name, MaintenanceType::Unmount)
>> -            .inspect_err(|e| warn!("maintenance mode was not as expected: {e}"))
>> -            .and_then(|(lock, config)| {
>> -                unset_maintenance(lock, config)
>> -                    .inspect_err(|e| warn!("could not reset maintenance mode: {e}"))
>> -            });
>> -        bail!("aborted, due to user request");
>> -    } else {
>> -        let (lock, config) = expect_maintenance_type(&datastore.name, MaintenanceType::Unmount)?;
>> -        crate::tools::disks::unmount_by_mountpoint(Path::new(&mount_point))?;
>> -        unset_maintenance(lock, config)
>> -            .map_err(|e| format_err!("could not reset maintenance mode: {e}"))?;
>> -    }
>> -    Ok(())
>> +    let mount_point = datastore.absolute_path();
>> +    clear_or_run_maintenance_locked(
>> +        &datastore.name,
>> +        worker,
>> +        MaintenanceType::Unmount,
>> +        aborted,
>> +        || crate::tools::disks::unmount_by_mountpoint(Path::new(&mount_point)),
>> +    )
>>   }
>>   
>>   #[api(
>> @@ -2747,6 +2739,32 @@ fn wait_on_active_operations(
>>       Ok(false)
>>   }
>>   
>> +// Either clear the current maintenance mode if the worker was aborted or run the provided callback
>> +// while keeping the datastore config lock, so the mode cannot be altered. Clears the maintenance
>> +// mode after successful callback execution.
>> +fn clear_or_run_maintenance_locked(
>> +    store: &str,
>> +    worker: Option<&dyn WorkerTaskContext>,
> 
> this can also drop the Option ;)
> 
> but given that we now have two helpers with two almost identical call
> sites, could we not make it a single helper?

Okay, can combine them into one. For me it was mentally less friction to 
have these separated, as especially given that the waiting on active 
operations seemed worth it's own encapsulation and gets a dedicated 
callback. Now this requires either 2 callbacks or a format string and 
callback.

But since this is not required elsewhere (yet?) I will combine this 
helper into one.

> e.g., `run_maintenance_locked` that
> - waits for operations to finish while checking worker status and
>    maintenance type (with status updates via a passed in format string or
>    callback)
> - obtains the lock
> - runs the actual maintenance task via a provided closure/callback/..
> - clears the maintenance mode and drops the lock
> 
>> +    maintenance_expected: MaintenanceType,
>> +    aborted: bool,
>> +    callback: impl Fn() -> Result<(), Error>,
>> +) -> Result<(), Error> {
>> +    if aborted || worker.is_some_and(|w| w.abort_requested()) {
>> +        let _ = expect_maintenance_type(store, maintenance_expected)
>> +            .inspect_err(|e| warn!("maintenance mode was not as expected: {e}"))
>> +            .and_then(|(lock, config)| {
>> +                unset_maintenance(lock, config)
>> +                    .inspect_err(|e| warn!("could not reset maintenance mode: {e}"))
>> +            });
>> +        bail!("aborted, due to user request");
>> +    } else {
>> +        let (lock, config) = expect_maintenance_type(store, maintenance_expected)?;
>> +        callback()?;
>> +        unset_maintenance(lock, config)
>> +            .map_err(|e| format_err!("could not reset maintenance mode: {e}"))
>> +    }
>> +}
>> +
>>   #[sortable]
>>   const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>>       (
>> -- 
>> 2.47.3
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel