Re: [pbs-devel] [PATCH proxmox-backup v13 06/26] datastore: add helper for checking if a datastore is mounted

["Fabian Grünbichler" <f.gruenbichler@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