[pbs-devel] [PATCH proxmox-backup v12 06/26] datastore: add helper for checking if a removable datastore is available
Fabian Grünbichler
f.gruenbichler at proxmox.com
Mon Oct 14 15:42:37 CEST 2024
On September 4, 2024 4:11 pm, Hannes Laimer wrote:
> Co-authored-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
> Signed-off-by: Hannes Laimer <h.laimer at proxmox.com>
> ---
> 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<Operation>) -> 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/<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<Arc<Self>, 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<DiskStat>) {
> {
> 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 at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
More information about the pbs-devel
mailing list