Re: [PATCH proxmox v2 05/25] disks: import from Proxmox Backup Server

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 20.03.26 um 11:23 schrieb Lukas Wagner:
> This is based on the disks module from PBS and left unchanged.
> 
> The version has not been set to 1.0 yet since it seems like this crate
> could use a bit a cleanup (custom error type instead of anyhow,
> documentation).

+1, and also some other clean up for pre-existing pain points w.r.t.
S.M.A.R.T, see inline.


> ---
>  Cargo.toml                         |    6 +
>  proxmox-disks/Cargo.toml           |   30 +
>  proxmox-disks/debian/changelog     |    5 +
>  proxmox-disks/debian/control       |   94 ++
>  proxmox-disks/debian/copyright     |   18 +
>  proxmox-disks/debian/debcargo.toml |    7 +
>  proxmox-disks/src/lib.rs           | 1396 ++++++++++++++++++++++++++++
>  proxmox-disks/src/lvm.rs           |   60 ++
>  proxmox-disks/src/parse_helpers.rs |   52 ++
>  proxmox-disks/src/smart.rs         |  227 +++++
>  proxmox-disks/src/zfs.rs           |  205 ++++
>  proxmox-disks/src/zpool_list.rs    |  294 ++++++
>  proxmox-disks/src/zpool_status.rs  |  496 ++++++++++
>  13 files changed, 2890 insertions(+)
>  create mode 100644 proxmox-disks/Cargo.toml
>  create mode 100644 proxmox-disks/debian/changelog
>  create mode 100644 proxmox-disks/debian/control
>  create mode 100644 proxmox-disks/debian/copyright
>  create mode 100644 proxmox-disks/debian/debcargo.toml
>  create mode 100644 proxmox-disks/src/lib.rs
>  create mode 100644 proxmox-disks/src/lvm.rs
>  create mode 100644 proxmox-disks/src/parse_helpers.rs
>  create mode 100644 proxmox-disks/src/smart.rs
>  create mode 100644 proxmox-disks/src/zfs.rs
>  create mode 100644 proxmox-disks/src/zpool_list.rs
>  create mode 100644 proxmox-disks/src/zpool_status.rs
> 


> diff --git a/proxmox-disks/src/lib.rs b/proxmox-disks/src/lib.rs
> new file mode 100644
> index 00000000..e6056c14
> --- /dev/null
> +++ b/proxmox-disks/src/lib.rs

[...]

> +#[api(
> +    properties: {
> +        used: {
> +            type: DiskUsageType,
> +        },
> +        "disk-type": {
> +            type: DiskType,
> +        },
> +        status: {
> +            type: SmartStatus,
> +        },
> +        partitions: {
> +            optional: true,
> +            items: {
> +                type: PartitionInfo
> +            }
> +        }
> +    }
> +)]
> +#[derive(Debug, Serialize, Deserialize)]
> +#[serde(rename_all = "kebab-case")]
> +/// Information about how a Disk is used
> +pub struct DiskUsageInfo {
> +    /// Disk name (`/sys/block/<name>`)
> +    pub name: String,
> +    pub used: DiskUsageType,
> +    pub disk_type: DiskType,
> +    pub status: SmartStatus,

pre-existing, I know, but after taking a third look I remembered the s.m.a.r.t slow
saga we had a while ago [0], and I think factoring this out would be a good time to
finally address the bigger pain points of that. One would be to make this optional
or drop this from the struct completely.

[0]: https://lore.proxmox.com/all/7b0ac740-096b-4850-a81d-5fa6bc9c347a@proxmox.com/

> +    /// Disk wearout
> +    pub wearout: Option<f64>,
> +    /// Vendor
> +    pub vendor: Option<String>,
> +    /// Model
> +    pub model: Option<String>,
> +    /// WWN
> +    pub wwn: Option<String>,
> +    /// Disk size
> +    pub size: u64,
> +    /// Serisal number
> +    pub serial: Option<String>,
> +    /// Partitions on the device
> +    pub partitions: Option<Vec<PartitionInfo>>,
> +    /// Linux device path (/dev/xxx)
> +    pub devpath: Option<String>,
> +    /// Set if disk contains a GPT partition table
> +    pub gpt: bool,
> +    /// RPM
> +    pub rpm: Option<u64>,
> +}
> +

[...]

