From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id ED9B51FF16B
for <inbox@lore.proxmox.com>; Thu, 20 Mar 2025 12:57:15 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 6C525334BC;
Thu, 20 Mar 2025 12:57:15 +0100 (CET)
Date: Thu, 20 Mar 2025 12:57:12 +0100
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Hannes Laimer <h.laimer@proxmox.com>
Message-ID: <ycpmovdfrdqfkuhgsa4sbafto55bbtbiassq2jd6iczy4ocn6m@x2dnxvc7nw7j>
References: <20250320094851.36395-1-h.laimer@proxmox.com>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <20250320094851.36395-1-h.laimer@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.079 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [datastore.rs]
Subject: Re: [pbs-devel] [PATCH proxmox-backup] api: config: use guard for
unmounting on failed datastore creation
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Cc: pbs-devel@lists.proxmox.com
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.proxmox.com>
On Thu, Mar 20, 2025 at 10:48:51AM +0100, Hannes Laimer wrote:
> Currently if any `?`/`bail!` happens between mounting and completing
> the creation process unmounting will be skipped. Adding this guard
> solves that problem and makes it easier to add things in the future
> without having to worry about a disk not being unmounted in case of a
> failed creation.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> Reported-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
> src/api2/config/datastore.rs | 86 ++++++++++++++++++++++--------------
> 1 file changed, 52 insertions(+), 34 deletions(-)
>
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index fe3260f6d..99c699814 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -1,7 +1,7 @@
> use std::path::{Path, PathBuf};
>
> use ::serde::{Deserialize, Serialize};
> -use anyhow::{bail, format_err, Error};
> +use anyhow::{bail, Error};
> use hex::FromHex;
> use serde_json::Value;
> use tracing::warn;
> @@ -70,6 +70,33 @@ pub fn list_datastores(
> Ok(list.into_iter().filter(filter_by_privs).collect())
> }
>
> +struct UnmountGuard {
> + path: PathBuf,
> + should_unmount: bool,
Instead of the extra `bool` it would be cheaper if we just make `path`
an `Option<PathBuf>`.
> +}
> +
> +impl UnmountGuard {
> + fn new(path: PathBuf, should_unmount: bool) -> Self {
> + UnmountGuard {
> + path,
> + should_unmount,
> + }
> + }
> + fn disable(&mut self) {
This could take `self` by-move since we don't intend to re-enable it and
then we don't need the guard to be mutable down where it's used.
> + self.should_unmount = false;
> + }
> +}
> +
> +impl Drop for UnmountGuard {
> + fn drop(&mut self) {
> + if self.should_unmount {
> + if let Err(e) = unmount_by_mountpoint(&self.path) {
> + warn!("could not unmount device: {e}");
> + }
> + }
> + }
> +}
> +
> pub(crate) fn do_create_datastore(
> _lock: BackupLockGuard,
> mut config: SectionConfigData,
> @@ -87,59 +114,50 @@ pub(crate) fn do_create_datastore(
> param_bail!("path", err);
> }
>
> - let need_unmount = datastore.backing_device.is_some();
> - if need_unmount {
> - do_mount_device(datastore.clone())?;
> - };
> -
> let tuning: DatastoreTuning = serde_json::from_value(
> DatastoreTuning::API_SCHEMA
> .parse_property_string(datastore.tuning.as_deref().unwrap_or(""))?,
> )?;
>
> - let res = if reuse_datastore {
> - ChunkStore::verify_chunkstore(&path)
> + let mut unmount_guard = if datastore.backing_device.is_some() {
> + do_mount_device(datastore.clone())?;
> + UnmountGuard::new(path.clone(), true)
> + } else {
> + UnmountGuard::new(path.clone(), false)
With `path` as `Option` this could be a cheap `UnmountGuard::new(None)`.
> + };
> +
> + if reuse_datastore {
> + ChunkStore::verify_chunkstore(&path)?;
> } else {
> - let mut is_empty = true;
> 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") {
> - is_empty = false;
> - break;
> + bail!("datastore path not empty");
> }
> }
> }
> - if is_empty {
> - 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(),
> - )
> - .map(|_| ())
> - } else {
> - Err(format_err!("datastore path not empty"))
> - }
> + 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(),
> + )
> + .map(|_| ())?;
> };
>
> - if res.is_err() {
> - if need_unmount {
> - if let Err(e) = unmount_by_mountpoint(&path) {
> - warn!("could not unmount device: {e}");
> - }
> - }
> - return res;
> - }
> -
> config.set_data(&datastore.name, "datastore", &datastore)?;
>
> pbs_config::datastore::save_config(&config)?;
>
> - jobstate::create_state_file("garbage_collection", &datastore.name)
> + jobstate::create_state_file("garbage_collection", &datastore.name)?;
> +
> + unmount_guard.disable();
> +
> + Ok(())
> }
>
> #[api(
> --
> 2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel