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

[Dominik Csapak <d.csapak@proxmox.com>]

On 12/15/21 13:41, Thomas Lamprecht wrote:
> 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.
> 

sure, makes sense

>> 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..

how about FileSystemInformation (or FSInfo if the length is a problem)? 
IMHO "usage" is wrong if we include things like the type or id

> 
> 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?

yeah sounds good, i'd also include the fs type, too, could be useful 
sometimes.

> 
>> +    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.

yeah sorry, i still do not do that automatically ....

> 
> 
>> +    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
>> +    })
>> +}
>