[PATCH proxmox-backup v4 14/14] datastore: protect datastore creation by maintenance-mode

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

Instead of holding the lock for the whole datastore creation, use the
maintenance-mode `create`, protected by the maintenance-mode lock.
It is inserted and written to the config when setting the maintenance
mode before even starting any datastore related operation. Cleanup
from config if creation failed must be performed manually.

This has the advantage that a potentially long running creation does
not block any concurrent access to the datastore config, thereby
blocking also any unrelated datastore.

The config has to be re-parsed a second time for this, but given that
this unblocks the config and this is no performance critical path
that tradeoff is better than its alternative. By refactoring the
helpers to set and unset the maintenance mode, they can be reused for
all call sites.

Rely on re-acquisition of the lock to also succeed if other operations
are being performed, due to the 10s timeout.

Ordering of locking cannot always be preserved, but deadlocks are
excluded by timeouts.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs   |  8 +---
 src/api2/admin/datastore.rs      | 41 ++---------------
 src/api2/config/datastore.rs     | 35 +++++++-------
 src/api2/helpers.rs              | 78 +++++++++++++++++++++++++++++++-
 src/api2/node/disks/directory.rs |  1 -
 src/api2/node/disks/zfs.rs       |  1 -
 6 files changed, 101 insertions(+), 63 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 9e0d9586c..b74b72f76 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -481,7 +481,7 @@ impl DataStore {
 
     /// Create or reuse the chunk store from given datastore configuration
     pub fn construct_chunk_store(
-        store_config: &mut DataStoreConfig,
+        store_config: &DataStoreConfig,
         reuse_existing: bool,
         overwrite_in_use: bool,
     ) -> Result<(), Error> {
@@ -552,12 +552,6 @@ impl DataStore {
                         });
                     }
                 }
-                // starting out in maintenance mode s3-refresh,
-                // so no other operation will start until done with that.
-                store_config.set_maintenance_mode(Some(MaintenanceMode {
-                    ty: MaintenanceType::S3Refresh,
-                    message: None,
-                }))?;
             }
             let backup_user = proxmox_product_config::get_api_user();
             ChunkStore::create(
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 82ef21b1b..19eab6223 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -61,13 +61,14 @@ use pbs_datastore::index::IndexFile;
 use pbs_datastore::manifest::BackupManifest;
 use pbs_datastore::prune::compute_prune_info;
 use pbs_datastore::{
-    BackupDir, DataStore, LocalChunkReader, MaintenanceModeLock, StoreProgress, check_backup_owner,
+    BackupDir, DataStore, LocalChunkReader, StoreProgress, check_backup_owner,
     ensure_datastore_is_mounted, maintenance_mode_lock, task_tracking,
 };
 use pbs_tools::json::required_string_param;
 use proxmox_rest_server::{WorkerTask, formatter, worker_is_active};
 
 use crate::api2::backup::optional_ns_param;
+use crate::api2::helpers::{expect_maintenance_type, unset_maintenance_mode};
 use crate::api2::node::rrd::create_value_from_rrd;
 use crate::backup::{ListAccessibleBackupGroups, NS_PRIVS_OK, VerifyWorker, check_ns_privs_full};
 use crate::server::jobstate::{Job, JobState, compute_schedule_status};
@@ -2694,40 +2695,6 @@ pub fn mount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Er
     Ok(json!(upid))
 }
 
