Add disk I/O stats #1376

samin-cf · 2024-11-02T23:18:20Z

This PR adds support to retrieve DiskUsage for any given Disk on Windows, Linux, and macOS. The implementation was manually tested on all three platforms by verifying the collected values against:

psutil
Activity Monitor on macOS
Built-in Disk Monitoring view in task manager
iostat on Linux

Part of #304

src/common/mod.rs

GuillaumeGomez · 2024-11-03T12:10:27Z

src/unix/apple/macos/disk.rs

+            // Many external drive vendors do not advertise their device's storage medium.
+            //
+            // In these cases, assuming that there were _any_ properties about them registered, we fallback
+            // to `HDD` when no storage medium is provided by the device instead of `Unknown`.


Why not Unknown? USB storage and other equivalents are not HDD after all, so I think Unknown would fit better in this case.

This was existing code that was just moved due to refactoring; I was hoping git blame -C would preserve the original author, but it doesn't seem to be doing so.

I do agree Unknown makes the most sense here and can make the change!

GuillaumeGomez · 2024-11-03T12:11:08Z

src/unix/freebsd/disk.rs

@@ -58,6 +58,11 @@ impl DiskInner {
            refresh_disk(self, &mut vfs)
        }
    }
+
+    pub(crate) fn usage(&self) -> DiskUsage {
+        // TODO: Until disk i/o stats are added, return the default


Thanks! I'll take a look at adding support for it once this PR is merged.

src/common/disk.rs

GuillaumeGomez · 2024-11-03T12:12:59Z

src/unknown/disk.rs

@@ -42,6 +42,10 @@ impl DiskInner {
    pub(crate) fn refresh(&mut self) -> bool {
        true
    }
+
+    pub(crate) fn usage(&self) -> DiskUsage {
+        unreachable!()


I'd prefer if it returned an empty DiskUsage if you don't mind. Like that we can eventually instantiate a Disk on all platforms and use its methods without worrying about whether or not it might panic.

Makes sense. I will update that along with the kind and name functions to not panic

GuillaumeGomez · 2024-11-03T12:16:19Z

src/windows/disk.rs

+    unsafe {
+        // See <https://learn.microsoft.com/en-us/windows/win32/api/winioctl/ni-winioctl-ioctl_disk_performance> for reference
+        DeviceIoControl(
+            handle.0,
+            IOCTL_DISK_PERFORMANCE,
+            None, // Must be None as per docs
+            0,
+            Some(&mut disk_perf as *mut _ as _),
+            size_of::<DISK_PERFORMANCE>() as u32,
+            Some(&mut bytes_returned),
+            None,
+        )
+    }
+    .map_err(|err| {
+        sysinfo_debug!("Error: DeviceIoControl(IOCTL_DISK_PERFORMANCE) = {:?}", err);
+        err
+    })
+    .ok()
+    .and_then(|_| {
+        disk_perf
+            .BytesRead
+            .try_into()
+            .ok()
+            .zip(disk_perf.BytesWritten.try_into().ok())
+    })


Seems a bit more complicated than needed. What do you think about:

Suggested change

unsafe {

// See <https://learn.microsoft.com/en-us/windows/win32/api/winioctl/ni-winioctl-ioctl_disk_performance> for reference

DeviceIoControl(

handle.0,

IOCTL_DISK_PERFORMANCE,

None, // Must be None as per docs

0,

Some(&mut disk_perf as *mut _ as _),

size_of::<DISK_PERFORMANCE>() as u32,

Some(&mut bytes_returned),

None,

)

}

.map_err(|err| {

sysinfo_debug!("Error: DeviceIoControl(IOCTL_DISK_PERFORMANCE) = {:?}", err);

err

})

.ok()

.and_then(|_| {

disk_perf

.BytesRead

.try_into()

.ok()

.zip(disk_perf.BytesWritten.try_into().ok())

})

unsafe {

// See <https://learn.microsoft.com/en-us/windows/win32/api/winioctl/ni-winioctl-ioctl_disk_performance> for reference

DeviceIoControl(

handle.0,

IOCTL_DISK_PERFORMANCE,

None, // Must be None as per docs

0,

Some(&mut disk_perf as *mut _ as _),

size_of::<DISK_PERFORMANCE>() as u32,

Some(&mut bytes_returned),

None,

)

}

.map_err(|err| {

sysinfo_debug!("Error: DeviceIoControl(IOCTL_DISK_PERFORMANCE) = {:?}", err);

err

})

.ok()?;

Some((disk_perf.BytesRead.try_into().ok()?, disk_perf.BytesWritten.try_into().ok()?))

GuillaumeGomez · 2024-11-03T12:17:17Z

Looks very promising! A few things to change and then I'll merge.

samin-cf · 2024-11-03T16:20:20Z

Looks very promising! A few things to change and then I'll merge.

Thanks! Any idea when the next release (after this change is merged) might be? We are looking to make use of the disk I/O functionality in the next week or so, and it would be great to have this available.

GuillaumeGomez · 2024-11-03T18:54:53Z

Someone wants to work on #1375 and I'd like for #1355 to be done as well. After that, just the time for me to write the changelog and then I'll make the release.

GuillaumeGomez · 2024-11-03T19:04:25Z

src/unix/apple/macos/disk.rs

+        let disk_type = unsafe {
+            super::disk::get_str_value(
+                properties.inner(),
+                DictKey::Defined(ffi::kIOPropertyMediumTypeKey),
+            )
+        };
+
+        if let Some(disk_type) = disk_type.and_then(|medium| match medium.as_str() {
+            _ if medium == ffi::kIOPropertyMediumTypeSolidStateKey => Some(DiskKind::SSD),
+            _ if medium == ffi::kIOPropertyMediumTypeRotationalKey => Some(DiskKind::HDD),
+            _ => None,
+        }) {
+            Some(disk_type)
+        } else {
+            Some(DiskKind::Unknown(-1))
+        }


What do you think about:

Suggested change

let disk_type = unsafe {

super::disk::get_str_value(

properties.inner(),

DictKey::Defined(ffi::kIOPropertyMediumTypeKey),

)

};

if let Some(disk_type) = disk_type.and_then(|medium| match medium.as_str() {

_ if medium == ffi::kIOPropertyMediumTypeSolidStateKey => Some(DiskKind::SSD),

_ if medium == ffi::kIOPropertyMediumTypeRotationalKey => Some(DiskKind::HDD),

_ => None,

}) {

Some(disk_type)

} else {

Some(DiskKind::Unknown(-1))

}

let disk_type = unsafe {

super::disk::get_str_value(

properties.inner(),

DictKey::Defined(ffi::kIOPropertyMediumTypeKey),

)

}?;

match medium.as_str() {

_ if medium == ffi::kIOPropertyMediumTypeSolidStateKey => Some(DiskKind::SSD),

_ if medium == ffi::kIOPropertyMediumTypeRotationalKey => Some(DiskKind::HDD),

_ => Some(DiskKind::Unknown(-1)),

}

Much easier to reason with, thanks. I will update!

GuillaumeGomez · 2024-11-03T19:06:39Z

src/unix/apple/macos/disk.rs

+            super::disk::get_int_value(
+                properties.inner(),
+                DictKey::Defined(ffi::kIOBlockStorageDriverStatisticsBytesReadKey),
+            )
+            .zip(super::disk::get_int_value(
+                properties.inner(),
+                DictKey::Defined(ffi::kIOBlockStorageDriverStatisticsBytesWrittenKey),
+            ))
+        }
+        .and_then(|(read_bytes, written_bytes)| {
+            read_bytes
+                .try_into()
+                .ok()
+                .zip(written_bytes.try_into().ok())
+        })


Suggested change

super::disk::get_int_value(

properties.inner(),

DictKey::Defined(ffi::kIOBlockStorageDriverStatisticsBytesReadKey),

)

.zip(super::disk::get_int_value(

properties.inner(),

DictKey::Defined(ffi::kIOBlockStorageDriverStatisticsBytesWrittenKey),

))

}

.and_then(|(read_bytes, written_bytes)| {

read_bytes

.try_into()

.ok()

.zip(written_bytes.try_into().ok())

})

let read_bytes = super::disk::get_int_value(

properties.inner(),

DictKey::Defined(ffi::kIOBlockStorageDriverStatisticsBytesReadKey),

)?;

let written_bytes = super::disk::get_int_value(

properties.inner(),

DictKey::Defined(ffi::kIOBlockStorageDriverStatisticsBytesWrittenKey),

)?;

Some((read_bytes.try_into().ok()?, written_bytes.try_into().ok()?))

GuillaumeGomez · 2024-11-03T19:07:38Z

src/unix/linux/disk.rs

+/// * <https://github.com/torvalds/linux/blob/4f671fe2f9523a1ea206f63fe60a7c7b3a56d5c7/include/linux/bio.h#L99>
+/// * <https://lkml.org/lkml/2015/8/17/234>
+///
+/// [`psutil`]: <https://github.com/giampaolo/psutil/blob/master/psutil/_pslinux.py#L103>


Explanation is VERY appreciated, thanks a lot for that!

GuillaumeGomez · 2024-11-03T19:14:41Z

One thing missing: a test. For this test, please just check before/after creating a temporary file (with the tempfile crate as a dev-dependency) that the numbers got increased in at least one disk for supported systems.

GuillaumeGomez · 2024-11-03T19:16:25Z

We are looking to make use of the disk I/O functionality in the next week or so, and it would be great to have this available.

Also I see you used "we". If it's part of your work in a company, please don't forget to try to make them sponsor this crate as it might allow me to free more time to work on it. :)

samin-cf · 2024-11-04T05:00:40Z

One thing missing: a test. For this test, please just check before/after creating a temporary file (with the tempfile crate as a dev-dependency) that the numbers got increased in at least one disk for supported systems.

I have added a test, but it always seems to fail in CI on Ubuntu. Here is one of the test runs on a test branch: https://github.com/samin-cf/sysinfo/actions/runs/11658282075/job/32457102152. It seems like /proc/diskstats just isn't being updated for the disks that we recognize (for reference, the refresh interval is 10s so that's plenty of time for it to have updated).

Any idea what might be going on here?

GuillaumeGomez · 2024-11-04T12:26:08Z

tests/disk.rs

+
+    let mut disks = sysinfo::Disks::new_with_refreshed_list();
+
+    let mut file = NamedTempFile::new().unwrap();


I wonder if this test is failing because temp files on linux are actually not written on disk. Maybe try with a temp folder within which you add a file?

It works fine locally on my Ubuntu 22.04 machine, but I did try out your suggestion and unfortunately, still the same result. I am wondering if this is some limitation with how the Ubuntu CI runner is set up (by Github)...

Let's use the trick used above:

// If we don't have any physical core present, it's very likely that we're inside a VM... if s.physical_core_count().unwrap_or_default() > 0 {

