[pbs-devel] [PATCH proxmox-backup 1/2] api: avoid retrieving lsblk result twice

[pbs-devel] [PATCH proxmox-backup 1/2] api: avoid retrieving lsblk result twice

[Gabriel Goller]

Avoid running `lsblk` twice when executing the `list_disk`
endpoint/command. This and the various other small nits improve the
performance of the endpoint.

Does not really fix, but is related to: #4961.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 src/api2/node/disks/mod.rs    |  6 +++++-
 src/tools/disks/mod.rs        | 10 +++++++---
 src/tools/disks/smart.rs      | 10 +++-------
 src/tools/disks/zfs.rs        | 15 ++++++++-------
 src/tools/disks/zpool_list.rs |  2 +-
 5 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/src/api2/node/disks/mod.rs b/src/api2/node/disks/mod.rs
index 4ef4ee2b8ac7..abcb8ee40ef1 100644
--- a/src/api2/node/disks/mod.rs
+++ b/src/api2/node/disks/mod.rs
@@ -115,7 +115,11 @@ pub fn smart_status(disk: String, healthonly: Option<bool>) -> Result<SmartData,
 
     let manager = DiskManage::new();
     let disk = manager.disk_by_name(&disk)?;
-    get_smart_data(&disk, healthonly)
+    if let Some(path) = disk.device_path() {
+        get_smart_data(path, healthonly)
+    } else {
+        bail!("disk {:?} has no node in /dev", disk.syspath());
+    }
 }
 
 #[api(
diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index c729c26a20a9..04f62b818238 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -863,8 +863,8 @@ fn get_partitions_info(
     lvm_devices: &HashSet<u64>,
     zfs_devices: &HashSet<u64>,
     file_system_devices: &HashSet<u64>,
+    lsblk_infos: &[LsblkInfo],
 ) -> Vec<PartitionInfo> {
-    let lsblk_infos = get_lsblk_info().ok();
     partitions
         .values()
         .map(|disk| {
@@ -887,8 +887,8 @@ fn get_partitions_info(
 
             let mounted = disk.is_mounted().unwrap_or(false);
             let mut filesystem = None;
-            if let (Some(devpath), Some(infos)) = (devpath.as_ref(), lsblk_infos.as_ref()) {
-                for info in infos.iter().filter(|i| i.path.eq(devpath)) {
+            if let Some(devpath) = devpath.as_ref() {
+                for info in lsblk_infos.iter().filter(|i| i.path.eq(devpath)) {
                     used = match info.partition_type.as_deref() {
                         Some("21686148-6449-6e6f-744e-656564454649") => PartitionUsageType::BIOS,
                         Some("c12a7328-f81f-11d2-ba4b-00a0c93ec93b") => PartitionUsageType::EFI,
@@ -942,6 +942,7 @@ fn get_disks(
     // 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)?
     {
@@ -1009,6 +1010,8 @@ fn get_disks(
             .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 {
@@ -1018,6 +1021,7 @@ fn get_disks(
                     &lvm_devices,
                     &zfs_devices,
                     &file_system_devices,
+                    &lsblk_info,
                 ))
             })
         } else {
diff --git a/src/tools/disks/smart.rs b/src/tools/disks/smart.rs
index 3ad782b7b248..4868815f6f32 100644
--- a/src/tools/disks/smart.rs
+++ b/src/tools/disks/smart.rs
@@ -1,8 +1,8 @@
-use std::collections::{HashMap, HashSet};
 use std::sync::LazyLock;
+use std::{collections::{HashMap, HashSet}, path::Path};
 
 use ::serde::{Deserialize, Serialize};
-use anyhow::{bail, Error};
+use anyhow::Error;
 
 use proxmox_schema::api;
 
@@ -76,7 +76,7 @@ pub struct SmartData {
 }
 
 /// Read smartctl data for a disk (/dev/XXX).
-pub fn get_smart_data(disk: &super::Disk, health_only: bool) -> Result<SmartData, Error> {
+pub fn get_smart_data(disk_path: &Path, health_only: bool) -> Result<SmartData, Error> {
     const SMARTCTL_BIN_PATH: &str = "smartctl";
 
     let mut command = std::process::Command::new(SMARTCTL_BIN_PATH);
@@ -85,10 +85,6 @@ pub fn get_smart_data(disk: &super::Disk, health_only: bool) -> Result<SmartData
         command.args(["-A", "-j"]);
     }
 
-    let disk_path = match disk.device_path() {
-        Some(path) => path,
-        None => bail!("disk {:?} has no node in /dev", disk.syspath()),
-    };
     command.arg(disk_path);
 
     let output = proxmox_sys::command::run_command(
diff --git a/src/tools/disks/zfs.rs b/src/tools/disks/zfs.rs
index 2abb5176c1e6..3b7da1540835 100644
--- a/src/tools/disks/zfs.rs
+++ b/src/tools/disks/zfs.rs
@@ -70,7 +70,7 @@ pub fn zfs_pool_stats(pool: &OsStr) -> Result<Option<BlockDevStat>, Error> {
 ///
 /// The set is indexed by using the unix raw device number (dev_t is u64)
 pub fn zfs_devices(lsblk_info: &[LsblkInfo], pool: Option<String>) -> Result<HashSet<u64>, Error> {
-    let list = zpool_list(pool, true)?;
+    let list = zpool_list(pool.as_ref(), true)?;
 
     let mut device_set = HashSet::new();
     for entry in list {
@@ -79,12 +79,13 @@ pub fn zfs_devices(lsblk_info: &[LsblkInfo], pool: Option<String>) -> Result<Has
             device_set.insert(meta.rdev());
         }
     }
-
-    for info in lsblk_info.iter() {
-        if let Some(partition_type) = &info.partition_type {
-            if ZFS_UUIDS.contains(partition_type.as_str()) {
-                let meta = std::fs::metadata(&info.path)?;
-                device_set.insert(meta.rdev());
+    if pool.is_none() {
+        for info in lsblk_info.iter() {
+            if let Some(partition_type) = &info.partition_type {
+                if ZFS_UUIDS.contains(partition_type.as_str()) {
+                    let meta = std::fs::metadata(&info.path)?;
+                    device_set.insert(meta.rdev());
+                }
             }
         }
     }
diff --git a/src/tools/disks/zpool_list.rs b/src/tools/disks/zpool_list.rs
index 83ce063dfe16..485a721b9865 100644
--- a/src/tools/disks/zpool_list.rs
+++ b/src/tools/disks/zpool_list.rs
@@ -136,7 +136,7 @@ fn parse_zpool_list(i: &str) -> Result<Vec<ZFSPoolInfo>, Error> {
 ///
 /// Devices are only included when run with verbose flags
 /// set. Without, device lists are empty.
-pub fn zpool_list(pool: Option<String>, verbose: bool) -> Result<Vec<ZFSPoolInfo>, Error> {
+pub fn zpool_list(pool: Option<&String>, verbose: bool) -> Result<Vec<ZFSPoolInfo>, Error> {
     // Note: zpools list verbose output can include entries for 'special', 'cache' and 'logs'
     // and maybe other things.
 
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 2/2] api: parallelize smartctl checks

[Gabriel Goller]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 3259 bytes --]

To improve the performance of the smartctl checks, especially when a lot
of disks are used, parallelize the checks using the `ParallelHandler`.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---

Benchmark:
Benchmark 1: proxmox-backup-manager disk list (parallel)
  Time (mean ± σ):      75.8 ms ±   5.7 ms    [User: 0.8 ms, System: 0.5 ms]
  Range (min … max):    63.4 ms …  95.3 ms    100 runs

Benchmark 2: proxmox-backup-manager disk list (sequential)
  Time (mean ± σ):     189.4 ms ±   6.7 ms    [User: 0.7 ms, System: 1.0 ms]
  Range (min … max):   178.8 ms … 223.0 ms    100 runs

Summary
  'proxmox-backup-manager disk list (parallel)' ran
    2.50 ± 0.21 times faster than 'proxmox-backup-manager disk list (sequential)'


 src/tools/disks/mod.rs | 42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index 04f62b818238..09a3003a9333 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -15,13 +15,15 @@ use once_cell::sync::OnceCell;
 use ::serde::{Deserialize, Serialize};
 
 use proxmox_lang::{io_bail, io_format_err};
+use proxmox_log::info;
 use proxmox_schema::api;
 use proxmox_sys::linux::procfs::{mountinfo::Device, MountInfo};
 
 use pbs_api_types::{BLOCKDEVICE_DISK_AND_PARTITION_NAME_REGEX, BLOCKDEVICE_NAME_REGEX};
 
+use crate::tools::parallel_handler::ParallelHandler;
+
 mod zfs;
-use tracing::info;
 pub use zfs::*;
 mod zpool_status;
 pub use zpool_status::*;
@@ -1047,16 +1049,6 @@ fn get_disks(
             usage = DiskUsageType::DeviceMapper;
         }
 
-        let mut status = SmartStatus::Unknown;
-        let mut wearout = None;
-
-        if !no_smart {
-            if let Ok(smart) = get_smart_data(&disk, false) {
-                status = smart.status;
-                wearout = smart.wearout;
-            }
-        }
-
         let info = DiskUsageInfo {
             name: name.clone(),
             vendor,
@@ -1067,8 +1059,8 @@ fn get_disks(
             size,
             wwn,
             disk_type,
-            status,
-            wearout,
+            status: SmartStatus::Unknown,
+            wearout: None,
             used: usage,
             gpt: disk.has_gpt(),
             rpm: disk.ata_rotation_rate_rpm(),
@@ -1077,6 +1069,30 @@ fn get_disks(
         result.insert(name, info);
     }
 
+    if !no_smart {
+        let (tx, rx) = crossbeam_channel::bounded(result.len());
+
+        let parallel_handler =
+            ParallelHandler::new("smartctl data", 4, move |device: (String, String)| {
+                let smart_data = get_smart_data(Path::new(&device.1), false)?;
+                tx.send((device.0, smart_data))?;
+                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)
 }
 
-- 
2.39.5




[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 2/2] api: parallelize smartctl checks

[Gabriel Goller]

Oops, the charset of this mail is messed up, will send a v2!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel