[PATCH proxmox{,-backup} v2 00/10] keep datastore config unlock during long running operations

[PATCH proxmox{,-backup} v2 00/10] keep datastore config unlock during long running operations

[Christian Ebner]

Currently an s3-refresh for datastores backed by s3, mount and
unmount for removalbe datastores as well as datastore creation
(independent of datastore type) run while holding the datastore
config lock for consistency.
However, this also blocks config access by all unrelated datastores.

Instead, implement per-datastore locks for maintenance-mode changes,
and rely on block unexpected maintenance-mode changes during ongoing
operations. This allows to concurrently create datastores, run s3
refresh and perform mount/unmount without blocking config access longer
than necessary.

Based on a report in the community forum: https://forum.proxmox.com/threads/183244/

Changes since version 1 (thanks @Fabian for review):
- Fully reworked locking helpers for s3-refresh and mount/unmount operations
- Include new maintenance type create, used to unblock concurrent datastore creation
- Refactor and code cleanup, especially with respect to chunk store scope restriction


proxmox:

Christian Ebner (1):
  pbs-api-types: add datastore create maintenance-mode type

 pbs-api-types/src/datastore.rs   | 8 ++++++++
 pbs-api-types/src/maintenance.rs | 4 ++++
 2 files changed, 12 insertions(+)


proxmox-backup:

Christian Ebner (9):
  api: config: rearrange independent code block for datastore creation
  api/datastore: refactor datastore creation helper logic
  datastore: restrict chunk store scope to pbs-datastore crate
  datastore: move lock files base path constant to central location
  datastore: move file lock helper to centralized place
  datastore: create lockdir with correct mode for backup user access
  api/datastore: use maintenance-mode lock to protect against changes
  api: config: unlocked s3 bucket access check for datastore creation
  datastore: protect datastore creation by maintenance-mode

 pbs-datastore/src/backup_info.rs              |  36 +---
 pbs-datastore/src/chunk_store.rs              |  22 +--
 pbs-datastore/src/datastore.rs                | 127 ++++++++++++-
 pbs-datastore/src/lib.rs                      |  70 +++++++-
 .../src/local_datastore_lru_cache.rs          |   2 +-
 pbs-datastore/src/move_journal.rs             |   2 +-
 proxmox-backup-client/src/main.rs             |  15 +-
 src/api2/admin/datastore.rs                   |  67 +++----
 src/api2/config/datastore.rs                  | 167 ++++--------------
 src/api2/helpers.rs                           |  77 +++++++-
 src/api2/node/disks/directory.rs              |  27 ++-
 src/api2/node/disks/zfs.rs                    |  30 +++-
 12 files changed, 395 insertions(+), 247 deletions(-)


Summary over all repositories:
  14 files changed, 407 insertions(+), 247 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH proxmox v2 01/10] pbs-api-types: add datastore create maintenance-mode type

[Christian Ebner]

Datastore creation can take some time, especially for stores located
on network attached filesystems or datastores backed by S3.

Therefore, add a maintenance-mode type `create`, which will be used to
block access to the datastore while it is being created, without the
need to hold the datasore configuration lock for the whole
construction period. Rather the maintenance-mode will be locked until
the operation is completed or failed.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-api-types/src/datastore.rs   | 8 ++++++++
 pbs-api-types/src/maintenance.rs | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 098d2b7c..065afc2f 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -648,6 +648,14 @@ impl DataStoreConfig {
                     }
                 }
             }
