From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 6110C1FF0B2 for ; Fri, 25 Sep 2026 14:11:53 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 2B46821693; Fri, 25 Sep 2026 14:11:53 +0200 (CEST) Message-ID: Date: Fri, 25 Sep 2026 14:11:48 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Christian Ebner Subject: Re: [RFC PATCH proxmox-backup 1/4] datastore: factor out s3 chunk objects pagination into a helper To: Shan Shaji , pbs-devel@lists.proxmox.com References: <20260915155935.135460-1-s.shaji@proxmox.com> <20260915155935.135460-2-s.shaji@proxmox.com> Content-Language: en-US, de-DE In-Reply-To: <20260915155935.135460-2-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: 1790338308893 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.622 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust 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: 5STUIVWKDTR6DLIZZMNCRCIXTCBSZYZ6 X-Message-ID-Hash: 5STUIVWKDTR6DLIZZMNCRCIXTCBSZYZ6 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: Code changes look good to me in general, just a few suggestions to make the dependency on the datastore's chunk store instance a bit more explicit. On 9/15/26 6:00 PM, Shan Shaji wrote: > This is in preparation to re-use the chunk iteration logic for creating > empty marker files during s3 refresh. > > Signed-off-by: Shan Shaji > --- > pbs-datastore/src/datastore.rs | 85 ++++++++++++++++++++++------------ > 1 file changed, 56 insertions(+), 29 deletions(-) > > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs > index c3db52269..f3eab8a28 100644 > --- a/pbs-datastore/src/datastore.rs > +++ b/pbs-datastore/src/datastore.rs > @@ -2382,6 +2382,47 @@ impl DataStore { > } > } > > + /// Fetch chunk objects in batches of max 1000 per request. > + /// For each objects listed, the `callback` will be invoked with its > + /// S3 object key and size. > + fn paginate_s3_chunk_objects( > + &self, comment: As is, this does not need to be a method of DataStore, the reference to self is never used after all. But I would keep it as such and make the dependency on the datastore and it's chunk store explicit. To do this, I suggest to pass in the chunk store by reference to the callback and use the reference for all chunk store operations in the callback so the relation becomes stringent. So ... > + worker: &dyn WorkerTaskContext, > + s3_client: &S3Client, > + mut callback: F, > + ) -> Result<(), Error> > + where > + F: FnMut(S3ObjectKey, u64) -> Result<(), Error>, comment: Instead of the generic, this could use an `impl Trait` parameter declaration, which makes the method signature cleaner, and with the suggested reference to the chunk store passed along would become: fn paginate_s3_chunk_objects( &self, worker: &dyn WorkerTaskContext, s3_client: &S3Client, mut callback: impl FnMut(&ChunkStore, S3ObjectKey, u64) -> Result<(), Error>, ) -> Result<(), Error> { > + { > + let prefix = S3PathPrefix::Some(".chunks/".to_string()); > + let mut list_bucket_result = > + proxmox_async::runtime::block_on(s3_client.list_objects_v2(&prefix, None)) > + .context("failed to list chunk in s3 object store")?; > + > + loop { > + for content in list_bucket_result.contents { > + worker.check_abort()?; > + worker.fail_on_shutdown()?; > + ... here we would now call: ``` callback(self.inner.chunk_store.as_ref(), content.key, content.size)?; ``` > + callback(content.key, content.size)?; > + } > + > + // Process next batch of chunks if there are more. > + if list_bucket_result.is_truncated { > + list_bucket_result = proxmox_async::runtime::block_on(s3_client.list_objects_v2( > + &prefix, > + list_bucket_result.next_continuation_token.as_deref(), > + ))?; > + > + continue; > + } > + > + break; > + } > + > + Ok(()) > + } > + > fn garbage_collection_impl( > &self, > worker: &dyn WorkerTaskContext, > @@ -2473,28 +2514,23 @@ impl DataStore { > > if let Some(ref s3_client) = s3_client { > let mut chunk_count = 0; > - let prefix = S3PathPrefix::Some(".chunks/".to_string()); > - // Operates in batches of 1000 objects max per request > - let mut list_bucket_result = > - proxmox_async::runtime::block_on(s3_client.list_objects_v2(&prefix, None)) > - .context("failed to list chunk in s3 object store")?; > - > let mut delete_list = > S3DeleteList::with_thresholds(S3_DELETE_BATCH_LIMIT, S3_DELETE_DEFER_LIMIT_SECONDS); > - loop { > - for content in list_bucket_result.contents { > - worker.check_abort()?; > - worker.fail_on_shutdown()?; > + > + self.paginate_s3_chunk_objects( > + worker, > + s3_client, > + |object_key, size| -> Result<(), Error> { ... this would then have the direct reference to the chunk store: ``` |chunk_store, object_key, size| -> Result<(), Error> { ``` > let (chunk_path, digest, bad) = > - match self.chunk_path_from_object_key(&content.key) { > + match self.chunk_path_from_object_key(&object_key) { > Some(path) => path, > - None => continue, > + None => return Ok(()), > }; > > let timeout = std::time::Duration::from_secs(0); > let _chunk_guard = match self.inner.chunk_store.lock_chunk(&digest, timeout) { ... so operations on that would use that directly instead of binding to self, so here for example ``` let _chunk_guard = match chunk_store.lock_chunk(&digest, timeout) { ``` > Ok(guard) => guard, > - Err(_) => continue, > + Err(_) => return Ok(()), > }; > let _guard = self.inner.chunk_store.mutex().lock().unwrap(); > > @@ -2526,7 +2562,7 @@ impl DataStore { > atime, > min_atime, > oldest_writer, > - content.size, > + size, > bad, > &mut gc_status, > || { > @@ -2537,15 +2573,15 @@ impl DataStore { > std::fs::remove_file(chunk_path)?; > } > } > - delete_list.push(content.key, _chunk_guard); > + delete_list.push(object_key, _chunk_guard); > Ok(()) > }, > )?; > } > } else { > gc_status.removed_chunks += 1; > - gc_status.removed_bytes += content.size; > - delete_list.push(content.key, _chunk_guard); > + gc_status.removed_bytes += size; > + delete_list.push(object_key, _chunk_guard); > } > > chunk_count += 1; > @@ -2555,19 +2591,10 @@ impl DataStore { > > // limit pending deletes to avoid holding too many chunk flocks > delete_list.conditional_delete_and_drop_locks(s3_client)?; > - } > - // Process next batch of chunks if there is more > - if list_bucket_result.is_truncated { > - list_bucket_result = > - proxmox_async::runtime::block_on(s3_client.list_objects_v2( > - &prefix, > - list_bucket_result.next_continuation_token.as_deref(), > - ))?; > - continue; > - } > > - break; > - } > + Ok(()) > + }, > + )?; > > // delete the last batch of objects, if there are any remaining > delete_list.delete_and_drop_locks(s3_client)?;