From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 3D1F91FF13B for ; Wed, 03 Jun 2026 13:58:55 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0E5F39A38; Wed, 3 Jun 2026 13:58:55 +0200 (CEST) Message-ID: <8ccb9ce9-df5b-4c60-997d-14b665010009@proxmox.com> Date: Wed, 3 Jun 2026 13:58:20 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup 2/6] task tracking: use parameter for initial count and refactor updates To: Robert Obkircher , pbs-devel@lists.proxmox.com References: <20260602130001.217482-1-r.obkircher@proxmox.com> <20260602130001.217482-8-r.obkircher@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20260602130001.217482-8-r.obkircher@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1780487864158 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.069 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: ALSCV2IIJORE5EEHPHUJ3WIO6H2H3NFM X-Message-ID-Hash: ALSCV2IIJORE5EEHPHUJ3WIO6H2H3NFM X-MailFrom: c.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 6/2/26 3:00 PM, Robert Obkircher wrote: > Don't hard-code the initial count to 1 and bail out if it is negative. > Replace the ActiveOperationStats initializers with the update logic > because they would be reformatted to multiple lines after adding a > field and an operation. > > Signed-off-by: Robert Obkircher Reviewed-by: Christian Ebner > --- > pbs-datastore/src/task_tracking.rs | 52 +++++++++++++++++------------- > 1 file changed, 30 insertions(+), 22 deletions(-) > > diff --git a/pbs-datastore/src/task_tracking.rs b/pbs-datastore/src/task_tracking.rs > index d4cc076aa..a67b77221 100644 > --- a/pbs-datastore/src/task_tracking.rs > +++ b/pbs-datastore/src/task_tracking.rs > @@ -1,4 +1,4 @@ > -use anyhow::Error; > +use anyhow::{Error, bail}; > use libc::pid_t; > use nix::unistd::Pid; > use std::iter::Sum; > @@ -15,6 +15,16 @@ pub struct ActiveOperationStats { > pub write: i64, > } > > +impl ActiveOperationStats { > + fn add(&mut self, count: i64, operation: Operation) { > + match operation { > + Operation::Read => self.read += count, > + Operation::Write => self.write += count, > + Operation::Lookup => (), // no IO must happen there > + } > + } > +} > + > impl Sum for ActiveOperationStats { > fn sum(iter: I) -> Self > where > @@ -101,14 +111,8 @@ pub fn update_active_operations( > let (_lock, options) = open_lock_file(name)?; > > let pid = std::process::id(); > - let starttime = procfs::PidStat::read_from_pid(Pid::from_raw(pid as pid_t))?.starttime; tiny nit: this change is somewhat unrelated to the rest of the patch, could live in its own patch or remain as is... but no hard opinion, just distracted by it initially while looking at the changes. > > - let mut updated_active_operations = match operation { > - Operation::Read => ActiveOperationStats { read: 1, write: 0 }, > - Operation::Write => ActiveOperationStats { read: 0, write: 1 }, > - Operation::Lookup => ActiveOperationStats { read: 0, write: 0 }, > - }; > - let mut found_entry = false; > + let mut updated = None; > let mut updated_tasks: Vec = match file_read_optional_string(&path)? { > Some(data) => serde_json::from_str::>(&data)? > .iter_mut() > @@ -117,13 +121,8 @@ pub fn update_active_operations( > Some(stat) if pid == task.pid && stat.starttime != task.starttime => None, > Some(_) => { > if pid == task.pid { > - found_entry = true; > - match operation { > - Operation::Read => task.active_operations.read += count, > - Operation::Write => task.active_operations.write += count, > - Operation::Lookup => (), // no IO must happen there > - }; > - updated_active_operations = task.active_operations; > + task.active_operations.add(count, operation); > + updated = Some(task.active_operations); > } > Some(task.clone()) > } > @@ -134,18 +133,27 @@ pub fn update_active_operations( > None => Vec::new(), > }; > > - if !found_entry { > + let result = if let Some(updated) = updated { > + updated > + } else { > + if count < 0 { > + bail!("unexpected initial active operation count: {count} {operation:?}") > + } > + let mut new = ActiveOperationStats::default(); > + new.add(count, operation); > updated_tasks.push(TaskOperations { > pid, > - starttime, > - active_operations: updated_active_operations, > - }) > - } > + starttime: procfs::PidStat::read_from_pid(Pid::from_raw(pid as pid_t))?.starttime, > + active_operations: new, > + }); > + new > + }; > + > replace_file( > &path, > serde_json::to_string(&updated_tasks)?.as_bytes(), > options, > false, > - ) > - .map(|_| updated_active_operations) > + )?; > + Ok(result) > }