Re: [PATCH proxmox-backup v3 07/10] datastore: create lockdir with correct mode for backup user access

[Robert Obkircher <r.obkircher@proxmox.com>]

> Instead of locking the whole config, which causes also unrelated
> datastores to not be available, use a per-datastore maintenance-mode
> lock to protect against changes during potentially longer running
> operations such as s3-refresh or removable datastore mount/unmount.
> 
> However keep the current semantic of first setting the maintenance
> mode, allow to still opt out from the mode while waiting for active
> operations to finish, and only then enforce the maintenance mode
> during the s3 refresh or removable datastore mount operation.
> 
> unset_maintenance() is adapted to also check for the expected
> maintenance type and acquire the maintenance-mode lock if not provided
> by the caller.
> 
> Fixes: https://forum.proxmox.com/threads/183244/
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 99adfde8a..3550f0963 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -3162,6 +3162,7 @@ impl DataStore {
>          let (mut config, _digest) = pbs_config::datastore::config()?;
>          let mut datastore_config: DataStoreConfig = config.lookup("datastore", name)?;
>  
> +        let maintenance_mode_lock = crate::maintenance_mode_lock(name, &config_lock)?;
>          datastore_config.set_maintenance_mode(Some(MaintenanceMode {
>              ty: MaintenanceType::Delete,
>              message: None,
> @@ -3169,6 +3170,7 @@ impl DataStore {
>  
>          config.set_data(name, "datastore", &datastore_config)?;
>          pbs_config::datastore::save_config(&config)?;
> +        drop(maintenance_mode_lock);
>          drop(config_lock);
>  
>          let (operations, _lock) = task_tracking::get_active_operations_locked(name)?;
> diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
> index 187de23f6..99f954f19 100644
> --- a/pbs-datastore/src/lib.rs
> +++ b/pbs-datastore/src/lib.rs
> @@ -159,8 +159,9 @@
>  
>  use std::os::unix::io::AsRawFd;
>  use std::path::Path;
> +use std::time::Duration;
>  
> -use anyhow::{Error, bail};
> +use anyhow::{Context, Error, bail};
>  
>  use proxmox_sys::fs::CreateOptions;
>  
> @@ -282,3 +283,21 @@ where
>  
>      Ok(lock)
>  }
> +
> +/// Acquire an exclusive lock for the datastore's maintenance-mode. The datastore config lock
> +/// must be acquired beforehand to avoid races.
> +pub fn maintenance_mode_lock(
> +    store: &str,
> +    _datastore_config_lock: &BackupLockGuard,
> +) -> Result<BackupLockGuard, Error> {
> +    let mut lock_path = Path::new(DATASTORE_LOCKS_DIR).join(store);
> +    lock_path.push("maintenance-mode.lck");
> +
> +    lock_helper(store, &lock_path, |p| {
> +        // never wait for the lock here: this is only acquired by long running operations
> +        // and the datastore config lock must be held anyways. Blocking the it longer than
> +        // necessary is not acceptable.
> +        pbs_config::open_backup_lockfile(p, Some(Duration::from_secs(0)), true)
> +            .context("unable to acquire exclusive datastore's maintenance-mode lock")
> +    })
> +}
Since those locks are used together it would be nicer to turn this into
a MaintenanceModeLock(BackupLockGuard) wrapper for some additional type
safety.

-- 
Robert Obkircher <r.obkircher@proxmox.com>