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 E787B1FF168 for ; Mon, 14 Oct 2024 15:42:11 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E21F035F8C; Mon, 14 Oct 2024 15:42:43 +0200 (CEST) Date: Mon, 14 Oct 2024 15:42:37 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20240904141155.350454-1-h.laimer@proxmox.com> <20240904141155.350454-7-h.laimer@proxmox.com> In-Reply-To: <20240904141155.350454-7-h.laimer@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1728907148.brs6f9l2g5.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup v12 06/26] datastore: add helper for checking if a removable datastore is available 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 September 4, 2024 4:11 pm, Hannes Laimer wrote: > Co-authored-by: Wolfgang Bumiller > Signed-off-by: Hannes Laimer > --- > pbs-api-types/src/maintenance.rs | 2 ++ > pbs-datastore/src/datastore.rs | 58 ++++++++++++++++++++++++++++++++ > pbs-datastore/src/lib.rs | 2 +- > src/bin/proxmox-backup-proxy.rs | 5 ++- > 4 files changed, 65 insertions(+), 2 deletions(-) > > 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 > } > > pub fn check(&self, operation: Option) -> Result<(), Error> { > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs > index fb37bd5a..29f98b37 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,52 @@ pub fn check_backup_owner(owner: &Authid, auth_id: &Authid) -> Result<(), Error> > Ok(()) > } > > +/// check if a removable datastore is currently available/mounted by > +/// comparing the `st_rdev` values of `/dev/disk/by-uuid/` and the source device in > +/// /proc/self/mountinfo check if a *removable* datastore is .. > +pub fn is_datastore_available(config: &DataStoreConfig) -> bool { > + use nix::sys::stat::SFlag; > + > + let uuid = match config.backing_device.as_deref() { > + Some(dev) => dev, > + None => return true, returns true if not a removable datastore? > + }; > + > + let Some(store_mount_point) = config.get_mount_point() else { > + return true; same here. also see further below - this should either be removable datastore specific, in which case it could take uuid and mountpoint as parameters, or it could take any DataStoreConfig, then the doc comment should reflect that and clearly describe the semantics.. > + }; > + let store_mount_point = Path::new(&store_mount_point); > + > + let dev_node = match nix::sys::stat::stat(format!("/dev/disk/by-uuid/{uuid}").as_str()) { > + Ok(stat) if SFlag::from_bits_truncate(stat.st_mode) == SFlag::S_IFBLK => stat.st_rdev, > + _ => return false, shouldn't this differentiate between: - stat failed with ENOENT - stat failed for other reasons - stat worked but result is not as expected ? also, that code (continued below at (A)) > + }; > + > + let Ok(mount_info) = MountInfo::read() else { > + return false; shouldn't this be an error? > + }; > + > + 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; > + } (A) and this code could go into a helper.. > + } > + } > + > + false > +} > + > /// Datastore Management > /// > /// A Datastore can store severals backups, and provides the > @@ -155,6 +203,12 @@ impl DataStore { > } > } > > + if config.backing_device.is_some() && !is_datastore_available(&config) { > + let mut datastore_cache = DATASTORE_MAP.lock().unwrap(); > + datastore_cache.remove(&config.name); > + bail!("Removable Datastore is not mounted"); > + } so here the helper is only called for removable datastores.. > + > let mut datastore_cache = DATASTORE_MAP.lock().unwrap(); > let entry = datastore_cache.get(name); > > @@ -258,6 +312,10 @@ impl DataStore { > ) -> Result, Error> { > let name = config.name.clone(); > > + if !is_datastore_available(&config) { > + bail!("Datastore is not available") > + } but here it is called for all datastores > + > 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..6d58f327 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_available, DataStore}; > > mod hierarchy; > pub use hierarchy::{ > diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs > index 6ccf2f96..58fb8b84 100644 > --- a/src/bin/proxmox-backup-proxy.rs > +++ b/src/bin/proxmox-backup-proxy.rs > @@ -23,7 +23,7 @@ use proxmox_sys::fs::{CreateOptions, FileSystemInformation}; > use proxmox_sys::linux::procfs::{Loadavg, ProcFsMemInfo, ProcFsNetDev, ProcFsStat}; > use proxmox_sys::logrotate::LogRotate; > > -use pbs_datastore::DataStore; > +use pbs_datastore::{is_datastore_available, DataStore}; > > use proxmox_rest_server::{ > cleanup_old_tasks, cookie_from_header, rotate_task_log_archive, ApiConfig, Redirector, > @@ -1132,6 +1132,9 @@ fn collect_disk_stats_sync() -> (DiskStat, Vec) { > { > continue; > } > + if !is_datastore_available(&config) { > + continue; > + } and here as well.. > let path = std::path::PathBuf::from(config.absolute_path()); > datastores.push(gather_disk_stats(disk_manager.clone(), &path, &config.name)); > } > -- > 2.39.2 > > > > _______________________________________________ > 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