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 772451FF125 for ; Mon, 03 Aug 2026 11:08:50 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 7E90E214B0; Mon, 03 Aug 2026 11:08:28 +0200 (CEST) From: Christian Ebner To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox-backup v4 14/14] datastore: protect datastore creation by maintenance-mode Date: Mon, 3 Aug 2026 11:07:47 +0200 Message-ID: <20260803090747.265683-15-c.ebner@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260803090747.265683-1-c.ebner@proxmox.com> References: <20260803090747.265683-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: 1785748075851 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.129 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_LOW -0.7 Sender listed at https://www.dnswl.org/, low 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: KDLW5HBA4IEXQ6KD5LEK44UXUAYWI74I X-Message-ID-Hash: KDLW5HBA4IEXQ6KD5LEK44UXUAYWI74I 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 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 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, @@ -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, 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