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 81B741FF138 for ; Mon, 29 Jun 2026 10:38:11 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4DBF38D11; Mon, 29 Jun 2026 10:38:11 +0200 (CEST) Message-ID: <0f0316a8-0b06-4cbf-ab65-8c8dfbdfe892@proxmox.com> Date: Mon, 29 Jun 2026 10:38:05 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Christian Ebner Subject: Re: [RFC PATCH proxmox-backup 5/6] refactor: rename `UploadCounters` to `UploadProgress` To: Shan Shaji , pbs-devel@lists.proxmox.com References: <20260610180208.801614-1-s.shaji@proxmox.com> <20260610180208.801614-6-s.shaji@proxmox.com> Content-Language: en-US, de-DE In-Reply-To: <20260610180208.801614-6-s.shaji@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: 1782722275600 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: HVHJL25N3C6SXHTXRZZLZ2KA6DMON5ZO X-Message-ID-Hash: HVHJL25N3C6SXHTXRZZLZ2KA6DMON5ZO 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: With the suggested rework of the logic in patch 4, this could remain as is I think. On 6/10/26 8:02 PM, Shan Shaji wrote: > Earlier, `UploadCounters` was used for accounting upload stream > progress information. Now the `PxarArchiverProgressStats` has been added > as an optional field inside the struct, which makes the responsibility of > struct not only just counting the progress but also to show the the pxar > archive progress. So, renamed `UploadCounters` to `UploadProgress`. > > Signed-off-by: Shan Shaji > --- > pbs-client/src/backup_stats.rs | 4 +-- > pbs-client/src/backup_writer.rs | 37 +++++++++++++------------- > pbs-client/src/inject_reused_chunks.rs | 8 +++--- > 3 files changed, 25 insertions(+), 24 deletions(-) > > diff --git a/pbs-client/src/backup_stats.rs b/pbs-client/src/backup_stats.rs > index aa5ae4889..2866c29a1 100644 > --- a/pbs-client/src/backup_stats.rs > +++ b/pbs-client/src/backup_stats.rs > @@ -43,7 +43,7 @@ impl UploadStats { > > /// Atomic counters for accounting upload stream progress information > #[derive(Clone)] > -pub(crate) struct UploadCounters { > +pub(crate) struct UploadProgress { > injected_chunk_count: Arc, > known_chunk_count: Arc, > total_chunk_count: Arc, > @@ -54,7 +54,7 @@ pub(crate) struct UploadCounters { > pub pxar_progress: Option>, > } > > -impl UploadCounters { > +impl UploadProgress { > /// Create and zero init new upload counters > pub(crate) fn new(pxar_progress: Option>) -> Self { > Self { > diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs > index d0246689c..68379f725 100644 > --- a/pbs-client/src/backup_writer.rs > +++ b/pbs-client/src/backup_writer.rs > @@ -30,7 +30,7 @@ use proxmox_time::TimeSpan; > > use crate::pxar::PxarArchiverProgressStats; > > -use super::backup_stats::{BackupStats, UploadCounters, UploadStats}; > +use super::backup_stats::{BackupStats, UploadProgress, UploadStats}; > use super::inject_reused_chunks::{InjectChunks, InjectReusedChunks, InjectedChunksInfo}; > use super::merge_known_chunks::{MergeKnownChunks, MergedChunkInfo}; > > @@ -330,8 +330,8 @@ impl BackupWriter { > .as_u64() > .unwrap(); > > - let mut counters = UploadCounters::new(None); > - let counters_readonly = counters.clone(); > + let mut upload_progress = UploadProgress::new(None); > + let upload_progress_readonly = upload_progress.clone(); > > let is_fixed_chunk_size = prefix == "fixed"; > > @@ -343,8 +343,8 @@ impl BackupWriter { > match merged_chunk_info { > MergedChunkInfo::New(ref chunk_info) => { > let chunk_len = chunk_info.chunk_len; > - let offset = > - counters.add_new_chunk(chunk_len as usize, chunk_info.chunk.raw_size()); > + let offset = upload_progress > + .add_new_chunk(chunk_len as usize, chunk_info.chunk.raw_size()); > let end_offset = offset as u64 + chunk_len; > let mut guard = index_csum.lock().unwrap(); > let csum = guard.as_mut().unwrap(); > @@ -355,7 +355,7 @@ impl BackupWriter { > } > MergedChunkInfo::Known(ref mut known_chunk_list) => { > for (chunk_len, digest) in known_chunk_list { > - let offset = counters.add_known_chunk(*chunk_len as usize); > + let offset = upload_progress.add_known_chunk(*chunk_len as usize); > let end_offset = offset as u64 + *chunk_len; > let mut guard = index_csum.lock().unwrap(); > let csum = guard.as_mut().unwrap(); > @@ -379,7 +379,7 @@ impl BackupWriter { > prefix, > stream, > index_csum_2, > - counters_readonly, > + upload_progress_readonly, > options.is_metadata_mode, > ) > .await?; > @@ -780,8 +780,8 @@ impl BackupWriter { > pxar_archive_progress_stats: Option>, > is_metadata_mode: bool, > ) -> impl Future> { > - let mut counters = UploadCounters::new(pxar_archive_progress_stats); > - let counters_readonly = counters.clone(); > + let mut upload_progress = UploadProgress::new(pxar_archive_progress_stats); > + let upload_progress_readonly = upload_progress.clone(); > > let is_fixed_chunk_size = prefix == "fixed"; > > @@ -789,7 +789,7 @@ impl BackupWriter { > let index_csum_2 = index_csum.clone(); > > let stream = stream > - .inject_reused_chunks(injections, counters.clone()) > + .inject_reused_chunks(injections, upload_progress.clone()) > .and_then(move |chunk_info| match chunk_info { > InjectedChunksInfo::Known(chunks) => { > // account for injected chunks > @@ -797,7 +797,7 @@ impl BackupWriter { > let mut guard = index_csum.lock().unwrap(); > let csum = guard.as_mut().unwrap(); > for chunk in chunks { > - let offset = counters.add_injected_chunk(&chunk) as u64; > + let offset = upload_progress.add_injected_chunk(&chunk) as u64; > let digest = chunk.digest(); > known.push((offset, digest)); > let end_offset = offset + chunk.size(); > @@ -819,13 +819,14 @@ impl BackupWriter { > let mut known_chunks = known_chunks.lock().unwrap(); > let digest = *chunk_builder.digest(); > let (offset, res) = if known_chunks.contains(&digest) { > - let offset = counters.add_known_chunk(chunk_len) as u64; > + let offset = upload_progress.add_known_chunk(chunk_len) as u64; > (offset, MergedChunkInfo::Known(vec![(offset, digest)])) > } else { > match chunk_builder.build() { > Ok((chunk, digest)) => { > - let offset = > - counters.add_new_chunk(chunk_len, chunk.raw_size()) as u64; > + let offset = upload_progress > + .add_new_chunk(chunk_len, chunk.raw_size()) > + as u64; > known_chunks.insert(digest); > ( > offset, > @@ -863,7 +864,7 @@ impl BackupWriter { > prefix, > stream, > index_csum_2, > - counters_readonly, > + upload_progress_readonly, > is_metadata_mode, > ) > } > @@ -875,7 +876,7 @@ impl BackupWriter { > prefix: &str, > stream: impl Stream>, > index_csum: Arc>>, > - counters: UploadCounters, > + upload_progress: UploadProgress, > is_metadata_mode: bool, > ) -> impl Future> { > let append_chunk_path = format!("{prefix}_index"); > @@ -891,7 +892,7 @@ impl BackupWriter { > || archive.ends_with(".pxar.didx") > || archive.ends_with(".ppxar.didx") > { > - let counters = counters.clone(); > + let counters = upload_progress.clone(); > Some(tokio::spawn(async move { > loop { > tokio::time::sleep(tokio::time::Duration::from_secs(60)).await; > @@ -998,7 +999,7 @@ impl BackupWriter { > handle.abort(); > } > > - futures::future::ok(counters.to_upload_stats(csum, start_time.elapsed())) > + futures::future::ok(upload_progress.to_upload_stats(csum, start_time.elapsed())) > }) > } > > diff --git a/pbs-client/src/inject_reused_chunks.rs b/pbs-client/src/inject_reused_chunks.rs > index c85a28a08..314dd94ce 100644 > --- a/pbs-client/src/inject_reused_chunks.rs > +++ b/pbs-client/src/inject_reused_chunks.rs > @@ -7,7 +7,7 @@ use anyhow::{Error, anyhow}; > use futures::{Stream, ready}; > use pin_project_lite::pin_project; > > -use crate::backup_stats::UploadCounters; > +use crate::backup_stats::UploadProgress; > use crate::pxar::create::ReusableDynamicEntry; > > pin_project! { > @@ -16,7 +16,7 @@ pin_project! { > input: S, > next_injection: Option, > injections: Option>, > - counters: UploadCounters, > + counters: UploadProgress, > } > } > > @@ -42,7 +42,7 @@ pub trait InjectReusedChunks: Sized { > fn inject_reused_chunks( > self, > injections: Option>, > - counters: UploadCounters, > + counters: UploadProgress, > ) -> InjectReusedChunksQueue; > } > > @@ -53,7 +53,7 @@ where > fn inject_reused_chunks( > self, > injections: Option>, > - counters: UploadCounters, > + counters: UploadProgress, > ) -> InjectReusedChunksQueue { > InjectReusedChunksQueue { > input: self,