Re: [PATCH v2 proxmox 1/3] pbs-api-types: add gc chunk metadata prefetch tuning options

[Christian Ebner <c.ebner@proxmox.com>]

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 <plantulli@gmail.com>
> ---
>   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<usize>,
>       #[serde(skip_serializing_if = "Option::is_none")]
> +    pub gc_chunk_metadata_prefetch: Option<bool>,
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub gc_prefetch_batch_size: Option<usize>,
> +    #[serde(skip_serializing_if = "Option::is_none")]
>       pub default_verification_workers: Option<usize>,
>       #[serde(skip_serializing_if = "Option::is_none")]
>       pub default_verification_readers: Option<usize>,
> 
> base-commit: e3e3ff11b9b92fe1ace89b84c1e15c150e2db660