Re: [pbs-devel] [PATCH proxmox v2 2/3] proxmox-sys: add DiskUsage struct and helper

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

On 15.12.21 10:19, Dominik Csapak wrote:
> copied from proxmox-backup
> 

It's copied I know, but maybe we can use the need of touching this to improve it
slightly, so please read my comments inline as I would make then on this being a
clean slate addition.

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  proxmox-sys/src/fs/mod.rs | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/proxmox-sys/src/fs/mod.rs b/proxmox-sys/src/fs/mod.rs
> index 9fe333b..5935b1a 100644
> --- a/proxmox-sys/src/fs/mod.rs
> +++ b/proxmox-sys/src/fs/mod.rs
> @@ -3,6 +3,7 @@ use std::fs::File;
>  use std::path::Path;
>  
>  use anyhow::{bail, Error};
> +use serde::Serialize;
>  
>  use std::os::unix::io::{AsRawFd, RawFd};
>  use nix::unistd::{Gid, Uid};
> @@ -102,3 +103,28 @@ impl CreateOptions {
>      */
>  }
>  
> +/// Basic disk usage information

would add the source, e.g.: ... derived from what statfs64 reports

> +#[derive(Serialize)]
> +pub struct DiskUsage {

Is DiskUsage really a good name here? As with "Disk" I thing block level, but
this is filesystem level.. Maybe FileSystemUsage (shorteting to FSUsage could
be done, but that's not helping that much..

can we get short doc comments here about what unit this is, IMO some slight
redundancy here never hurt anyone or made it harder to understand.

/// total bytes available in a mounted filesystem

> +    pub total: u64,
> +    pub used: u64,

Available is derived from f_bavail, which is the available to the unprivileged user,
maybe note that here, as with ext4 default that means that you get about 5% less
here than `total - used` which may be good to know for users, that would then also
explain why we even have `available` as else, `total` and `used` would be enough
info.

general notes, in general a file system usage could be improved with inode total and
free count and mayb the FSID for mapping such a struct back to the physical FS?

> +    pub available: u64,
> +}
> +
> +/// Get disk usage information from path
> +pub fn disk_usage(path: &std::path::Path) -> Result<DiskUsage, Error> {
> +    let mut stat: libc::statfs64 = unsafe { std::mem::zeroed() };
> +
> +    use nix::NixPath;
> +
> +    let res = path.with_nix_path(|cstr| unsafe { libc::statfs64(cstr.as_ptr(), &mut stat) })?;
> +    nix::errno::Errno::result(res)?;
> +
> +    let bsize = stat.f_bsize as u64;
> +


nit but can you rust format new entries, it's a bit a PITA as the test-range operating
feature is only available in nightly IIRC, but proxmox-sys should be relatively clean so
doing it on the whole file shouldn't that big of a nuisance.


> +    Ok(DiskUsage{

missing space before {

> +        total: stat.f_blocks*bsize,

space between operands of a math evaluation


> +        used: (stat.f_blocks-stat.f_bfree)*bsize,

same here

> +        available: stat.f_bavail*bsize,

same here
> +    })
> +}