[pbs-devel] [PATCH proxmox-backup v3 2/7] api: admin: rework waiting on active operations and maintenance locking

[Christian Ebner]

Move the logic to wait on no more active operations on a given
datastore and locking the datastore config into a dedicated helper
function.

While this is currently only used by the device unmount for removable
datastores, the same logic will be reused by the s3-refresh. The
helper gets a callback to be executed in locked context so the
maintenance mode cannot be altered and the status message on busy
waiting is adapted to be generic to fit both cases.

Instead of e.g.:

`cannot unmount yet, still 1 read and 1 write operations active`

the status line now states

`waiting for active operations to finsish before continuing: read 1, write 1`

Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
---
changes since version 1:
- combine previously split helpers into one

 src/api2/admin/datastore.rs | 90 +++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 38 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 935933bc5..0e81533ce 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -2584,45 +2584,11 @@ 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 active_operations = task_tracking::get_active_operations(&datastore.name)?;
-    let mut old_status = String::new();
-    let mut aborted = false;
-    while active_operations.read + active_operations.write > 0 {
-        if worker.abort_requested()
-            || expect_maintenance_type(&datastore.name, MaintenanceType::Unmount).is_err()
-        {
-            aborted = true;
-            break;
-        }
-        let status = format!(
-            "cannot unmount yet, still {} read and {} write operations active",
-            active_operations.read, active_operations.write
-        );
-        if status != old_status {
-            info!("{status}");
-            old_status = status;
-        }
-        std::thread::sleep(std::time::Duration::from_secs(1));
-        active_operations = task_tracking::get_active_operations(&datastore.name)?;
-    }
-
-    if aborted || worker.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();
+    run_maintenance_locked(&datastore.name, MaintenanceType::Unmount, worker, || {
+        crate::tools::disks::unmount_by_mountpoint(Path::new(&mount_point))
+    })
 }
 
 #[api(
@@ -2723,6 +2689,54 @@ pub async fn s3_refresh(store: String, rpcenv: &mut dyn RpcEnvironment) -> Resul
     Ok(json!(upid))
 }
 
+/// Wait for no more active operations on the given datastore and run the provided callback in
+/// a datastore locked context, protecting against maintenance mode changes.
+fn run_maintenance_locked(
+    store: &str,
+    maintenance_expected: MaintenanceType,
+    worker: &dyn WorkerTaskContext,
+    callback: impl Fn() -> Result<(), Error>,
+) -> Result<(), Error> {
+    let mut active_operations = task_tracking::get_active_operations(store)?;
+    let mut old_status = String::new();
+    let mut aborted = false;
+
+    while active_operations.read + active_operations.write > 0 {
+        if worker.abort_requested() || expect_maintenance_type(store, maintenance_expected).is_err()
+        {
+            aborted = true;
+            break;
+        }
+        let status = format!(
+            "waiting for active operations to finsish before continuing: read {}, write {}",
+            active_operations.read, active_operations.write,
+        );
+        if status != old_status {
+            info!("{status}");
+            old_status = status;
+        }
+        std::thread::sleep(std::time::Duration::from_secs(1));
+        active_operations = task_tracking::get_active_operations(store)?;
+    }
+
+    if aborted || worker.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");
+    }
+
+    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}"))?;
+
+    Ok(())
+}
+
 #[sortable]
 const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
     (
-- 
2.47.3