[pbs-devel] [PATCH proxmox-backup v4 1/3] fix #4077: Estimated Full metric on ext4 file systems

[Daniel Tschlatscher]

On 10/5/22 17:56, Thomas Lamprecht wrote:
> Am 24/08/2022 um 12:26 schrieb Daniel Tschlatscher:
>> The rrd data now includes tracking the total disk usage for the unpri-
>> vileged backup user. The calculation for the estimated_time_full was
>> adapted to use the total for the unpriviliged user.
>>
>> The unpriv_total is the sum of the used space in the file system, plus
>> the available space for the unpriviliged user.
>>
>> Signed-off-by: Daniel Tschlatscher <d.tschlatscher at proxmox.com>
>> Reviewed-by: Matthias Heiserer <m.heiserer at proxmox.com>
>> ---
>> No changes from v3
>>
>>  src/api2/status.rs              | 3 ++-
>>  src/bin/proxmox-backup-proxy.rs | 2 ++
>>  2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/api2/status.rs b/src/api2/status.rs
>> index 50d401c2..b918156e 100644
>> --- a/src/api2/status.rs
>> +++ b/src/api2/status.rs
>> @@ -84,7 +84,8 @@ pub async fn datastore_status(
>>          let get_rrd =
>>              |what: &str| extract_rrd_data(&rrd_dir, what, RRDTimeFrame::Month, RRDMode::Average);
>>  
>> -        let total_res = get_rrd("total")?;
>> +        // Use the space for the unpriviliged user, as e.g. ext4 reserves 5% of disks for root only
>> +        let total_res = get_rrd("unpriv_total")?;
> 
> But we only just started to even record that, so it's not _that_ ideal to directly switch
> as the user will notice a possible big jump after the upgrade. Can we check how many data is
> in there to decide which one to use, or do it more heuristically and start recording now but
> switch only in a later usage (e.g., the point release after the next one).
> 

It seems to me, finding a way to tell which dataset to use for the
calculation is the preferable way here, as the heuristic approach of
recording it anyway and switching at a later date would show the same
jump for users upgrading from an earlier version or in cases where the
RRDB did not have enough time to populate between versions.
Or would you reckon this is acceptable in such (less frequent) cases, as
it is more of a cosmetic feature anyway?

>>          let used_res = get_rrd("used")?;
>>  
>>          if let (
>> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
>> index 214bc9b1..e5c8813f 100644
>> --- a/src/bin/proxmox-backup-proxy.rs
>> +++ b/src/bin/proxmox-backup-proxy.rs
>> @@ -1273,6 +1273,8 @@ fn rrd_update_disk_stat(disk: &DiskStat, rrd_prefix: &str) {
>>          rrd_update_gauge(&rrd_key, status.total as f64);
>>          let rrd_key = format!("{}/used", rrd_prefix);
>>          rrd_update_gauge(&rrd_key, status.used as f64);
>> +        let rrd_key = format!("{}/unpriv_total", rrd_prefix);
>> +        rrd_update_gauge(&rrd_key, (status.used + status.available) as f64);
> 
> what speaks against broadcasting available? I mean one got the same information
> either way, but mirroring the disk stats and returning total, used, available instead
> of "two" totals sounds somewhat nicer to be from an API-User POV, but no hard feelings
> here, just stuck quite a bit out to me after reviewing this series finally.
> 

Mostly, it made it easier to just drop in place instead of the value for
total, rather than querying combining the data for used and available. I
didn't overwrite it also, as that would be a breaking change.

In my eyes, it is also more easily understandable from the POV of an API
user because the naming for available does not inherently carry the
information that this value excludes any unusable storage for
unprivileged users.
I see potential for confusion here, as one might incorrectly assume that
used + available = total, when that is, in my testing, really never the
case. Potentially, because of reserved blocks on ext4 file systems and
because of a small amount which seems to be claimed by the kernel.

>>      }
>>  
>>      if let Some(stat) = &disk.dev {
>