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

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

On 8/10/26 10:32 PM, Enrico Plant wrote:
> 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.

Thanks, checked with office, they will reach out to you for further 
clarification.

>> 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?

Keeping this as opt-in is preferable, as not all storage backends might 
benefit from this but could rather show performance regerssion. For 
example, keep in mind that there are many users running PBS datastores 
backed by some network attached storages. Performance figures for these 
are of interest as well.

> 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.

Maybe this could be an independent value provided by the tuning option 
for the time being. That would also allow for easier benchmarking by 
tuning the option accordingly.


[..]