> +/// Get disk usage information for multiple disks
> +fn get_disks(
> +    // filter - list of device names (without leading /dev)
> +    disks: Option<Vec<String>>,
> +    // do no include data from smartctl
> +    no_smart: bool,
> +    // include partitions
> +    include_partitions: bool,
> +) -> Result<HashMap<String, DiskUsageInfo>, Error> {
> +    let disk_manager = DiskManage::new();
> +
> +    let lsblk_info = get_lsblk_info()?;
> +
> +    let zfs_devices =
> +        zfs_devices(&lsblk_info, None).or_else(|err| -> Result<HashSet<u64>, Error> {
> +            eprintln!("error getting zfs devices: {err}");
> +            Ok(HashSet::new())
> +        })?;
> +
> +    let lvm_devices = get_lvm_devices(&lsblk_info)?;
> +
> +    let file_system_devices = get_file_system_devices(&lsblk_info)?;
> +
> +    // fixme: ceph journals/volumes
> +
> +    let mut result = HashMap::new();
> +    let mut device_paths = Vec::new();
> +
> +    for item in proxmox_sys::fs::scan_subdir(libc::AT_FDCWD, "/sys/block", &BLOCKDEVICE_NAME_REGEX)?
> +    {
> +        let item = item?;
> +
> +        let name = item.file_name().to_str().unwrap().to_string();
> +
> +        if let Some(ref disks) = disks {
> +            if !disks.contains(&name) {
> +                continue;
> +            }
> +        }
> +
> +        let sys_path = format!("/sys/block/{name}");
> +
> +        if let Ok(target) = std::fs::read_link(&sys_path) {
> +            if let Some(target) = target.to_str() {
> +                if ISCSI_PATH_REGEX.is_match(target) {
> +                    continue;
> +                } // skip iSCSI devices
> +            }
> +        }
> +
> +        let disk = disk_manager.clone().disk_by_sys_path(&sys_path)?;
> +
> +        let devnum = disk.devnum()?;
> +
> +        let size = match disk.size() {
> +            Ok(size) => size,
> +            Err(_) => continue, // skip devices with unreadable size
> +        };
> +
> +        let disk_type = match disk.guess_disk_type() {
> +            Ok(disk_type) => disk_type,
> +            Err(_) => continue, // skip devices with undetectable type
> +        };
> +
> +        let mut usage = DiskUsageType::Unused;
> +
> +        if lvm_devices.contains(&devnum) {
> +            usage = DiskUsageType::LVM;
> +        }
> +
> +        match disk.is_mounted() {
> +            Ok(true) => usage = DiskUsageType::Mounted,
> +            Ok(false) => {}
> +            Err(_) => continue, // skip devices with undetectable mount status
> +        }
> +
> +        if zfs_devices.contains(&devnum) {
> +            usage = DiskUsageType::ZFS;
> +        }
> +
> +        let vendor = disk
> +            .vendor()
> +            .unwrap_or(None)
> +            .map(|s| s.to_string_lossy().trim().to_string());
> +
> +        let model = disk.model().map(|s| s.to_string_lossy().into_owned());
> +
> +        let serial = disk.serial().map(|s| s.to_string_lossy().into_owned());
> +
> +        let devpath = disk
> +            .device_path()
> +            .map(|p| p.to_owned())
> +            .map(|p| p.to_string_lossy().to_string());
> +
> +        device_paths.push((name.clone(), devpath.clone()));
> +
> +        let wwn = disk.wwn().map(|s| s.to_string_lossy().into_owned());
> +
> +        let partitions: Option<Vec<PartitionInfo>> = if include_partitions {
> +            disk.partitions().map_or(None, |parts| {
> +                Some(get_partitions_info(
> +                    parts,
> +                    &lvm_devices,
> +                    &zfs_devices,
> +                    &file_system_devices,
> +                    &lsblk_info,
> +                ))
> +            })
> +        } else {
> +            None
> +        };
> +
> +        if usage != DiskUsageType::Mounted {
> +            match scan_partitions(disk_manager.clone(), &lvm_devices, &zfs_devices, &name) {
> +                Ok(part_usage) => {
> +                    if part_usage != DiskUsageType::Unused {
> +                        usage = part_usage;
> +                    }
> +                }
> +                Err(_) => continue, // skip devices if scan_partitions fail
> +            };
> +        }
> +
> +        if usage == DiskUsageType::Unused && file_system_devices.contains(&devnum) {
> +            usage = DiskUsageType::FileSystem;
> +        }
> +
> +        if usage == DiskUsageType::Unused && disk.has_holders()? {
> +            usage = DiskUsageType::DeviceMapper;
> +        }
> +
> +        let info = DiskUsageInfo {
> +            name: name.clone(),
> +            vendor,
> +            model,
> +            partitions,
> +            serial,
> +            devpath,
> +            size,
> +            wwn,
> +            disk_type,
> +            status: SmartStatus::Unknown,
> +            wearout: None,
> +            used: usage,
> +            gpt: disk.has_gpt(),
> +            rpm: disk.ata_rotation_rate_rpm(),
> +        };
> +
> +        result.insert(name, info);
> +    }
> +
> +    if !no_smart {

I still think such a branch should simple not exist for a get_disk (list) method, like
mentioned in (potentially now slightly outdated) [0].

> +        let (tx, rx) = crossbeam_channel::bounded(result.len());
> +
> +        let parallel_handler =
> +            ParallelHandler::new("smartctl data", 4, move |device: (String, String)| {
> +                match get_smart_data(Path::new(&device.1), false) {
> +                    Ok(smart_data) => tx.send((device.0, smart_data))?,
> +                    // do not fail the whole disk output just because smartctl couldn't query one
> +                    Err(err) => {
> +                        proxmox_log::error!("failed to gather smart data for {} – {err}", device.1)
> +                    }
> +                }
> +                Ok(())
> +            });
> +
> +        for (name, path) in device_paths.into_iter() {
> +            if let Some(p) = path {
> +                parallel_handler.send((name, p))?;
> +            }
> +        }
> +
> +        parallel_handler.complete()?;
> +        while let Ok(msg) = rx.recv() {
> +            if let Some(value) = result.get_mut(&msg.0) {
> +                value.wearout = msg.1.wearout;
> +                value.status = msg.1.status;
> +            }
> +        }
> +    }
> +    Ok(result)
> +}
> +