Hi,

thank you for the quick and thorough review!

> For it to be considered please send the signed contributors license
> agreement

Already done - I sent the signed individual CLA to office@proxmox.com
on Aug 10, our mails probably crossed. Happy to resend if it did not
arrive.

> I think there might be additional performance improvements to be
> gained from this: In particular, instead of performing the atime
> updates on the chunks on first encounter right away, these could be
> kept in a list with upper boundary, the utimensat() call deferred
> until the list reached it's maximum or there are not further chunks
> to process. Sorting the list by digest and performing the readdir()
> calls on that ordered list should give similar benefits if multiple
> chunks are to be touched within the same directory AFAIU, but
> without having to read empty directories and with the benefit of
> warming the cache when needed.

I can offer strong empirical support for exactly this design. Since
sending the series I have run the one-pass version on datastore A
(72M chunks) in production, and the one-pass approach shows its limit
there:

- the prefetch itself was fast: 81.8M directory entries in 59m32s;
- phase 1 then started fast, but after ~4 hours collapsed to
  ~100-150 chunks/s. Kernel stack samples of the GC worker showed it
  blocked in zio_wait <- dbuf_read <- zap_get_leaf_byblk (and
  dnode_hold_impl), i.e. re-reading from disk the very metadata the
  prefetch had loaded hours earlier;
- the ARC had recycled those blocks: it was sitting at its adaptive
  target (c = 138G) even though c_max was 250G, so the 7-hour-old
  prefetched blocks were evicted long before phase 1 reached them;
- re-running the same directory walk externally, concurrently with
  phase 1, recovered the rate only modestly: about +30% on the rate
  of first-touched chunks, with cold demand reads persisting at
  ~55/s even right after the walk. With the ARC sitting at its
  adaptive target, blocks warmed minutes earlier are already being
  recycled by the time the marker reaches them.

So on a store where phase 1 runs for hours, neither one warming pass
up front nor periodic full re-walks solve it: the warming has to
happen right before use, which is exactly what your deferred-batch
design does - and it makes the survival question disappear entirely.
It should also compose nicely
with the existing LRU dedup cache: the deferred entries are precisely
the cache misses, so bounding the list relative to the LRU capacity
sounds right to me. And taking the parent directory handle for
utimensat() is a further nice win on top.

For scale: on datastore B, where phase 1 fits well inside the
eviction horizon, the steady state with the one-pass version is
prefetch 12.4s + phase 1 in 12m38s daily (down from 2h14m for
phase 1 alone before).

> If deferring utimensat() calls as suggested above, this could most
> likely be implemented without much hustle, requiring sorting by
> inode instead of by digest.

Agreed - once the touches go through a sorted batch, the sort key is
pluggable and sort-by-inode becomes almost free to try.

I will rework the series (v2) along these lines: deferred bounded
list of pending touches, sorted flush with readdir on the directories
actually needed, parent-dir file handles for utimensat(), and the
iterator's hex filtering made optional as you suggested. Two
questions before I start:

1. With the deferred design the warming happens on demand, so the
   original gc-chunk-metadata-prefetch tuning option loses most of
   its meaning. Would you prefer the batched behaviour to be
   unconditional (no new option), or should it stay behind a knob?

2. Any preference on the list bound - a fixed count, or derived from
   gc-cache-capacity? One observation: sorted by digest, a flush of N
   entries spans min(N, 65536) directories, so the bound also sets
   the readdir amplification - N/65536 chunks actually used per
   directory read. A large, LRU-sized bound (~8M entries is only a
   few hundred MB of digests) gives ~128 used chunks per readdir,
   while a small bound would warm ~1100 dnodes per directory to use
   a handful.

Thanks,
Enrico

Il giorno lun 10 ago 2026 alle ore 12:13 Christian Ebner <
c.ebner@proxmox.com> ha scritto:

> Hi Enrico,
>
> thanks for your contribution! For it to be considered please send the
> signed contributors license agreement as outlined in if you have not
> already done so:
>
>
> https://pbs.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright
>
> In the mean time some high level comments.
>
> On 8/10/26 7:02 AM, Enrico Plantulli wrote:
> > Hi,
> >
> > garbage collection phase 1 resolves each chunk from its digest and calls
> > utimensat() on it directly, so it never iterates the chunk directories.
> > On a cold store that means every chunk costs an independent, serialized
> > metadata read, and those reads are issued in digest order - which is
> > uncorrelated with the on-disk layout of the inodes by construction,
> > since the digest picks the directory while the inode number follows
> > creation order.
> >
>
> [..]
>
> >
> > Design notes and open questions, on which I would appreciate guidance:
> >
> > * The win is conditional on the prefetched metadata surviving in the
> >    cache until phase 1 consumes it. On ZFS that is roughly 512 bytes of
> >    dnode per chunk: about 2.5 GiB for the 5.2M chunks sampled from
> >    datastore B (twice that for the full store) and about 34 GiB for
> >    datastore A. On a store whose dnodes do not fit in the ARC the pass
> >    does not pay off. And even where they fit, whether the tail of the
> >    prefetch survives until a phase 1 that runs for hours reaches it -
> >    against the index files phase 1 itself reads, the dnodes it dirties
> >    and concurrent backup traffic - is an open question on a store of A's
> >    size. If it does not, the better design would be prefetch windows
> >    interleaved with marking, which this simple one-pass version does not
> >    attempt. Treat the datastore A figures as motivation, not as a
> >    measured result of this patch.
>
> I think there might be additional performance improvements to be gained
> from this: In particular, instead of performing the atime updates on the
> chunks on first encounter right away, these could be kept in a list with
> upper boundary, the utimensat() call deferred until the list reached
> it's maximum or there are not further chunks to process. Sorting the
> list by digest and performing the readdir() calls on that ordered list
> should give similar benefits if multiple chunks are to be touched within
> the same directory AFAIU, but without having to read empty directories
> and with the benefit of warming the cache when needed. Also, utimensat()
> calls could get the open parent directory file handle, reducing
> filesystem traversal for lookup.
>
> List limits must however be considered with care, might be based on the
> LRU cache size used to avoid multiple atime updates.
>
> > * The warm figures bound the read side only. Phase 1 also dirties every
> >    chunk's dnode through utimensat(), and in digest order two touches
> >    that share a 16 KiB metadnode block are millions of operations apart,
> >    so copy-on-write rewrites the block once per touch instead of once
> >    per block: on datastore A that is ~72M block rewrites, on the order
> >    of 1 TiB of metadnode churn plus a multiple of that in dirtied
> >    indirect blocks. This is the same volume phase 1 writes today, only
> >    compressed into less wall time - a faster phase 1 even coalesces the
> >    indirect block updates better. But it does cap the marking rate well
> >    below what the lstat() figure alone would suggest: with the default
> >    4 GiB zfs_dirty_data_max the ZFS write throttle starts to shape the
> >    rate in the low thousands of utimensat() per second. So the end
> >    state I expect on datastore A is phase 1 bound by metadata
> >    write-back at a few thousand chunks per second - two orders of
> >    magnitude above the 39.2 chunks/s it does today, but not the tens of
> >    thousands the warm read numbers alone might suggest. On datastore B
> >    the end-to-end run stayed below any throttling (~13700 chunks/s with
> >    dmu_tx_dirty_delay at 0), so the cap was not reached there; for A
> >    this remains an estimate, not a measurement.
>
> Definitely worth investigation further as well.
>
> > * Opt-in was the conservative choice. Note that tying it to chunk-order
> >    would not be conservative at all: chunk-order defaults to `inode`, so
> >    that would effectively enable the pass everywhere. If you want it on
> >    by default, I would rather make the option default to true than key it
> >    off chunk-order.
> >
> > * The pass is not free, but it is also not a whole extra pass: GC phase
> >    2 already walks the same directories with the same iterator, so the
> >    prefetch can warm phase 2 as well - though after an hours-long phase 1
> >    that rewrites the dnodes it touches, how much of that warmth is left
> >    for phase 2 is equally open.
> >
> > * The pass is best effort: a failure is logged and ignored, except for
> >    abort and shutdown requests, which are re-checked in the error path so
> >    that cancelling the task still stops the collection.
> >
> > * It reuses get_chunk_store_iterator(), the same iterator phase 2 uses,
> >    so there is no second implementation of the chunk directory walk. The
> >    per-entry hex filtering is redundant for this use, but reusing the
> >    iterator seemed better than duplicating the walk.
>
> This helper is and internal implementation, so could be extended to make
> the filtering optional.
>
> > * The pass is sequential, one directory at a time, so it keeps the queue
> >    depth of the pool low - the same limitation that makes phase 1 slow in
> >    the first place. Parallelising it over ranges of the 65536
> >    subdirectories would likely help further on wide pools, but it would
> >    need a second walk implementation instead of reusing
> >    get_chunk_store_iterator(), so I left it out of this first version.
> >    Happy to add it if you would take it.
>
> This could be added at a later point IMO.
>
> > * This is complementary to the LRU cache added in [0] for #5331: that
> >    commit removes redundant atime updates, this one makes the remaining
> >    ones cheap. It does not change what phase 1 marks, only what it has to
> >    wait for.
> >
> > * A larger version of the same idea would be to have phase 1 itself walk
> >    in inode order, the way chunk-order=inode already does for verify.
> >    That is a much more invasive change and I did not attempt it; the
> >    readdir pass gets most of the benefit for a fraction of the risk.
>
> If deferring utimensat() calls as suggested above, this could most
> likely be implemented without much hustle, requiring sorting by inode
> instead of by digest.
>
> >
> > [0]
> https://git.proxmox.com/?p=proxmox-backup.git;a=commit;h=03143eee0a59cf319be0052e139f7e20e124d572
>
>
>
>