Re: [PATCH v2 proxmox proxmox-backup 0/3] datastore: gc: defer and batch chunk atime updates

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

On 8/11/26 11:37 AM, Enrico Plantulli wrote:
> Hi,
> 
> this is v2 of the chunk metadata prefetch series, reworked along the
> lines suggested in the review of v1: instead of one whole-store
> readdir pass before phase 1, the chunk atime updates themselves are
> now deferred into a bounded list and flushed sorted by digest, opening
> and iterating each chunk directory right before its chunks are
> touched, with the utimensat() calls going through the open directory
> file descriptor.
> 
> Changes since v1:
> 
> * dropped the whole-store prefetch pass and the
>    prefetch_chunk_metadata() helper entirely;
> * added deferred, batched atime updates behind the same opt-in
>    gc-chunk-metadata-prefetch tuning option (kept opt-in as requested:
>    backends that do not prefetch on readdir, e.g. network attached
>    storage, may regress);
> * added a separate gc-prefetch-batch-size tuning option (default
>    1048576, range 1024 - 16M) as an independent knob, as suggested, to
>    make benchmarking on different backends easy;
> * the batch is used for filesystem backed datastores only; S3 keeps
>    the immediate path, since its in-use markers need per-chunk
>    handling anyway;
> * a chunk found missing at flush time is handled as before (touch the
>    .bad companions, warn), but the warning can only name the digest,
>    not the referencing index file - noted in patch 2;
> * GUI patch now covers both options.
> 
> Why the rework was needed, empirically: I ran the v1 one-pass version
> on the 72M chunk production datastore (A in the v1 thread). The pass
> itself was fast (81.8M directory entries in 59m32s), but the warmed
> metadata did not survive until a phase 1 that runs for hours reached
> it: the GC worker was observed blocked in
> zio_wait <- dbuf_read <- zap_get_leaf_byblk (and dnode_hold_impl),
> re-reading from disk the very blocks the pass had loaded hours
> earlier, with the ARC sitting at its adaptive target far below
> c_max. External re-walks recovered the rate only modestly (~+30%).
> Warming right before use removes that window entirely, which is what
> this v2 does.
> 
> The batch size bound also controls the readdir amplification: a flush
> of N sorted entries spans up to min(N, 65536) directories, so about
> N/65536 chunks are served per directory read. The default of 1M gives
> ~16 used chunks per directory read on a full store; benchmarking
> larger values is exactly what the separate option is for.
> 
> Testing: compile-tested (cargo build, clippy with no new warnings,
> fmt, cargo test) against current master of both repositories. I have
> not yet run this exact v2 end-to-end in production; I will follow up
> with figures from datastore A and B once it has. NAS-backed figures
> would be very welcome from anyone with such a setup, as discussed.
> 
> CLA: signed and sent to office@proxmox.com on Aug 10; office has been
> in touch.

Thanks for v2 of the patches, they have seen an improvement over the 
previous version but still require major revision. I left more details 
on the patches, but here some general comments as well.

In general it would be desirable to only have one optional parameter to
control the GC behavior for this, see comments on patch 1 for details.

Also, while the patches improve GC performance in case of cold caches, 
they do also introduce a performance regression for hot cache case. Some 
results from my testing (5 GC runs each, dropped caches via `echo 3 > 
/proc/sys/vm/drop_caches`), measuring just phase 1 of GC:

ZFS on SDD backed datastore:
     Chunk cache: hits 2671816, misses 924604 (hit ratio 74.29%)
     On-Disk usage: 2.077 TiB
     On-Disk chunks: 924604
     Deduplication factor: 5.32

     no readdir (caches dropped):                        54.54 ± 0.28s
     no readdir (caches kept):                            6.72 ± 0.02s
     with readdir (caches dropped, default batch size):  32.66 ± 0.16s
     with readdir (caches kept, default batch size):      9.82 ± 0.97s
     with readdir (caches dropped, max batch size):      32.65 ± 0.09s

A particular observation is that the speedup for GC is observed even 
when the readdir() is not performed (result not included above, see 
comment and diff on patch 2).
The major speed gain is therefore attributed to opening the chunk prefix 
directory in a structured way, allowing the utimensat() calls to be 
relative to that open file handle brings additional gains. This could 
further be optimized by only doing the additional directory open call
if more than 1 chunk have to be touched within it, therefore reducing 
number of syscalls if not required.

Another observation is that progress logging is currently broken, since 
now the index file reading is strongly decoupled from the atime updates. 
This should be improved upon.

Also, please do provide some testing results from your side as well, 
getting additional datapoints here is desired.