+            Some(MaintenanceType::Create) => {
+                match new_type {
+                    Some(MaintenanceType::Delete) => { /* allows deletion if creation failed */ }
+                    Some(MaintenanceType::S3Refresh) => { /* refresh from s3 during creation */ }
+                    None => { /* switch to regular operation after successful creation */ }
+                    _ => bail!("datastore is being created or creation failed"),
+                }
+            }
             #[cfg(feature = "enum-fallback")]
             Some(MaintenanceType::UnknownEnumValue(s)) => {
                 bail!("unknown maintenance type: {s}")
diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
index fc7d240d..9d3b769a 100644
--- a/pbs-api-types/src/maintenance.rs
+++ b/pbs-api-types/src/maintenance.rs
@@ -44,6 +44,8 @@ pub enum MaintenanceType {
     //  - Add "GarbageCollection" or "DeleteOnly" as type and track GC (or all deletes) as separate
     //    operation, so that one can enable a mode where nothing new can be added but stuff can be
     //    cleaned
+    /// The datastore is being created
+    Create,
     /// Only read operations are allowed on the datastore.
     ReadOnly,
     /// Neither read nor write operations are allowed on the datastore.
@@ -96,6 +98,8 @@ impl MaintenanceMode {
     pub fn check(&self, operation: Operation) -> Result<(), Error> {
         if self.ty == MaintenanceType::Delete {
             bail!("datastore is being deleted");
+        } else if self.ty == MaintenanceType::Create {
+            bail!("datastore is being created");
         }
 
         let message = percent_encoding::percent_decode_str(self.message.as_deref().unwrap_or(""))
-- 
2.47.3

[PATCH proxmox-backup v2 02/10] api: config: rearrange independent code block for datastore creation

[Christian Ebner]

In preparation for moving most of the datastore creation logic from
the worker task helper to the datastore implementation itself.

The unmount guard is only crated for removable datastores, so it can
also be placed before the backend type check and s3 client
instantiation.

No functional changes.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/config/datastore.rs | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 16e85a636..7c3531a4b 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -126,6 +126,13 @@ pub(crate) fn do_create_datastore(
             .parse_property_string(datastore.tuning.as_deref().unwrap_or(""))?,
     )?;
 
+    let unmount_guard = if datastore.backing_device.is_some() {
+        do_mount_device(datastore.clone())?;
+        UnmountGuard::new(Some(path.clone()))
+    } else {
+        UnmountGuard::new(None)
+    };
+
     let (backend_type, backend_s3_client) =
         match DataStore::s3_client_and_backend_from_datastore_config(&datastore)? {
             (backend_type, Some(s3_client)) => {
@@ -155,13 +162,6 @@ pub(crate) fn do_create_datastore(
             (backend_type, None) => (backend_type, None),
         };
 
-    let unmount_guard = if datastore.backing_device.is_some() {
-        do_mount_device(datastore.clone())?;
-        UnmountGuard::new(Some(path.clone()))
-    } else {
-        UnmountGuard::new(None)
-    };
-
     let chunk_store = if reuse_datastore && backend_type == DatastoreBackendType::Filesystem {
         ChunkStore::verify_chunkstore(&path).and_then(|_| {
             // Must be the only instance accessing and locking the chunk store,
-- 
2.47.3

[PATCH proxmox-backup v2 03/10] api/datastore: refactor datastore creation helper logic

[Christian Ebner]

Move most of the logic from the datastore creation worker task helper
to the datastore implementation itself.

This has the advantage of better encapsulation, reducing the
interdependencies.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 131 ++++++++++++++++++++++++++++++++-
 src/api2/config/datastore.rs   | 123 ++-----------------------------
 2 files changed, 136 insertions(+), 118 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 34efcd398..84314cd29 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -19,7 +19,7 @@ use tracing::{info, warn};
 use proxmox_human_byte::HumanByte;
 use proxmox_s3_client::{
     RequestCounterThresholds, S3Client, S3ClientConf, S3ClientOptions, S3ObjectKey, S3PathPrefix,
-    S3RateLimiterOptions, S3RequestCounterConfig, SharedRequestCounters,
+    S3RateLimiterOptions, S3RequestCounterConfig, SharedRequestCounters, S3_HTTP_REQUEST_TIMEOUT,
 };
 use proxmox_schema::ApiType;
 
@@ -388,6 +388,17 @@ impl DatastoreThreadSettings {
     }
 }
 
+#[derive(Default, serde::Deserialize, serde::Serialize)]
+#[serde(rename_all = "kebab-case")]
+/// S3 in-use marker content
+///
+/// Allows to de-/serialize the contents of the in-use object contents stored on S3 backends to
+/// protect against access by multiple PBS instances.
+struct InUseContent {
+    #[serde(skip_serializing_if = "Option::is_none")]
+    hostname: Option<String>,
+}
+
 /// Returns the parsed datastore config (`datastore.cfg`) and its
 /// generation.
 ///
@@ -464,6 +475,124 @@ impl DataStore {
         })
     }
 
+    /// Create or reuse the chunk store from given datastore configuration
+    pub fn construct_chunk_store(
+        store_config: &mut DataStoreConfig,
+        tuning: &DatastoreTuning,
+        reuse_existing: bool,
+        overwrite_in_use: bool,
+    ) -> Result<(), Error> {
+        let (backend_type, backend_s3_client) =
+            match DataStore::s3_client_and_backend_from_datastore_config(store_config)? {
+                (backend_type, Some(s3_client)) => {
+                    if !overwrite_in_use {
+                        let object_key = S3ObjectKey::try_from(S3_DATASTORE_IN_USE_MARKER)
+                            .context("failed to generate s3 object key")?;
+                        if let Some(response) = proxmox_async::runtime::block_on(
+                            s3_client.get_object(object_key.clone()),
+                        )
+                        .context("failed to get in-use marker from bucket")?
+                        {
+                            let content =
+                                proxmox_async::runtime::block_on(response.content.collect())
+                                    .unwrap_or_default();
+                            let content =
+                                String::from_utf8(content.to_bytes().to_vec()).unwrap_or_default();
+                            let in_use: InUseContent =
+                                serde_json::from_str(&content).unwrap_or_default();
+                            if let Some(hostname) = in_use.hostname {
+                                bail!(
+                                    "Bucket already contains datastore in use by host {hostname}"
+                                );
+                            } else {
+                                bail!("Bucket already contains datastore in use");
+                            }
+                        }
+                    }
+
+                    (backend_type, Some(Arc::new(s3_client)))
+                }
+                (backend_type, None) => (backend_type, None),
+            };
+
+        let path: PathBuf = store_config.absolute_path().into();
+
+        let chunk_store = if reuse_existing && backend_type == DatastoreBackendType::Filesystem {
+            ChunkStore::verify_chunkstore(&path).and_then(|_| {
+                // Must be the only instance accessing and locking the chunk store,
+                // dropping will close all other locks from this process on the lockfile as well.
+                ChunkStore::open(
+                    &store_config.name,
+                    &path,
+                    tuning.sync_level.unwrap_or_default(),
+                )
+            })?
+        } else {
+            if !reuse_existing && backend_type == DatastoreBackendType::Filesystem {
+                if let Ok(dir) = std::fs::read_dir(&path) {
+                    for file in dir {
+                        let name = file?.file_name();
+                        let name = name.to_str();
+                        if !name.is_some_and(|name| name.starts_with('.') || name == "lost+found") {
+                            bail!("datastore path not empty");
+                        }
+                    }
+                }
+            }
+            if reuse_existing && backend_type == DatastoreBackendType::S3 {
+                let chunks_path = path.join(".chunks");
+                if let Err(err) = std::fs::remove_dir_all(&chunks_path) {
+                    if err.kind() != std::io::ErrorKind::NotFound {
+                        return Err(err).with_context(|| {
+                            format!("failed to remove pre-existing chunks in {chunks_path:?}")
+                        });
+                    }
+                }
+                // 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 = pbs_config::backup_user()?;
+            ChunkStore::create(
+                &store_config.name,
+                path.clone(),
+                backup_user.uid,
+                backup_user.gid,
+                tuning.sync_level.unwrap_or_default(),
+            )?
+        };
+
+        if let Some(ref s3_client) = backend_s3_client {
+            let object_key = S3ObjectKey::try_from(S3_DATASTORE_IN_USE_MARKER)
+                .context("failed to generate s3 object key")?;
+            let content = serde_json::to_string(&InUseContent {
+                hostname: Some(proxmox_sys::nodename().to_string()),
+            })
+            .context("failed to encode hostname")?;
+            proxmox_async::runtime::block_on(s3_client.put_object(
+                object_key,
+                hyper::body::Bytes::from(content).into(),
+                Some(S3_HTTP_REQUEST_TIMEOUT),
+                true,
+            ))
+            .context("failed to upload in-use marker for datastore")?;
+        }
+
+        if tuning.gc_atime_safety_check.unwrap_or(true) {
+            chunk_store
+                .check_fs_atime_updates(true, backend_s3_client)
+                .context("access time safety check failed")?;
+            info!("Access time update check successful.");
+        } else {
+            info!("Access time update check skipped.");
+        }
+
+        Ok(())
+    }
+
     /// Get the backend for this datastore based on it's configuration
     pub fn backend(&self) -> Result<DatastoreBackend, Error> {
         let backend = match self.backend_type() {
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 7c3531a4b..70e349b2f 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -1,11 +1,9 @@
 use std::path::{Path, PathBuf};
-use std::sync::Arc;
 
 use ::serde::{Deserialize, Serialize};
 use anyhow::{bail, format_err, Context, Error};
-use http_body_util::BodyExt;
 use serde_json::Value;
-use tracing::{info, warn};
+use tracing::warn;
 
 use proxmox_router::{http_bail, Permission, Router, RpcEnvironment, RpcEnvironmentType};
 use proxmox_schema::{api, param_bail, ApiType};
@@ -14,12 +12,11 @@ use proxmox_uuid::Uuid;
 
 use pbs_api_types::{
     Authid, DataStoreConfig, DataStoreConfigUpdater, DatastoreBackendType, DatastoreNotify,
-    DatastoreTuning, KeepOptions, MaintenanceMode, MaintenanceType, PruneJobConfig,
-    PruneJobOptions, DATASTORE_SCHEMA, PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT,
-    PRIV_DATASTORE_MODIFY, PRIV_SYS_MODIFY, PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA,
+    DatastoreTuning, KeepOptions, MaintenanceMode, PruneJobConfig, PruneJobOptions,
+    DATASTORE_SCHEMA, PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY,
+    PRIV_SYS_MODIFY, PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA,
 };
 use pbs_config::BackupLockGuard;
-use pbs_datastore::chunk_store::ChunkStore;
 
 use crate::api2::admin::datastore::do_mount_device;
 use crate::api2::admin::prune::list_prune_jobs;
@@ -31,20 +28,12 @@ use crate::api2::config::tape_backup_job::{delete_tape_backup_job, list_tape_bac
 use crate::api2::config::verify::delete_verification_job;
 use pbs_config::CachedUserInfo;
 
-use pbs_datastore::{get_datastore_mount_status, DataStore, S3_DATASTORE_IN_USE_MARKER};
+use pbs_datastore::{get_datastore_mount_status, DataStore};
 use proxmox_rest_server::WorkerTask;
-use proxmox_s3_client::{S3ObjectKey, S3_HTTP_REQUEST_TIMEOUT};
 
 use crate::server::jobstate;
 use crate::tools::disks::unmount_by_mountpoint;
 
-#[derive(Default, serde::Deserialize, serde::Serialize)]
-#[serde(rename_all = "kebab-case")]
-struct InUseContent {
-    #[serde(skip_serializing_if = "Option::is_none")]
-    hostname: Option<String>,
-}
-
 #[api(
     input: {
         properties: {},
@@ -133,107 +122,7 @@ pub(crate) fn do_create_datastore(
         UnmountGuard::new(None)
     };
 
-    let (backend_type, backend_s3_client) =
-        match DataStore::s3_client_and_backend_from_datastore_config(&datastore)? {
-            (backend_type, Some(s3_client)) => {
-                if !overwrite_in_use {
-                    let object_key = S3ObjectKey::try_from(S3_DATASTORE_IN_USE_MARKER)
-                        .context("failed to generate s3 object key")?;
-                    if let Some(response) =
-                        proxmox_async::runtime::block_on(s3_client.get_object(object_key.clone()))
-                            .context("failed to get in-use marker from bucket")?
-                    {
-                        let content = proxmox_async::runtime::block_on(response.content.collect())
-                            .unwrap_or_default();
-                        let content =
-                            String::from_utf8(content.to_bytes().to_vec()).unwrap_or_default();
-                        let in_use: InUseContent =
-                            serde_json::from_str(&content).unwrap_or_default();
-                        if let Some(hostname) = in_use.hostname {
-                            bail!("Bucket already contains datastore in use by host {hostname}");
-                        } else {
-                            bail!("Bucket already contains datastore in use");
-                        }
-                    }
-                }
-
-                (backend_type, Some(Arc::new(s3_client)))
-            }
-            (backend_type, None) => (backend_type, None),
-        };
-
-    let chunk_store = if reuse_datastore && backend_type == DatastoreBackendType::Filesystem {
-        ChunkStore::verify_chunkstore(&path).and_then(|_| {
-            // Must be the only instance accessing and locking the chunk store,
-            // dropping will close all other locks from this process on the lockfile as well.
-            ChunkStore::open(
-                &datastore.name,
-                &path,
-                tuning.sync_level.unwrap_or_default(),
-            )
-        })?
-    } else {
-        if !reuse_datastore && backend_type == DatastoreBackendType::Filesystem {
-            if let Ok(dir) = std::fs::read_dir(&path) {
-                for file in dir {
-                    let name = file?.file_name();
-                    let name = name.to_str();
-                    if !name.is_some_and(|name| name.starts_with('.') || name == "lost+found") {
-                        bail!("datastore path not empty");
-                    }
-                }
-            }
-        }
-        if reuse_datastore && backend_type == DatastoreBackendType::S3 {
-            let chunks_path = path.join(".chunks");
-            if let Err(err) = std::fs::remove_dir_all(&chunks_path) {
-                if err.kind() != std::io::ErrorKind::NotFound {
-                    return Err(err).with_context(|| {
-                        format!("failed to remove pre-existing chunks in {chunks_path:?}")
-                    });
-                }
-            }
-            // starting out in maintenance mode s3-refresh,
-            // so no other operation will start until done with that.
-            datastore.set_maintenance_mode(Some(MaintenanceMode {
-                ty: MaintenanceType::S3Refresh,
-                message: None,
-            }))?;
-        }
-        let backup_user = pbs_config::backup_user()?;
-        ChunkStore::create(
-            &datastore.name,
-            path.clone(),
-            backup_user.uid,
-            backup_user.gid,
-            tuning.sync_level.unwrap_or_default(),
-        )?
-    };
-
-    if let Some(ref s3_client) = backend_s3_client {
-        let object_key = S3ObjectKey::try_from(S3_DATASTORE_IN_USE_MARKER)
-            .context("failed to generate s3 object key")?;
-        let content = serde_json::to_string(&InUseContent {
-            hostname: Some(proxmox_sys::nodename().to_string()),
-        })
-        .context("failed to encode hostname")?;
-        proxmox_async::runtime::block_on(s3_client.put_object(
-            object_key,
-            hyper::body::Bytes::from(content).into(),
-            Some(S3_HTTP_REQUEST_TIMEOUT),
-            true,
-        ))
-        .context("failed to upload in-use marker for datastore")?;
-    }
-
-    if tuning.gc_atime_safety_check.unwrap_or(true) {
-        chunk_store
-            .check_fs_atime_updates(true, backend_s3_client)
-            .context("access time safety check failed")?;
-        info!("Access time update check successful.");
-    } else {
-        info!("Access time update check skipped.");
-    }
+    DataStore::construct_chunk_store(&mut datastore, &tuning, reuse_datastore, overwrite_in_use)?;
 
     config.set_data(&datastore.name, "datastore", &datastore)?;
 
-- 
2.47.3

[PATCH proxmox-backup v2 04/10] datastore: restrict chunk store scope to pbs-datastore crate

[Christian Ebner]

Implementation details of the chunk store should not be used outside
of the pbs-datastore crate. After refactoring datastore creation to
be contained within the crate boundary in previous code changes, the
only remaining outside dependency is the list and check for allowed
chunk sizes, which is however only used by the proxmox-backup-client.

Therefore, move the list to the client, inline the check and limit
the chunk store scope to be crate only.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs               | 17 -----------------
 pbs-datastore/src/lib.rs                       |  3 +--
 pbs-datastore/src/local_datastore_lru_cache.rs |  2 +-
 proxmox-backup-client/src/main.rs              | 15 +++++++++++++--
 4 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 68db88eab..735744506 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -39,23 +39,6 @@ pub struct ChunkStore {
 
 // TODO: what about sysctl setting vm.vfs_cache_pressure (0 - 100) ?
 
-pub fn verify_chunk_size(size: usize) -> Result<(), Error> {
-    static SIZES: [usize; 7] = [
-        64 * 1024,
-        128 * 1024,
-        256 * 1024,
-        512 * 1024,
-        1024 * 1024,
-        2048 * 1024,
-        4096 * 1024,
-    ];
-
-    if !SIZES.contains(&size) {
-        bail!("Got unsupported chunk size '{size}'");
-    }
-    Ok(())
-}
-
 fn digest_to_prefix(digest: &[u8]) -> PathBuf {
     let mut buf = Vec::<u8>::with_capacity(2 + 1 + 2 + 1);
 
diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index 6647ee2b6..34d9a6339 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -183,7 +183,7 @@ pub mod catalog;
 pub mod checksum_reader;
 pub mod checksum_writer;
 pub mod chunk_stat;
-pub mod chunk_store;
+pub(crate) mod chunk_store;
 pub mod chunker;
 pub mod crypt_reader;
 pub mod crypt_writer;
@@ -206,7 +206,6 @@ pub mod fixed_index;
 pub use backup_info::{BackupDir, BackupGroup, BackupInfo};
 pub use checksum_reader::ChecksumReader;
 pub use checksum_writer::ChecksumWriter;
-pub use chunk_store::ChunkStore;
 pub use chunker::{Chunker, ChunkerImpl, PayloadChunker};
 pub use crypt_reader::CryptReader;
 pub use crypt_writer::CryptWriter;
diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs
index ac27d4637..b4853a3d8 100644
--- a/pbs-datastore/src/local_datastore_lru_cache.rs
+++ b/pbs-datastore/src/local_datastore_lru_cache.rs
@@ -9,7 +9,7 @@ use http_body_util::BodyExt;
 use pbs_tools::async_lru_cache::AsyncLruCache;
 use proxmox_s3_client::S3Client;
 
-use crate::ChunkStore;
+use crate::chunk_store::ChunkStore;
 use crate::DataBlob;
 
 /// LRU cache using local datastore for caching chunks
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 5e8bb5393..c2943dbcd 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -51,7 +51,6 @@ use pbs_client::{
     BACKUP_SOURCE_SCHEMA,
 };
 use pbs_datastore::catalog::{BackupCatalogWriter, CatalogReader, CatalogWriter};
-use pbs_datastore::chunk_store::verify_chunk_size;
 use pbs_datastore::dynamic_index::{BufferedDynamicReader, DynamicIndexReader, LocalDynamicReadAt};
 use pbs_datastore::fixed_index::FixedIndexReader;
 use pbs_datastore::index::IndexFile;
@@ -85,6 +84,16 @@ pub use snapshot::*;
 mod task;
 pub use task::*;
 
+static ALLOWED_CHUNK_SIZES: [usize; 7] = [
+    64 * 1024,
+    128 * 1024,
+    256 * 1024,
+    512 * 1024,
+    1024 * 1024,
+    2048 * 1024,
+    4096 * 1024,
+];
+
 fn record_repository(repo: &BackupRepository) {
     let base = match BaseDirectories::with_prefix("proxmox-backup") {
         Ok(v) => v,
@@ -813,7 +822,9 @@ async fn create_backup(
     let chunk_size_opt = param["chunk-size"].as_u64().map(|v| (v * 1024) as usize);
 
     if let Some(size) = chunk_size_opt {
-        verify_chunk_size(size)?;
+        if !ALLOWED_CHUNK_SIZES.contains(&size) {
+            bail!("Got unsupported chunk size '{size}'");
+        }
     }
 
     let rate_limit = RateLimitConfig::from_client_config(limit);
-- 
2.47.3

[PATCH proxmox-backup v2 05/10] datastore: move lock files base path constant to central location

[Christian Ebner]

For better organization of the locking related helpers. This constant
is used in various places for locking, therefore move it to the more
fitting central crate lib.

No functional changes.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/backup_info.rs  | 3 +--
 pbs-datastore/src/chunk_store.rs  | 3 +--
 pbs-datastore/src/lib.rs          | 2 ++
 pbs-datastore/src/move_journal.rs | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index 223a8cbd4..9919b908a 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -24,9 +24,8 @@ use crate::datastore::{GROUP_NOTES_FILE_NAME, GROUP_OWNER_FILE_NAME};
 use crate::manifest::{BackupManifest, MANIFEST_LOCK_NAME};
 use crate::move_journal;
 use crate::s3::S3_CONTENT_PREFIX;
-use crate::{DataBlob, DataStore, DatastoreBackend};
+use crate::{DataBlob, DataStore, DatastoreBackend, DATASTORE_LOCKS_DIR};
 
-pub const DATASTORE_LOCKS_DIR: &str = "/run/proxmox-backup/locks";
 pub const PROTECTED_MARKER_FILENAME: &str = ".protected";
 
 proxmox_schema::const_regex! {
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 735744506..f239b4237 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -18,12 +18,11 @@ use proxmox_sys::process_locker::{
 };
 use proxmox_worker_task::WorkerTaskContext;
 
-use crate::backup_info::DATASTORE_LOCKS_DIR;
 use crate::data_blob::DataChunkBuilder;
 use crate::file_formats::{
     COMPRESSED_BLOB_MAGIC_1_0, ENCRYPTED_BLOB_MAGIC_1_0, UNCOMPRESSED_BLOB_MAGIC_1_0,
 };
-use crate::{DataBlob, LocalDatastoreLruCache};
+use crate::{DataBlob, LocalDatastoreLruCache, DATASTORE_LOCKS_DIR};
 
 const USING_MARKER_FILENAME_EXT: &str = "using";
 
diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index 34d9a6339..358f418d1 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -162,6 +162,8 @@ pub const ACTIVE_OPERATIONS_DIR: &str = concat!(
     pbs_buildcfg::PROXMOX_BACKUP_RUN_DIR_M!(),
     "/active-operations"
 );
+/// Base path for datastore related file locks.
+pub const DATASTORE_LOCKS_DIR: &str = "/run/proxmox-backup/locks";
 
 #[macro_export]
 macro_rules! PROXMOX_BACKUP_PROTOCOL_ID_V1 {
diff --git a/pbs-datastore/src/move_journal.rs b/pbs-datastore/src/move_journal.rs
index 891644d7a..fb41e863c 100644
--- a/pbs-datastore/src/move_journal.rs
+++ b/pbs-datastore/src/move_journal.rs
@@ -54,7 +54,7 @@ use proxmox_sys::fs::{open_file_locked, CreateOptions};
 
 use pbs_config::backup_user;
 
-use crate::backup_info::DATASTORE_LOCKS_DIR;
+use crate::DATASTORE_LOCKS_DIR;
 
 const JOURNAL_FILENAME: &str = "move-journal";
 const APPEND_LOCK_TIMEOUT: Duration = Duration::from_secs(10);
-- 
2.47.3

[PATCH proxmox-backup v2 06/10] datastore: move file lock helper to centralized place

[Christian Ebner]

Moves the implementation to a more central location, as it is not
only used by the datastore contents, but also by the chunk store.

No functional changes.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/backup_info.rs | 35 ++---------------------------
 pbs-datastore/src/chunk_store.rs |  2 +-
 pbs-datastore/src/lib.rs         | 38 ++++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 34 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index 9919b908a..bae136d3c 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -1,5 +1,5 @@
 use std::fmt;
-use std::os::unix::io::{AsRawFd, RawFd};
+use std::os::unix::io::RawFd;
 use std::os::unix::prelude::OsStrExt;
 use std::path::Path;
 use std::path::PathBuf;
@@ -24,7 +24,7 @@ use crate::datastore::{GROUP_NOTES_FILE_NAME, GROUP_OWNER_FILE_NAME};
 use crate::manifest::{BackupManifest, MANIFEST_LOCK_NAME};
 use crate::move_journal;
 use crate::s3::S3_CONTENT_PREFIX;
-use crate::{DataBlob, DataStore, DatastoreBackend, DATASTORE_LOCKS_DIR};
+use crate::{lock_helper, DataBlob, DataStore, DatastoreBackend, DATASTORE_LOCKS_DIR};
 
 pub const PROTECTED_MARKER_FILENAME: &str = ".protected";
 
@@ -1210,34 +1210,3 @@ fn lock_file_path_helper(ns: &BackupNamespace, path: PathBuf) -> PathBuf {
 
     to_return.join(format!("{first_eigthy}...{last_eighty}-{hash}"))
 }
-
-/// Helps implement the double stat'ing procedure. It avoids certain race conditions upon lock
-/// deletion.
-///
-/// It also creates the base directory for lock files.
-pub(crate) fn lock_helper<F>(
-    store_name: &str,
-    path: &std::path::Path,
-    lock_fn: F,
-) -> Result<BackupLockGuard, Error>
-where
-    F: Fn(&std::path::Path) -> Result<BackupLockGuard, Error>,
-{
-    let mut lock_dir = Path::new(DATASTORE_LOCKS_DIR).join(store_name);
-
-    if let Some(parent) = path.parent() {
-        lock_dir = lock_dir.join(parent);
-    };
-
-    std::fs::create_dir_all(&lock_dir)?;
-
-    let lock = lock_fn(path)?;
-
-    let inode = nix::sys::stat::fstat(lock.as_raw_fd())?.st_ino;
-
-    if nix::sys::stat::stat(path).map_or(true, |st| inode != st.st_ino) {
-        bail!("could not acquire lock, another thread modified the lock file");
-    }
-
-    Ok(lock)
-}
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index f239b4237..e9dadce59 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -927,7 +927,7 @@ impl ChunkStore {
         timeout: Duration,
     ) -> Result<BackupLockGuard, Error> {
         let lock_path = self.chunk_lock_path(digest);
-        let guard = crate::backup_info::lock_helper(self.name(), &lock_path, |path| {
+        let guard = crate::lock_helper(self.name(), &lock_path, |path| {
             pbs_config::open_backup_lockfile(path, Some(timeout), true)
         })?;
         Ok(guard)
diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index 358f418d1..f10a2840d 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -157,6 +157,13 @@
 
 #![deny(unsafe_op_in_unsafe_fn)]
 
+use std::os::unix::io::AsRawFd;
+use std::path::Path;
+
+use anyhow::{bail, Error};
+
+use pbs_config::BackupLockGuard;
+
 /// Directory path where active operations counters are saved.
 pub const ACTIVE_OPERATIONS_DIR: &str = concat!(
     pbs_buildcfg::PROXMOX_BACKUP_RUN_DIR_M!(),
@@ -236,3 +243,34 @@ pub use local_chunk_reader::LocalChunkReader;
 
 mod local_datastore_lru_cache;
 pub use local_datastore_lru_cache::LocalDatastoreLruCache;
+
+/// Helps implement the double stat'ing procedure. It avoids certain race conditions upon lock
+/// deletion.
+///
+/// It also creates the base directory for lock files.
+pub(crate) fn lock_helper<F>(
+    store_name: &str,
+    path: &Path,
+    lock_fn: F,
+) -> Result<BackupLockGuard, Error>
+where
+    F: Fn(&Path) -> Result<BackupLockGuard, Error>,
+{
+    let mut lock_dir = Path::new(DATASTORE_LOCKS_DIR).join(store_name);
+
+    if let Some(parent) = path.parent() {
+        lock_dir = lock_dir.join(parent);
+    };
+
+    std::fs::create_dir_all(&lock_dir)?;
+
+    let lock = lock_fn(path)?;
+
+    let inode = nix::sys::stat::fstat(lock.as_raw_fd())?.st_ino;
+
+    if nix::sys::stat::stat(path).map_or(true, |st| inode != st.st_ino) {
+        bail!("could not acquire lock, another thread modified the lock file");
+    }
+
+    Ok(lock)
+}
-- 
2.47.3

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

[Christian Ebner]

Since the helper can now also get called via the privileged api
server, running as root, assure the base lock directories are created
so user backup can access them.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/lib.rs | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index f10a2840d..17d26647e 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -162,6 +162,8 @@ use std::path::Path;
 
 use anyhow::{bail, Error};
 
+use proxmox_sys::fs::CreateOptions;
+
 use pbs_config::BackupLockGuard;
 
 /// Directory path where active operations counters are saved.
@@ -262,7 +264,13 @@ where
         lock_dir = lock_dir.join(parent);
     };
 
-    std::fs::create_dir_all(&lock_dir)?;
+    let backup_user = pbs_config::backup_user()?;
+    let dir_create_options = CreateOptions::new()
+        .perm(nix::sys::stat::Mode::from_bits_truncate(0o755))
+        .owner(backup_user.uid)
+        .group(backup_user.gid);
+
+    proxmox_sys::fs::create_path(lock_dir, Some(dir_create_options), Some(dir_create_options))?;
 
     let lock = lock_fn(path)?;
 
-- 
2.47.3

[PATCH proxmox-backup v2 08/10] api/datastore: use maintenance-mode lock to protect against changes

[Christian Ebner]

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>
---
 pbs-datastore/src/datastore.rs |  2 ++
 pbs-datastore/src/lib.rs       | 21 +++++++++++-
 src/api2/admin/datastore.rs    | 62 ++++++++++++++++++++--------------
 src/api2/config/datastore.rs   |  6 ++--
 4 files changed, 63 insertions(+), 28 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 84314cd29..d2136460e 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -3165,6 +3165,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,
@@ -3172,6 +3173,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 17d26647e..c1f528889 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::{bail, Error};
+use anyhow::{bail, Context, Error};
 
 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")
+    })
+}
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 1e0a78f3a..e5f05c809 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -61,8 +61,8 @@ use pbs_datastore::index::IndexFile;
 use pbs_datastore::manifest::BackupManifest;
 use pbs_datastore::prune::compute_prune_info;
 use pbs_datastore::{
-    check_backup_owner, ensure_datastore_is_mounted, task_tracking, BackupDir, DataStore,
-    LocalChunkReader, StoreProgress,
+    check_backup_owner, ensure_datastore_is_mounted, maintenance_mode_lock, task_tracking,
+    BackupDir, DataStore, LocalChunkReader, StoreProgress,
 };
 use pbs_tools::json::required_string_param;
 use proxmox_rest_server::{formatter, worker_is_active, WorkerTask};
@@ -2686,11 +2686,7 @@ 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<(pbs_config::BackupLockGuard, DataStoreConfig), Error> {
-    let lock = pbs_config::datastore::lock_config()?;
+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)?;
 
@@ -2701,16 +2697,25 @@ fn expect_maintenance_type(
         bail!("maintenance mode is not '{maintenance_type}'");
     }
 
-    Ok((lock, store_config))
+    Ok(())
 }
 
 fn unset_maintenance(
-    _lock: pbs_config::BackupLockGuard,
-    mut config: DataStoreConfig,
+    maintenance_mode_lock: Option<pbs_config::BackupLockGuard>,
+    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()?;
-    config.maintenance_mode = None;
-    section_config.set_data(&config.name, "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(())
 }
@@ -2759,10 +2764,11 @@ async fn do_unmount(store: String, auth_id: Authid, to_stdout: bool) -> Result<V
         }
     }
 
-    let _lock = pbs_config::datastore::lock_config()?;
+    let lock = pbs_config::datastore::lock_config()?;
     let (mut section_config, _digest) = pbs_config::datastore::config()?;
     let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
 
+    let maintenance_mode_lock = maintenance_mode_lock(&store, &lock)?;
     datastore.set_maintenance_mode(Some(MaintenanceMode {
         ty: MaintenanceType::Unmount,
         message: None,
@@ -2770,7 +2776,8 @@ async fn do_unmount(store: String, auth_id: Authid, to_stdout: bool) -> Result<V
     section_config.set_data(&store, "datastore", &datastore)?;
     pbs_config::datastore::save_config(&section_config)?;
 
-    drop(_lock);
+    drop(maintenance_mode_lock);
+    drop(lock);
 
     if let Ok(proxy_pid) = proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)
     {
@@ -2841,10 +2848,11 @@ pub fn s3_refresh(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
 
-    let _lock = pbs_config::datastore::lock_config()?;
+    let lock = pbs_config::datastore::lock_config()?;
     let (mut section_config, _digest) = pbs_config::datastore::config()?;
     let mut datastore_config: DataStoreConfig = section_config.lookup("datastore", &store)?;
 
+    let maintenance_mode_lock = maintenance_mode_lock(&store, &lock)?;
     datastore_config.set_maintenance_mode(Some(MaintenanceMode {
         ty: MaintenanceType::S3Refresh,
         message: None,
@@ -2852,7 +2860,8 @@ pub fn s3_refresh(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
 
     section_config.set_data(&store, "datastore", &datastore_config)?;
     pbs_config::datastore::save_config(&section_config)?;
-    drop(_lock);
+    drop(maintenance_mode_lock);
+    drop(lock);
 
     let upid = WorkerTask::new_thread(
         "s3-refresh",
@@ -2875,7 +2884,8 @@ pub(crate) fn do_s3_refresh(store: &str, worker: &dyn WorkerTaskContext) -> Resu
 }
 
 /// 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.
+/// a maintenance mode locked context, protecting against leaving the mode for the functions
+/// lifetime.
 fn run_maintenance_locked(
     store: &str,
     maintenance_expected: MaintenanceType,
@@ -2905,18 +2915,20 @@ fn run_maintenance_locked(
     }
 
     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}"))
-            });
+        let _ = unset_maintenance(None, store, maintenance_expected)
+            .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)?;
+    let lock = pbs_config::datastore::lock_config()?;
+    let maintenance_mode_lock = maintenance_mode_lock(store, &lock)?;
+    expect_maintenance_type(store, maintenance_expected)?;
+    // avoid blocking config access/updates, but keep maintenance-mode locked
+    // until done.
+    drop(lock);
+
     callback()?;
-    unset_maintenance(lock, config)
+    unset_maintenance(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 70e349b2f..fe8e641a3 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -28,7 +28,7 @@ use crate::api2::config::tape_backup_job::{delete_tape_backup_job, list_tape_bac
 use crate::api2::config::verify::delete_verification_job;
 use pbs_config::CachedUserInfo;
 
-use pbs_datastore::{get_datastore_mount_status, DataStore};
+use pbs_datastore::{get_datastore_mount_status, maintenance_mode_lock, DataStore};
 use proxmox_rest_server::WorkerTask;
 
 use crate::server::jobstate;
@@ -371,7 +371,7 @@ pub fn update_datastore(
     delete: Option<Vec<DeletableProperty>>,
     digest: Option<String>,
 ) -> Result<(), Error> {
-    let _lock = pbs_config::datastore::lock_config()?;
+    let lock = pbs_config::datastore::lock_config()?;
 
     // pass/compare digest
     let (mut config, expected_digest) = pbs_config::datastore::config()?;
@@ -429,6 +429,7 @@ pub fn update_datastore(
                     data.tuning = None;
                 }
                 DeletableProperty::MaintenanceMode => {
+                    let _maintenance_mode_lock = maintenance_mode_lock(&name, &lock)?;
                     data.set_maintenance_mode(None)?;
                 }
                 DeletableProperty::NotificationThresholds => {
@@ -527,6 +528,7 @@ pub fn update_datastore(
             )?),
             None => None,
         };
+        let _maintenance_mode_lock = maintenance_mode_lock(&name, &lock)?;
         data.set_maintenance_mode(maintenance_mode)?;
     }
 
-- 
2.47.3

[PATCH proxmox-backup v2 09/10] api: config: unlocked s3 bucket access check for datastore creation

[Christian Ebner]

The bucket access check performed when creating a new datastore with
s3 backend can theoretically block up to the set s3 client request
timeout of 30 min. It is not acceptable to hold the config lock for
this long, effectively blocking configuration access for unrelated
datastores.

Move the check to the start so it is performed before even locking
the config.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/config/datastore.rs | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index fe8e641a3..3061219ae 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -169,6 +169,13 @@ pub fn create_datastore(
     overwrite_in_use: bool,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<String, Error> {
+    let (backend, s3_client) = DataStore::s3_client_and_backend_from_datastore_config(&config)?;
+    if let Some(s3_client) = s3_client {
+        proxmox_async::runtime::block_on(s3_client.head_bucket())
+            .context("failed to access bucket")
+            .map_err(|err| format_err!("{err:#}"))?;
+    }
+
     let lock = pbs_config::datastore::lock_config()?;
 
     let (section_config, _digest) = pbs_config::datastore::config()?;
@@ -233,13 +240,6 @@ pub fn create_datastore(
 
     let store_name = config.name.to_string();
 
-    let (backend, s3_client) = DataStore::s3_client_and_backend_from_datastore_config(&config)?;
-    if let Some(s3_client) = s3_client {
-        proxmox_async::runtime::block_on(s3_client.head_bucket())
-            .context("failed to access bucket")
-            .map_err(|err| format_err!("{err:#}"))?;
-    }
-
     WorkerTask::new_thread(
         "create-datastore",
         Some(store_name.clone()),
-- 
2.47.3

[PATCH proxmox-backup v2 10/10] datastore: protect datastore creation by maintenance-mode

[Christian Ebner]

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      | 39 ++--------------
 src/api2/config/datastore.rs     | 40 +++++++++++------
 src/api2/helpers.rs              | 77 +++++++++++++++++++++++++++++++-
 src/api2/node/disks/directory.rs | 27 +++++++++--
 src/api2/node/disks/zfs.rs       | 30 ++++++++++---
 6 files changed, 154 insertions(+), 67 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index d2136460e..79d83206b 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -477,7 +477,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,
         tuning: &DatastoreTuning,
         reuse_existing: bool,
         overwrite_in_use: bool,
@@ -548,12 +548,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 = pbs_config::backup_user()?;
             ChunkStore::create(
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index e5f05c809..086697fd9 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -68,6 +68,7 @@ use pbs_tools::json::required_string_param;
 use proxmox_rest_server::{formatter, worker_is_active, WorkerTask};
 
 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::{check_ns_privs_full, ListAccessibleBackupGroups, VerifyWorker, NS_PRIVS_OK};
 use crate::server::jobstate::{compute_schedule_status, Job, JobState};
@@ -2686,40 +2687,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<pbs_config::BackupLockGuard>,
-    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,
@@ -2915,7 +2882,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");
     }
@@ -2928,7 +2895,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 3061219ae..b451b4350 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -12,9 +12,9 @@ use proxmox_uuid::Uuid;
 
 use pbs_api_types::{
     Authid, DataStoreConfig, DataStoreConfigUpdater, DatastoreBackendType, DatastoreNotify,
-    DatastoreTuning, KeepOptions, MaintenanceMode, PruneJobConfig, PruneJobOptions,
-    DATASTORE_SCHEMA, PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY,
-    PRIV_SYS_MODIFY, PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA,
+    DatastoreTuning, KeepOptions, MaintenanceMode, MaintenanceType, PruneJobConfig,
+    PruneJobOptions, DATASTORE_SCHEMA, PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT,
+    PRIV_DATASTORE_MODIFY, PRIV_SYS_MODIFY, PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA,
 };
 use pbs_config::BackupLockGuard;
 
@@ -31,6 +31,7 @@ use pbs_config::CachedUserInfo;
 use pbs_datastore::{get_datastore_mount_status, maintenance_mode_lock, DataStore};
 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;
 
@@ -93,9 +94,9 @@ impl Drop for UnmountGuard {
 }
 
 pub(crate) fn do_create_datastore(
-    _lock: BackupLockGuard,
-    mut config: SectionConfigData,
-    mut datastore: DataStoreConfig,
+    _lock: &BackupLockGuard,
+    config: SectionConfigData,
+    datastore: &DataStoreConfig,
     reuse_datastore: bool,
     overwrite_in_use: bool,
 ) -> Result<(), Error> {
@@ -122,11 +123,7 @@ pub(crate) fn do_create_datastore(
         UnmountGuard::new(None)
     };
 
-    DataStore::construct_chunk_store(&mut datastore, &tuning, reuse_datastore, overwrite_in_use)?;
-
-    config.set_data(&datastore.name, "datastore", &datastore)?;
-
-    pbs_config::datastore::save_config(&config)?;
+    DataStore::construct_chunk_store(datastore, &tuning, reuse_datastore, overwrite_in_use)?;
 
     jobstate::create_state_file("garbage_collection", &datastore.name)?;
 
@@ -232,12 +229,18 @@ pub fn create_datastore(
     }
 
     // clearing prune settings in the datastore config, as they are now handled by prune jobs
-    let config = DataStoreConfig {
+    let mut config = DataStoreConfig {
         prune_schedule: None,
         keep: KeepOptions::default(),
         ..config
     };
 
+    // 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(&config.name, &lock)?;
+    let mut current_type = MaintenanceType::Create;
+    set_maintenance_type(lock, &maintenance_mode_lock, &mut config, current_type)?;
+
     let store_name = config.name.to_string();
 
     WorkerTask::new_thread(
@@ -247,9 +250,9 @@ pub fn create_datastore(
         to_stdout,
         move |worker| {
             do_create_datastore(
-                lock,
+                &maintenance_mode_lock,
                 section_config,
-                config,
+                &config,
                 reuse_datastore,
                 overwrite_in_use,
             )?;
@@ -259,8 +262,17 @@ pub fn create_datastore(
             }
 
             if reuse_datastore && backend == DatastoreBackendType::S3 {
+                // starting out in maintenance mode s3-refresh,
+                // so no other operation will start until done with that.
+                let lock = pbs_config::datastore::lock_config()?;
+                current_type = MaintenanceType::S3Refresh;
+                set_maintenance_type(lock, &maintenance_mode_lock, &mut config, current_type)?;
+
                 crate::api2::admin::datastore::do_s3_refresh(&store_name, &worker)?;
             }
+
+            unset_maintenance_mode(Some(maintenance_mode_lock), &store_name, current_type)?;
+
             Ok(())
         },
     )
diff --git a/src/api2/helpers.rs b/src/api2/helpers.rs
index f346b0cca..9611d6151 100644
--- a/src/api2/helpers.rs
+++ b/src/api2/helpers.rs
@@ -1,12 +1,15 @@
 use std::path::PathBuf;
 
-use anyhow::Error;
+use anyhow::{bail, Error};
 use futures::stream::TryStreamExt;
 use hyper::{header, Response, StatusCode};
 
+use pbs_api_types::{DataStoreConfig, MaintenanceMode, MaintenanceType};
 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 +31,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: &BackupLockGuard,
+    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<BackupLockGuard>,
+    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 c37d65b8d..2d7948f12 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -11,8 +11,8 @@ use proxmox_schema::api;
 use proxmox_section_config::SectionConfigData;
 
 use pbs_api_types::{
-    DataStoreConfig, BLOCKDEVICE_NAME_SCHEMA, DATASTORE_MOUNT_DIR, DATASTORE_SCHEMA, NODE_SCHEMA,
-    PRIV_SYS_AUDIT, PRIV_SYS_MODIFY, UPID_SCHEMA,
+    DataStoreConfig, MaintenanceType, BLOCKDEVICE_NAME_SCHEMA, DATASTORE_MOUNT_DIR,
+    DATASTORE_SCHEMA, NODE_SCHEMA, PRIV_SYS_AUDIT, PRIV_SYS_MODIFY, UPID_SCHEMA,
 };
 
 use crate::tools::disks::{
@@ -240,7 +240,7 @@ pub fn create_datastore_disk(
 
             if add_datastore {
                 let lock = pbs_config::datastore::lock_config()?;
-                let datastore: DataStoreConfig = if removable_datastore {
+                let mut datastore: DataStoreConfig = if removable_datastore {
                     serde_json::from_value(
                         json!({ "name": name, "path": name, "backing-device": uuid }),
                     )?
@@ -253,8 +253,27 @@ pub fn create_datastore_disk(
                     bail!("datastore '{}' already exists.", datastore.name);
                 }
 
+                let maintenance_mode_lock =
+                    pbs_datastore::maintenance_mode_lock(&datastore.name, &lock)?;
+                crate::api2::helpers::set_maintenance_type(
+                    lock,
+                    &maintenance_mode_lock,
+                    &mut datastore,
+                    MaintenanceType::Create,
+                )?;
+
                 crate::api2::config::datastore::do_create_datastore(
-                    lock, config, datastore, false, false,
+                    &maintenance_mode_lock,
+                    config,
+                    &datastore,
+                    false,
+                    false,
+                )?;
+
+                crate::api2::helpers::unset_maintenance_mode(
+                    Some(maintenance_mode_lock),
+                    &datastore.name,
+                    MaintenanceType::Create,
                 )?;
             }
 
diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
index 3e5a7decf..ab48548c4 100644
--- a/src/api2/node/disks/zfs.rs
+++ b/src/api2/node/disks/zfs.rs
@@ -6,9 +6,9 @@ use proxmox_router::{Permission, Router, RpcEnvironment, RpcEnvironmentType};
 use proxmox_schema::api;
 
 use pbs_api_types::{
-    DataStoreConfig, ZfsCompressionType, ZfsRaidLevel, ZpoolListItem, DATASTORE_SCHEMA,
-    DISK_ARRAY_SCHEMA, DISK_LIST_SCHEMA, NODE_SCHEMA, PRIV_SYS_AUDIT, PRIV_SYS_MODIFY, UPID_SCHEMA,
-    ZFS_ASHIFT_SCHEMA, ZPOOL_NAME_SCHEMA,
+    DataStoreConfig, MaintenanceType, ZfsCompressionType, ZfsRaidLevel, ZpoolListItem,
+    DATASTORE_SCHEMA, DISK_ARRAY_SCHEMA, DISK_LIST_SCHEMA, NODE_SCHEMA, PRIV_SYS_AUDIT,
+    PRIV_SYS_MODIFY, UPID_SCHEMA, ZFS_ASHIFT_SCHEMA, ZPOOL_NAME_SCHEMA,
 };
 
 use crate::tools::disks::{
@@ -304,7 +304,7 @@ pub fn create_zpool(
 
             if add_datastore {
                 let lock = pbs_config::datastore::lock_config()?;
-                let datastore: DataStoreConfig =
+                let mut datastore: DataStoreConfig =
                     serde_json::from_value(json!({ "name": name, "path": mount_point }))?;
 
                 let (config, _digest) = pbs_config::datastore::config()?;
@@ -313,8 +313,28 @@ pub fn create_zpool(
                     bail!("datastore '{}' already exists.", datastore.name);
                 }
 
+                let maintenance_mode_lock =
+                    pbs_datastore::maintenance_mode_lock(&datastore.name, &lock)?;
+
+                crate::api2::helpers::set_maintenance_type(
+                    lock,
+                    &maintenance_mode_lock,
+                    &mut datastore,
+                    MaintenanceType::Create,
+                )?;
+
                 crate::api2::config::datastore::do_create_datastore(
-                    lock, config, datastore, false, false,
+                    &maintenance_mode_lock,
+                    config,
+                    &datastore,
+                    false,
+                    false,
+                )?;
+
+                crate::api2::helpers::unset_maintenance_mode(
+                    Some(maintenance_mode_lock),
+                    &datastore.name,
+                    MaintenanceType::Create,
                 )?;
             }
 
-- 
2.47.3