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 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?

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