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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On November 13, 2025 9:43 am, Christian Ebner wrote:
> 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.

we could skip the status callback and just make the status line generic
enough, it is almost identical anyway, and if we want to make it more
clear we can print a line ("Starting .." or whatever?) before calling
the helper?


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