From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 8FCC492079 for ; Mon, 10 Oct 2022 16:23:23 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7089348B9 for ; Mon, 10 Oct 2022 16:23:23 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 10 Oct 2022 16:23:21 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 9F3814490E for ; Mon, 10 Oct 2022 16:23:21 +0200 (CEST) Message-ID: Date: Mon, 10 Oct 2022 16:23:20 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 Content-Language: en-US To: Thomas Lamprecht , Proxmox Backup Server development discussion References: <20220824102657.159735-1-d.tschlatscher@proxmox.com> From: Daniel Tschlatscher In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.840 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -3.934 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup v4 1/3] fix #4077: Estimated Full metric on ext4 file systems X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Oct 2022 14:23:23 -0000 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 >> Reviewed-by: Matthias Heiserer >> --- >> 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 { >