-fn expect_maintenance_type(store: &str, maintenance_type: MaintenanceType) -> Result<(), Error> {
-    let (section_config, _digest) = pbs_config::datastore::config()?;
-    let store_config: DataStoreConfig = section_config.lookup("datastore", store)?;
-
-    if store_config
-        .get_maintenance_mode()
-        .is_none_or(|m| m.ty != maintenance_type)
-    {
-        bail!("maintenance mode is not '{maintenance_type}'");
-    }
-
-    Ok(())
-}
-
-fn unset_maintenance(
-    maintenance_mode_lock: Option<MaintenanceModeLock>,
-    store: &str,
-    expected: MaintenanceType,
-) -> Result<(), Error> {
-    let lock = pbs_config::datastore::lock_config()?;
-    let _maintenance_mode_lock = match maintenance_mode_lock {
-        Some(lock) => lock,
-        None => pbs_datastore::maintenance_mode_lock(store, &lock)?,
-    };
-    expect_maintenance_type(store, expected)?;
-    let (mut section_config, _digest) = pbs_config::datastore::config()?;
-
-    let mut store_config: DataStoreConfig = section_config.lookup("datastore", store)?;
-    store_config.set_maintenance_mode(None)?;
-    section_config.set_data(store, "datastore", &store_config)?;
-    pbs_config::datastore::save_config(&section_config)?;
-    Ok(())
-}
-
 fn do_unmount_device(
     datastore: DataStoreConfig,
     worker: &dyn WorkerTaskContext,
@@ -2923,7 +2890,7 @@ fn run_maintenance_locked(
     }
 
     if aborted || worker.abort_requested() {
-        let _ = unset_maintenance(None, store, maintenance_expected)
+        let _ = unset_maintenance_mode(None, store, maintenance_expected)
             .inspect_err(|e| warn!("could not reset maintenance mode: {e}"));
         bail!("aborted, due to user request");
     }
@@ -2936,7 +2903,7 @@ fn run_maintenance_locked(
     drop(lock);
 
     callback()?;
-    unset_maintenance(Some(maintenance_mode_lock), store, maintenance_expected)
+    unset_maintenance_mode(Some(maintenance_mode_lock), store, maintenance_expected)
         .map_err(|e| format_err!("could not reset maintenance mode: {e}"))?;
 
     Ok(())
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 5d0aa966a..57d256df4 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -7,17 +7,15 @@ use tracing::warn;
 
 use proxmox_router::{Permission, Router, RpcEnvironment, RpcEnvironmentType, http_bail};
 use proxmox_schema::{ApiType, api, param_bail};
-use proxmox_section_config::SectionConfigData;
 use proxmox_uuid::Uuid;
 use proxmox_worker_task::WorkerTaskContext;
 
 use pbs_api_types::{
     Authid, DATASTORE_SCHEMA, DataStoreConfig, DataStoreConfigUpdater, DatastoreBackendType,
-    DatastoreNotify, KeepOptions, MaintenanceMode, PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT,
-    PRIV_DATASTORE_MODIFY, PRIV_SYS_MODIFY, PROXMOX_CONFIG_DIGEST_SCHEMA, PruneJobConfig,
-    PruneJobOptions, UPID_SCHEMA,
+    DatastoreNotify, KeepOptions, MaintenanceMode, MaintenanceType, PRIV_DATASTORE_ALLOCATE,
+    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY, PRIV_SYS_MODIFY, PROXMOX_CONFIG_DIGEST_SCHEMA,
+    PruneJobConfig, PruneJobOptions, UPID_SCHEMA,
 };
-use pbs_config::BackupLockGuard;
 
 use crate::api2::admin::datastore::do_mount_device;
 use crate::api2::admin::prune::list_prune_jobs;
@@ -27,11 +25,12 @@ use crate::api2::config::prune::{delete_prune_job, do_create_prune_job, has_prun
 use crate::api2::config::sync::delete_sync_job;
 use crate::api2::config::tape_backup_job::{delete_tape_backup_job, list_tape_backup_jobs};
 use crate::api2::config::verify::delete_verification_job;
-use pbs_config::CachedUserInfo;
+use pbs_config::{BackupLockGuard, CachedUserInfo};
 
 use pbs_datastore::{DataStore, get_datastore_mount_status, maintenance_mode_lock};
 use proxmox_rest_server::WorkerTask;
 
+use crate::api2::helpers::{set_maintenance_type, unset_maintenance_mode};
 use crate::server::jobstate;
 use crate::tools::disks::unmount_by_mountpoint;
 
@@ -95,7 +94,6 @@ impl Drop for UnmountGuard {
 
 pub(crate) fn do_create_datastore(
     lock: BackupLockGuard,
-    mut config: SectionConfigData,
     worker: &dyn WorkerTaskContext,
     backend_type: DatastoreBackendType,
     mut datastore: DataStoreConfig,
@@ -103,7 +101,11 @@ pub(crate) fn do_create_datastore(
     reuse_datastore: bool,
     overwrite_in_use: bool,
 ) -> Result<(), Error> {
-    pbs_config::datastore::validate_new_datastore_config(&datastore, &config, &lock)?;
+    // always start out in maintenance-mode create, so access can be blocked during creation
+    // without needing to hold the config lock, only the maintenance-mode lock.
+    let maintenance_mode_lock = pbs_datastore::maintenance_mode_lock(&datastore.name, &lock)?;
+    let mut current_type = MaintenanceType::Create;
+    set_maintenance_type(lock, &maintenance_mode_lock, &mut datastore, current_type)?;
 
     let unmount_guard = if datastore.backing_device.is_some() {
         do_mount_device(datastore.clone())?;
@@ -112,26 +114,28 @@ pub(crate) fn do_create_datastore(
         UnmountGuard::new(None)
     };
 
-    DataStore::construct_chunk_store(&mut datastore, reuse_datastore, overwrite_in_use)?;
-
-    config.set_data(&datastore.name, "datastore", &datastore)?;
-
-    pbs_config::datastore::save_config(&config)?;
+    DataStore::construct_chunk_store(&datastore, reuse_datastore, overwrite_in_use)?;
 
     jobstate::create_state_file("garbage_collection", &datastore.name)?;
 
     unmount_guard.disable();
 
-    drop(lock);
-
     if let Some(prune_job_config) = prune_job_config {
         do_create_prune_job(prune_job_config)?;
     }
 
     if reuse_datastore && backend_type == DatastoreBackendType::S3 {
+        // update maintenance mode to s3-refresh, so can drop config lock again and no other
+        // operation will start until done with the refresh.
+        let lock = pbs_config::datastore::lock_config()?;
+        current_type = MaintenanceType::S3Refresh;
+        set_maintenance_type(lock, &maintenance_mode_lock, &mut datastore, current_type)?;
+
         crate::api2::admin::datastore::do_s3_refresh(&datastore.name, worker)?;
     }
 
+    unset_maintenance_mode(Some(maintenance_mode_lock), &datastore.name, current_type)?;
+
     Ok(())
 }
 
@@ -227,7 +231,6 @@ pub fn create_datastore(
         move |worker| {
             do_create_datastore(
                 lock,
-                section_config,
                 &worker,
                 backend,
                 config,
diff --git a/src/api2/helpers.rs b/src/api2/helpers.rs
index aa582a53c..6977656fb 100644
--- a/src/api2/helpers.rs
+++ b/src/api2/helpers.rs
@@ -1,12 +1,16 @@
 use std::path::PathBuf;
 
-use anyhow::Error;
+use anyhow::{Error, bail};
 use futures::stream::TryStreamExt;
 use hyper::{Response, StatusCode, header};
 
+use pbs_api_types::{DataStoreConfig, MaintenanceMode, MaintenanceType};
+use pbs_datastore::MaintenanceModeLock;
 use proxmox_http::Body;
 use proxmox_router::http_bail;
 
+use pbs_config::BackupLockGuard;
+
 pub async fn create_download_response(path: PathBuf) -> Result<Response<Body>, Error> {
     let file = match tokio::fs::File::open(path.clone()).await {
         Ok(file) => file,
@@ -28,3 +32,75 @@ pub async fn create_download_response(path: PathBuf) -> Result<Response<Body>, E
         .body(body)
         .unwrap())
 }
+
+/// Check if the current datastore config reflects the expected maintenance type.
+///
+/// Loads the config from file without locking.
+pub(super) fn expect_maintenance_type(
+    store: &str,
+    maintenance_type: MaintenanceType,
+) -> Result<(), Error> {
+    let (section_config, _digest) = pbs_config::datastore::config()?;
+    let store_config: DataStoreConfig = section_config.lookup("datastore", store)?;
+
+    if store_config
+        .get_maintenance_mode()
+        .is_none_or(|m| m.ty != maintenance_type)
+    {
+        bail!("maintenance mode is not '{maintenance_type}'");
+    }
+
+    Ok(())
+}
+
+/// Sets and stores the maintenance mode for give store config.
+///
+/// Requires and consumes thereby dropping the datastore config lock.
+/// Requires but does not consume the maintenance-mode lock.
+pub(super) fn set_maintenance_type(
+    _datastore_config_lock: BackupLockGuard,
+    _maintenance_mode_lock: &MaintenanceModeLock,
+    config: &mut DataStoreConfig,
+    maintenance_type: MaintenanceType,
+) -> Result<(), Error> {
+    let (mut section_config, _digest) = pbs_config::datastore::config()?;
+
+    config.set_maintenance_mode(Some(MaintenanceMode {
+        ty: maintenance_type,
+        message: None,
+    }))?;
+
+    section_config.set_data(&config.name, "datastore", &config)?;
+    pbs_config::datastore::save_config(&section_config)
+}
+
+/// Unsets the maintenance-mode and updates the config for give datastore.
+///
+/// Either acquires or consumes the provided maintenance-mode lock,
+/// but always acquires the config lock.
+pub(super) fn unset_maintenance_mode(
+    maintenance_mode_lock: Option<MaintenanceModeLock>,
+    store: &str,
+    expected: MaintenanceType,
+) -> Result<(), Error> {
+    // It is fine to acquire the lock here in reverse order if passed in via the caller,
+    // as the config lock must never be held for long.
+    let lock = pbs_config::datastore::lock_config()?;
+
+    let _maintenance_mode_lock = match maintenance_mode_lock {
+        Some(lock) => lock,
+        None => pbs_datastore::maintenance_mode_lock(store, &lock)?,
+    };
+
+    expect_maintenance_type(store, expected)?;
+
+    let (mut section_config, _digest) = pbs_config::datastore::config()?;
+    let mut store_config: DataStoreConfig = section_config.lookup("datastore", store)?;
+
+    store_config.set_maintenance_mode(None)?;
+
+    section_config.set_data(store, "datastore", &store_config)?;
+    pbs_config::datastore::save_config(&section_config)?;
+
+    Ok(())
+}
diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 7fc7187bf..61e4f9df6 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -252,7 +252,6 @@ pub fn create_datastore_disk(
 
                 crate::api2::config::datastore::do_create_datastore(
                     lock,
-                    config,
                     &worker,
                     DatastoreBackendType::Filesystem,
                     datastore,
diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
index 2a44c3d2d..b5a0dbfe6 100644
--- a/src/api2/node/disks/zfs.rs
+++ b/src/api2/node/disks/zfs.rs
@@ -312,7 +312,6 @@ pub fn create_zpool(
 
                 crate::api2::config::datastore::do_create_datastore(
                     lock,
-                    config,
                     &worker,
                     DatastoreBackendType::Filesystem,
                     datastore,
-- 
2.47.3