Re: [PATCH proxmox proxmox-backup 0/3] datastore: gc: prefetch chunk metadata before phase 1

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

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