Like that, the test will only run locally. I'll try to figure out later on what's wrong.

GuillaumeGomez · 2024-11-04T12:26:37Z

tests/disk.rs

+    // Write 10mb worth of data to the temp file. Repeat this 10 times to increase the chances
+    // of the OS registering the disk writes.
+    for _ in 0..10 {
+        let data = vec![1u8; 10 * 1024 * 1024];


Can be moved outside of the loop I think, no need to reallocate 10MB of memory at every turn.

GuillaumeGomez · 2024-11-04T12:27:09Z

tests/disk.rs

+
+    // Write 10mb worth of data to the temp file. Repeat this 10 times to increase the chances
+    // of the OS registering the disk writes.
+    for _ in 0..10 {


100MB seems like a lot no? ^^'

Maybe try 1MB 10 times?

GuillaumeGomez · 2024-11-04T20:51:54Z

tests/disk.rs

+
+    let s = sysinfo::System::new_all();
+
+    if !sysinfo::IS_SUPPORTED_SYSTEM || s.physical_core_count().unwrap_or_default() == 0 {


Seems like it's not enough...

Just had an idea though. Can you create file in Path::new(std::env::var_os("CARGO_TARGET_DIR").expect("missing CARGO_TARGET_DIR env var")).join("disk_usage_test.tmp") and write into it?

Yeah, seems like the CI runner has 2 cores.

I tried it with my test branch and CARGO_TARGET_DIR doesn't seem to be defined for some odd reason: https://github.com/samin-cf/sysinfo/actions/runs/11672513236/job/32501288423

Try CARGO_TARGET_TMPDIR as fallback?

Weird. No CARGO environment variables are defined: https://github.com/samin-cf/sysinfo/actions/runs/11672628189/job/32501653467

There are a few, but not the ones we're looking for. Let's try at compile time with env! then?

CARGO_TARGET_DIR isn't defined during compile time, but CARGO_TARGET_TMPDIR is. Still no luck: https://github.com/samin-cf/sysinfo/actions/runs/11672913856/job/32502524730

I didn't expect it to be such a nightmare. ^^'

Then just put the file in target/test_file.tmp and let's hope for the best this time...

😢 https://github.com/samin-cf/sysinfo/actions/runs/11673224654/job/32503482984#step:4:124

With the build just before the one I linked, the file was created in was target/tmp and that also failed.

I am finding it surprising no one has run into this. Either that or something is up with the implementation (but it works locally 🙃)

I think the best course of action here is disabling in CI. Since we saw what env variables were set, we can disable it on linux if we're in github CI.

Thanks for the help along the way! Quite the journey lol

GuillaumeGomez · 2024-11-04T22:03:22Z

tests/disk.rs

+    if std::env::var("RUNNER_OS").unwrap_or_default() == "Linux"
+        && std::env::var("CI").unwrap_or_default() == "true"


Suggested change

if std::env::var("RUNNER_OS").unwrap_or_default() == "Linux"

&& std::env::var("CI").unwrap_or_default() == "true"

if cfg!(target_os = "linux") && std::env::var("CI").is_ok()

GuillaumeGomez · 2024-11-05T16:17:52Z

Thanks a lot!

samin-cf commented Nov 2, 2024

View reviewed changes

src/common/mod.rs Show resolved Hide resolved

samin-cf force-pushed the disk-io branch 2 times, most recently from 5fa5dc0 to 0876661 Compare November 2, 2024 23:50

samin-cf added 5 commits November 2, 2024 20:00

Add disk i/o stats on macOS and Linux

4a0efa6

Add disk i/o stats on Windows

620c717

Add debug logs

40e7f83

Update for unsupported/unknown platforms

ab5a95f

Fix doc string error

cc7c945

samin-cf force-pushed the disk-io branch 3 times, most recently from d97c515 to d3b8b0c Compare November 3, 2024 00:04

Fix CI build errors

a4e7bd8

samin-cf force-pushed the disk-io branch from d3b8b0c to a4e7bd8 Compare November 3, 2024 01:54

GuillaumeGomez reviewed Nov 3, 2024

View reviewed changes

src/common/disk.rs Show resolved Hide resolved

GuillaumeGomez reviewed Nov 3, 2024

View reviewed changes

samin-cf force-pushed the disk-io branch from 10a239d to 3ff0060 Compare November 3, 2024 16:32

GuillaumeGomez reviewed Nov 3, 2024

View reviewed changes

Address PR feedback

fd52c86

samin-cf force-pushed the disk-io branch from 3ff0060 to fd52c86 Compare November 3, 2024 19:12

samin-cf force-pushed the disk-io branch from fc0bb2f to 48e7fed Compare November 4, 2024 02:02

samin-cf force-pushed the disk-io branch 5 times, most recently from faa8db8 to c45758c Compare November 4, 2024 04:15

GuillaumeGomez reviewed Nov 4, 2024

View reviewed changes

samin-cf force-pushed the disk-io branch 2 times, most recently from ebc5d3b to 7a7d204 Compare November 4, 2024 20:31

GuillaumeGomez reviewed Nov 4, 2024

View reviewed changes

samin-cf force-pushed the disk-io branch from 7a7d204 to d8c2b19 Compare November 4, 2024 21:59

GuillaumeGomez reviewed Nov 4, 2024

View reviewed changes

samin-cf force-pushed the disk-io branch 3 times, most recently from 3605000 to c90c5a2 Compare November 4, 2024 22:27

Add test for disk I/O usage

7df7803

samin-cf force-pushed the disk-io branch from c90c5a2 to 7df7803 Compare November 4, 2024 22:38

GuillaumeGomez approved these changes Nov 5, 2024

View reviewed changes

GuillaumeGomez merged commit 078c819 into GuillaumeGomez:master Nov 5, 2024
67 checks passed

GuillaumeGomez mentioned this pull request Nov 11, 2024

Remove procfs dependency and fix disk I/O test #1382

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add disk I/O stats #1376

Add disk I/O stats #1376

samin-cf commented Nov 2, 2024 •

edited by GuillaumeGomez

Loading

GuillaumeGomez Nov 3, 2024

samin-cf Nov 3, 2024

GuillaumeGomez Nov 3, 2024

GuillaumeGomez Nov 3, 2024

samin-cf Nov 3, 2024

GuillaumeGomez Nov 3, 2024

GuillaumeGomez commented Nov 3, 2024

samin-cf commented Nov 3, 2024

GuillaumeGomez commented Nov 3, 2024

GuillaumeGomez Nov 3, 2024

samin-cf Nov 3, 2024

GuillaumeGomez Nov 3, 2024

GuillaumeGomez Nov 3, 2024

GuillaumeGomez commented Nov 3, 2024

GuillaumeGomez commented Nov 3, 2024

samin-cf commented Nov 4, 2024

GuillaumeGomez Nov 4, 2024

samin-cf Nov 4, 2024 •

edited

Loading

GuillaumeGomez Nov 4, 2024

GuillaumeGomez Nov 4, 2024

GuillaumeGomez Nov 4, 2024

GuillaumeGomez Nov 4, 2024

samin-cf Nov 4, 2024

GuillaumeGomez Nov 4, 2024

samin-cf Nov 4, 2024

GuillaumeGomez Nov 4, 2024

samin-cf Nov 4, 2024

GuillaumeGomez Nov 4, 2024

samin-cf Nov 4, 2024

GuillaumeGomez Nov 4, 2024

samin-cf Nov 4, 2024

GuillaumeGomez Nov 4, 2024

GuillaumeGomez commented Nov 5, 2024


		let mut disks = sysinfo::Disks::new_with_refreshed_list();

		let mut file = NamedTempFile::new().unwrap();


		let s = sysinfo::System::new_all();

		if !sysinfo::IS_SUPPORTED_SYSTEM \|\| s.physical_core_count().unwrap_or_default() == 0 {

		if std::env::var("RUNNER_OS").unwrap_or_default() == "Linux"
		&& std::env::var("CI").unwrap_or_default() == "true"

	if std::env::var("RUNNER_OS").unwrap_or_default() == "Linux"
	&& std::env::var("CI").unwrap_or_default() == "true"
	if cfg!(target_os = "linux") && std::env::var("CI").is_ok()

Add disk I/O stats #1376

Add disk I/O stats #1376

Conversation

samin-cf commented Nov 2, 2024 • edited by GuillaumeGomez Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 3, 2024

samin-cf commented Nov 3, 2024

GuillaumeGomez commented Nov 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 3, 2024

GuillaumeGomez commented Nov 3, 2024

samin-cf commented Nov 4, 2024

Choose a reason for hiding this comment

samin-cf Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 5, 2024

samin-cf commented Nov 2, 2024 •

edited by GuillaumeGomez

Loading

samin-cf Nov 4, 2024 •

edited

Loading