From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id C37A81FF0E0 for ; Thu, 23 Jul 2026 13:52:18 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id CB64F214CE; Thu, 23 Jul 2026 13:52:16 +0200 (CEST) MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: Re: [PATCH proxmox-backup v3 07/10] datastore: create lockdir with correct mode for backup user access From: Robert Obkircher To: Christian Ebner In-Reply-To: <20260721103643.333028-8-c.ebner@proxmox.com> References: <20260721103643.333028-1-c.ebner@proxmox.com> <20260721103643.333028-8-c.ebner@proxmox.com> Date: Thu, 23 Jul 2026 13:52:10 +0200 Message-Id: <178480753020.88450.17561424625137931984.b4-review@b4> X-Mailer: b4 0.16-dev X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784807503080 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.181 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: VRIKIIJFVGWB6X4ITOS5DEQTO2J4JSHH X-Message-ID-Hash: VRIKIIJFVGWB6X4ITOS5DEQTO2J4JSHH X-MailFrom: r.obkircher@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 CC: pbs-devel@lists.proxmox.com 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 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 > > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs > index 99adfde8a..3550f0963 100644 > --- a/pbs-datastore/src/datastore.rs > +++ b/pbs-datastore/src/datastore.rs > @@ -3162,6 +3162,7 @@ impl DataStore { > let (mut config, _digest) = pbs_config::datastore::config()?; > let mut datastore_config: DataStoreConfig = config.lookup("datastore", name)?; > > + let maintenance_mode_lock = crate::maintenance_mode_lock(name, &config_lock)?; > datastore_config.set_maintenance_mode(Some(MaintenanceMode { > ty: MaintenanceType::Delete, > message: None, > @@ -3169,6 +3170,7 @@ impl DataStore { > > config.set_data(name, "datastore", &datastore_config)?; > pbs_config::datastore::save_config(&config)?; > + drop(maintenance_mode_lock); > drop(config_lock); > > let (operations, _lock) = task_tracking::get_active_operations_locked(name)?; > diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs > index 187de23f6..99f954f19 100644 > --- a/pbs-datastore/src/lib.rs > +++ b/pbs-datastore/src/lib.rs > @@ -159,8 +159,9 @@ > > use std::os::unix::io::AsRawFd; > use std::path::Path; > +use std::time::Duration; > > -use anyhow::{Error, bail}; > +use anyhow::{Context, Error, bail}; > > use proxmox_sys::fs::CreateOptions; > > @@ -282,3 +283,21 @@ where > > Ok(lock) > } > + > +/// Acquire an exclusive lock for the datastore's maintenance-mode. The datastore config lock > +/// must be acquired beforehand to avoid races. > +pub fn maintenance_mode_lock( > + store: &str, > + _datastore_config_lock: &BackupLockGuard, > +) -> Result { > + let mut lock_path = Path::new(DATASTORE_LOCKS_DIR).join(store); > + lock_path.push("maintenance-mode.lck"); > + > + lock_helper(store, &lock_path, |p| { > + // never wait for the lock here: this is only acquired by long running operations > + // and the datastore config lock must be held anyways. Blocking the it longer than > + // necessary is not acceptable. > + pbs_config::open_backup_lockfile(p, Some(Duration::from_secs(0)), true) > + .context("unable to acquire exclusive datastore's maintenance-mode lock") > + }) > +} Since those locks are used together it would be nicer to turn this into a MaintenanceModeLock(BackupLockGuard) wrapper for some additional type safety. -- Robert Obkircher