From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id E5A471FF0AA for ; Tue, 06 Oct 2026 16:47:49 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id E2DBB214D7; Tue, 06 Oct 2026 16:47:39 +0200 (CEST) From: Christian Ebner To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox-backup v5 14/14] datastore: protect datastore creation by maintenance-mode Date: Tue, 6 Oct 2026 16:46:44 +0200 Message-ID: <20261006144644.744818-15-c.ebner@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261006144644.744818-1-c.ebner@proxmox.com> References: <20261006144644.744818-1-c.ebner@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1791298027842 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.583 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: WWISNUNLJ5CA74OYVPPXSXTHXVVZJXAN X-Message-ID-Hash: WWISNUNLJ5CA74OYVPPXSXTHXVVZJXAN X-MailFrom: c.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 --- 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 f9cbe0673..a74a6a824 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 6327e5d3f..f25d095b4 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}; @@ -2710,40 +2711,6 @@ pub fn mount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result 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, - 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(§ion_config)?; - Ok(()) -} - fn do_unmount_device( datastore: DataStoreConfig, worker: &dyn WorkerTaskContext, @@ -2939,7 +2906,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"); } @@ -2952,7 +2919,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, 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, 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(§ion_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, + 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(§ion_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