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 10:08 AM, Fabian Grünbichler wrote:
> 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?

Yeah, let's get rid of the extra parameter and make the status output 
generic.


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