From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 4DFD91FF163 for ; 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?= To: Proxmox Backup Server development discussion 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 > Signed-off-by: Hannes Laimer > --- > 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) -> 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/` 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/ > +/// - /dev/disk/by-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, 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) { > 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