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 4DFD91FF163
for <inbox@lore.proxmox.com>; Thu, 21 Nov 2024 14:19:33 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 2100D1DFCF;
Thu, 21 Nov 2024 14:19:41 +0100 (CET)
Date: Thu, 21 Nov 2024 14:19:33 +0100
From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= <f.gruenbichler@proxmox.com>
To: Proxmox Backup Server development discussion <pbs-devel@lists.proxmox.com>
References: <20241113150102.164820-1-h.laimer@proxmox.com>
<20241113150102.164820-7-h.laimer@proxmox.com>
In-Reply-To: <20241113150102.164820-7-h.laimer@proxmox.com>
MIME-Version: 1.0
User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid)
Message-Id: <1732193726.vmgtl8o1rq.astroid@yuna.none>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.046 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. [proxmox.com, lib.rs, maintenance.rs, mod.rs, datastore.rs]
Subject: Re: [pbs-devel] [PATCH proxmox-backup v13 06/26] datastore: add
helper for checking if a datastore is mounted
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>
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 November 13, 2024 4:00 pm, Hannes Laimer wrote:
> ... at a specific location. This is removable datastore specific so it
> takes both a uuid and mount location.
>
> Co-authored-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> changes since v12:
> * clearify documentation
> * make function more removable datastore specific to remove ambiguity
> about what it does and what it is meant for
> * only use for removable datastore
>
> pbs-api-types/src/maintenance.rs | 2 +
> pbs-datastore/src/datastore.rs | 73 +++++++++++++++++++++++++++++
> pbs-datastore/src/lib.rs | 2 +-
> src/server/metric_collection/mod.rs | 10 ++++
> 4 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
> index fd4d3416..9f51292e 100644
> --- a/pbs-api-types/src/maintenance.rs
> +++ b/pbs-api-types/src/maintenance.rs
> @@ -82,6 +82,8 @@ impl MaintenanceMode {
> /// task finishes, so all open files are closed.
> pub fn is_offline(&self) -> bool {
> self.ty == MaintenanceType::Offline
> + || self.ty == MaintenanceType::Unmount
> + || self.ty == MaintenanceType::Delete
> }
>
this part here doesn't really belong into this commit (it's not part of
the helper mentioned above). the comment and the (new) contents also
don't match - if the MaintenanceType is Delete (i.e., the datastore
contents are currently being deleted) then all open files can't be
already closed? same for Unmount - if it is currently being unmounted,
there might still be references open..
I think this should rather be explicit -> at the end of
unmounting/deletion, remove from cache? or this helper should be renamed
and the comment adapted, if it is actually not "is the datastore
offline" but "is it a removal candidate once all tasks have exited"..
> pub fn check(&self, operation: Option<Operation>) -> Result<(), Error> {
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index fb37bd5a..cadf9245 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1,5 +1,6 @@
> use std::collections::{HashMap, HashSet};
> use std::io::{self, Write};
> +use std::os::unix::ffi::OsStrExt;
> use std::os::unix::io::AsRawFd;
> use std::path::{Path, PathBuf};
> use std::sync::{Arc, LazyLock, Mutex};
> @@ -14,6 +15,7 @@ use proxmox_schema::ApiType;
> use proxmox_sys::error::SysError;
> use proxmox_sys::fs::{file_read_optional_string, replace_file, CreateOptions};
> use proxmox_sys::fs::{lock_dir_noblock, DirLockGuard};
> +use proxmox_sys::linux::procfs::MountInfo;
> use proxmox_sys::process_locker::ProcessLockSharedGuard;
> use proxmox_worker_task::WorkerTaskContext;
>
> @@ -46,6 +48,55 @@ pub fn check_backup_owner(owner: &Authid, auth_id: &Authid) -> Result<(), Error>
> Ok(())
> }
>
> +/// Check if a device with a given UUID is currently mounted at store_mount_point by
> +/// comparing the `st_rdev` values of `/dev/disk/by-uuid/<uuid>` and the source device in
> +/// /proc/self/mountinfo.
> +///
> +/// If we can't check if it is mounted, we treat that as not mounted,
> +/// returning false.
> +///
> +/// Reasons it could fail other than not being mounted where expected:
> +/// - could not read /proc/self/mountinfo
> +/// - could not stat /dev/disk/by-uuid/<uuid>
> +/// - /dev/disk/by-uuid/<uuid> is not a block device
> +///
> +/// Since these are very much out of our control, there is no real value in distinguishing
> +/// between them, so for this function they all are treated as 'device not mounted'
> +pub fn is_datastore_mounted_at(store_mount_point: String, device_uuid: String) -> bool {
> + use nix::sys::stat::SFlag;
> +
> + let store_mount_point = Path::new(&store_mount_point);
> +
> + let dev_node = match nix::sys::stat::stat(format!("/dev/disk/by-uuid/{device_uuid}").as_str()) {
> + Ok(stat) if SFlag::from_bits_truncate(stat.st_mode) == SFlag::S_IFBLK => stat.st_rdev,
> + _ => return false,
> + };
> +
> + let Ok(mount_info) = MountInfo::read() else {
> + return false;
> + };
> +
> + for (_, entry) in mount_info {
> + let Some(source) = entry.mount_source else {
> + continue;
> + };
> +
> + if entry.mount_point != store_mount_point || !source.as_bytes().starts_with(b"/") {
> + continue;
> + }
> +
> + if let Ok(stat) = nix::sys::stat::stat(source.as_os_str()) {
> + let sflag = SFlag::from_bits_truncate(stat.st_mode);
> +
> + if sflag == SFlag::S_IFBLK && stat.st_rdev == dev_node {
> + return true;
> + }
> + }
> + }
> +
> + false
> +}
> +
> /// Datastore Management
> ///
> /// A Datastore can store severals backups, and provides the
> @@ -154,6 +205,18 @@ impl DataStore {
> bail!("datastore '{name}' is in {error}");
> }
> }
> + let mount_status = config
> + .get_mount_point()
> + .zip(config.backing_device.as_ref())
> + .map(|(mount_point, device_uuid)| {
> + is_datastore_mounted_at(mount_point, device_uuid.to_string())
> + });
> +
> + if mount_status == Some(false) {
> + let mut datastore_cache = DATASTORE_MAP.lock().unwrap();
> + datastore_cache.remove(&config.name);
> + bail!("Removable Datastore is not mounted");
note the message here
> + }
>
> let mut datastore_cache = DATASTORE_MAP.lock().unwrap();
> let entry = datastore_cache.get(name);
> @@ -258,6 +321,16 @@ impl DataStore {
> ) -> Result<Arc<Self>, Error> {
> let name = config.name.clone();
>
> + let mount_status = config
> + .get_mount_point()
> + .zip(config.backing_device.as_ref())
> + .map(|(mount_point, device_uuid)| {
> + is_datastore_mounted_at(mount_point, device_uuid.to_string())
> + });
> + if mount_status == Some(false) {
> + bail!("Datastore is not available")
and here - shouldn't they be the same? could also be combined into a
helper (e.g., ensure_removable_datastore_is_mounted), if desired, there
is a third call site with the exact same code but yet another error
message in src/api2/admin/datastore.rs (and three more slight variations
where it's not (always) fatal that it is not mounted, but with the same
zip+map code before).
> + }
> +
> let tuning: DatastoreTuning = serde_json::from_value(
> DatastoreTuning::API_SCHEMA
> .parse_property_string(config.tuning.as_deref().unwrap_or(""))?,
> diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
> index 202b0955..34113261 100644
> --- a/pbs-datastore/src/lib.rs
> +++ b/pbs-datastore/src/lib.rs
> @@ -204,7 +204,7 @@ pub use manifest::BackupManifest;
> pub use store_progress::StoreProgress;
>
> mod datastore;
> -pub use datastore::{check_backup_owner, DataStore};
> +pub use datastore::{check_backup_owner, is_datastore_mounted_at, DataStore};
>
> mod hierarchy;
> pub use hierarchy::{
> diff --git a/src/server/metric_collection/mod.rs b/src/server/metric_collection/mod.rs
> index b95dba20..edba512c 100644
> --- a/src/server/metric_collection/mod.rs
> +++ b/src/server/metric_collection/mod.rs
> @@ -176,6 +176,16 @@ fn collect_disk_stats_sync() -> (DiskStat, Vec<DiskStat>) {
> continue;
> }
>
> + let mount_status = config
> + .get_mount_point()
> + .zip(config.backing_device.as_ref())
> + .map(|(mount_point, device_uuid)| {
> + pbs_datastore::is_datastore_mounted_at(mount_point, device_uuid.to_string())
> + });
> + if mount_status == Some(false) {
> + continue;
> + }
> +
this one of those variations.. since all call sites to
is_datastore_mounted_at start with the config, it might make more sense
to implement it there (or rather, using that as parameter, since we
don't want all that stat code in an api type ;)) and avoid all the
repetitive code?
then a simple get_datastore_mount_status(&config) -> Option(bool), and
maybe ensure_datastore_is_mounted(&config) -> Result<(), Error> would
solve the same issue, but be nicer to read?
> datastores.push(gather_disk_stats(
> disk_manager.clone(),
> Path::new(&config.absolute_path()),
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel