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 7693F1FF0EA for ; Fri, 14 Aug 2026 16:34:41 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 42415226A3; Fri, 14 Aug 2026 16:34:41 +0200 (CEST) Message-ID: <7d91d14b-bc60-408f-817f-22040abf7eb4@proxmox.com> Date: Fri, 14 Aug 2026 16:34:34 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Christian Ebner Subject: Re: [PATCH v2 proxmox 1/3] pbs-api-types: add gc chunk metadata prefetch tuning options To: Enrico Plantulli , pbs-devel@lists.proxmox.com References: <20260810050201.347124-1-plantulli@gmail.com> <20260811093722.735290-1-plantulli@gmail.com> <20260811093722.735290-2-plantulli@gmail.com> Content-Language: en-US, de-DE In-Reply-To: <20260811093722.735290-2-plantulli@gmail.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: 1786718056239 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.192 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 RDNS_NONE 1.274 Delivered to internal network by a host with no rDNS 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: TLFIIOTUFFF3C7FUAVTGPNPG5FJTC43U X-Message-ID-Hash: TLFIIOTUFFF3C7FUAVTGPNPG5FJTC43U 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 8/11/26 11:37 AM, Enrico Plantulli wrote: > Add an opt-in option to defer and batch chunk access time updates in > garbage collection phase 1, plus a separate option for the batch size > to ease benchmarking on different storage backends. comment: The same option can be used here for both, opting in and defining the batch size. If the optional parameter is not set, this should simply imply fallback to the default atime update logic. This helps reduce code lines, config sizes and parsing overhead. > Batched updates are flushed sorted by digest, so the chunk directories > involved can be warmed via readdir right before their chunks are > touched. Keeping the switch opt-in follows the review of the first > version of this series: storage backends that do not prefetch inode > metadata on readdir, such as some network attached storages, may see > no benefit or a regression. nit: The commit message does not need to contain review history and best kept on-point. I would rather prefer an additional reasoning for the chosen default, minimum and maximum values here. > Signed-off-by: Enrico Plantulli > --- > pbs-api-types/src/datastore.rs | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs > index 93ccaf0..79ccff9 100644 > --- a/pbs-api-types/src/datastore.rs > +++ b/pbs-api-types/src/datastore.rs > @@ -256,6 +256,14 @@ pub const GC_CACHE_CAPACITY_SCHEMA: Schema = > .default(1024 * 1024) > .schema(); > > +pub const GC_PREFETCH_BATCH_SIZE_SCHEMA: Schema = IntegerSchema::new( > + "Batch size for deferred chunk access time updates in garbage collection phase 1", > +) > +.minimum(1024) > +.maximum(16 * 1024 * 1024) nit: adding some reasoning to the commit > +.default(1024 * 1024) > +.schema(); > + > #[api( > properties: { > "chunk-order": { > @@ -278,6 +286,18 @@ pub const GC_CACHE_CAPACITY_SCHEMA: Schema = > schema: GC_CACHE_CAPACITY_SCHEMA, > optional: true, > }, > + "gc-chunk-metadata-prefetch": { > + description: > + "Defer and batch chunk access time updates in garbage collection phase 1, \ > + warming each chunk directory via readdir right before its chunks are touched", nit: this is an user facing description, how exactly the caches are warmed is an implementation detail so I suggest to not include that part, but rather keep this a bit more concise, but this unneeded option should be dropped in favor of the other option anyways. > + optional: true, > + default: false, > + type: bool, > + }, > + "gc-prefetch-batch-size": { > + schema: GC_PREFETCH_BATCH_SIZE_SCHEMA, > + optional: true, > + }, > "default-verification-workers": { > schema: VERIFY_JOB_VERIFY_THREADS_SCHEMA, > optional: true, > @@ -304,6 +324,10 @@ pub struct DatastoreTuning { > #[serde(skip_serializing_if = "Option::is_none")] > pub gc_cache_capacity: Option, > #[serde(skip_serializing_if = "Option::is_none")] > + pub gc_chunk_metadata_prefetch: Option, > + #[serde(skip_serializing_if = "Option::is_none")] > + pub gc_prefetch_batch_size: Option, > + #[serde(skip_serializing_if = "Option::is_none")] > pub default_verification_workers: Option, > #[serde(skip_serializing_if = "Option::is_none")] > pub default_verification_readers: Option, > > base-commit: e3e3ff11b9b92fe1ace89b84c1e15c150e2db660