From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id BE0531FF0E1 for ; Mon, 10 Aug 2026 22:32:53 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 4C8ED21733; Mon, 10 Aug 2026 22:32:53 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; t=1786393960; cv=none; d=google.com; s=arc-20260327; b=l8Rjy18cIIYNfFgJznoSaa9UTxhZRpOHb6AIMkET1/uF8FwPTkPZ1CRBdimxOZvlSA jR/lgQfLAhtM7rNtQl0OlH/bARjCNkzLjumVLCsxctCBEck0rJVfwGn+gay0q+BAkuox 7XCLEs9DrxjxFDSuCzqdbgmCVMW3kZhZvT5jIugCU5OTg0L4LMUJkxCVxu4HaegEQoFC 7runJEZ/Q9f/cu1Kkb7riOJZL8nlGj4XRQgG/Qu4ANvvkt+WihiUK6+lmqp9R/4OYxGp 4i8QI0icI2+FHoHShFmx/oUCukB7sOaJfn8GQ6NPVW86EV9DWxaCngCDsNzNeU0vZLWa LsAg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20260327; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:dkim-signature; bh=IzCqT0TmW19WUaO7KbS9slezI2x2n0y82qzTTJF4JyI=; fh=x735zFtK7d+S3igtQIHSkrR20uCSKtnfpmFHdvkFDH0=; b=XR8LFxPMZQqeBQj3yP04/LvKNeqoKgAND0OeB+zgbqNqTz5qR06rPnB1oIdT9X/MLN iwh0D9sI5Kr/S65I61fw5xBhT9vSXDC6hh2ve7KWOZUpzCfx0ML4yMU5450JQjixnM5U bbdxEsUmQNo0j+2nhKeqbF3m+ZaSyfNYi554YD55ICk8ThDJ/Wa80ddzv3kaoLVWBpLY Qq6/FspY6YjRq+F4etB14ZB6w9rDOfK3dEqy+zB4dsNIH2o5aFI5o5PFkWyZiSyQzesj 8/Q8NUceT7RM/oN32qdVcn0252Ll2qQzY4PlVP2ZI7ke6qAoFZsDpHQrOM9oEprkQdnw 0H/Q==; darn=lists.proxmox.com ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1786393960; x=1786998760; darn=lists.proxmox.com; h=content-type:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:from:to:cc:subject:date:message-id:reply-to :content-type; bh=IzCqT0TmW19WUaO7KbS9slezI2x2n0y82qzTTJF4JyI=; b=dOUZWXSh86KKZSowZKPe26+MG/40a5AjWw8PuZ+RQoF1oPuRe97eU6mnSoB/4yOyQc MdnmEjjcuXjgVsWbtPUFrCXyfcaX6fa6HtMqNOluKyc84HLemDs0gATt/kCyg++UN9fT xBZEIlWERHfhn4c9S1aP/CZ5lzoTnyOwRTdSraKPdxnbAQE+qi3QwyF6BvYoyKZdzUM3 AY6ldlkkax/ufb/aX63t/GvI/RI7wMW3LQOGpubWYcVuNsauIspSpUTWd3EfBALqdrfh 5JE/Fj+rxBK0kKupgW3O2vyYuVLMb6L+2GGLI/qU68AKh08SgVzPTPKSajuw38sYXWQC R6EA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786393960; x=1786998760; h=content-type:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to:content-type; bh=IzCqT0TmW19WUaO7KbS9slezI2x2n0y82qzTTJF4JyI=; b=ZDxmNjQq3f/J+cnhl/5c674TrqtBaZlXNZyrQpV6r9Grk+8dAAiLXuMe//dqGEn7lg 77Rs+TAa2ZFtqBI2SxLaBCtR5V287RzyIvZUu4Ur5SWRDzYkorPBtSxkjqRV5zuQUS6n ZnNNytT++SLHA+cIRQe12kS95/E4ZGDOZhgnYZbsKesiXQQVGDmAPcdgTwd63LpxkW03 /DSyjixLh4A8pmI9TR1frkNoZKKFIYWGVBN57ROsBQvi0mOLXMOBfqJz9370Y8KNPoWH Jh4OYiMRp5wZKAHUyy/haNtq3vfwUzMZieb7na158mXDWUqbdWNHyZN4uUGyQ7wA33YR LYRQ== X-Gm-Message-State: AOJu0Yy18qGy95QbfLPsHVFv3yXbMTRcCv6OGdPdIp5PlmOakF/cRlKW bsKkRGo0RsHMZoLV3iKsdsczVyme57dQK0wL/5m/kHH9D8eopxTt6j0Cpw8+1Rv0dGB2INNGHTl Cv0/veB7IHOzruspKBjYjMjHSsfFvl8lkAKazBk8= X-Gm-Gg: AR+sD136I02qMmQTCkUgSKi819RUKhYcKaO9yiQX/HRW0YJy2oslw63XqFEMNFLnhUq Xj44HGGgwTYAqzGHTiuyO4JdLidgjpNX3Mu/728gzDiMTf15NgsWeYRJIvCZs1LTmB9lrCSUUfK 77v5cpjID1ajp7s3+g5q69FDmxixJ+Bq6JnAXZ6Bpj8EF2fy34A5gnwIGrYqiALbXRlG7MXRptB YbVVNaS84ohPNE8lquUSxoUowMk0x9icPYeafA7HTF6H4EuaKpFmAzXvWQhwrMy1vSFtmpwuPSO 82d02PyiUox2H2KzCkdPN+nNXHdb4PM4zBq1GUCV71WgybhpJY9Q92YrcfYl25VO7D+QjjAlu9h Dy07bJrGve+Q= X-Received: by 2002:a17:907:f509:b0:c1c:3b06:ed03 with SMTP id a640c23a62f3a-c20c697af6bmr226667266b.23.1786393960052; Mon, 10 Aug 2026 13:32:40 -0700 (PDT) MIME-Version: 1.0 References: <20260810050201.347124-1-plantulli@gmail.com> <96d49b8d-5049-473d-989c-c98060810cc2@proxmox.com> In-Reply-To: <96d49b8d-5049-473d-989c-c98060810cc2@proxmox.com> From: Enrico Plant Date: Mon, 10 Aug 2026 22:32:28 +0200 X-Gm-Features: AUfX_mzGzhMPj729jzMTwAt9XUFyHWmI8aV2IUb3j_qk_FYT2KWPeN3_vNQWTwI Message-ID: Subject: Re: [PATCH proxmox proxmox-backup 0/3] datastore: gc: prefetch chunk metadata before phase 1 To: Christian Ebner Content-Type: multipart/alternative; boundary="0000000000006afc750658b741fd" X-SPAM-LEVEL: Spam detection results: 0 AWL -0.250 Adjusted score from AWL reputation of From: address DKIM_SIGNED 0.1 Message has a DKIM or DK signature, not necessarily valid DKIM_VALID -0.1 Message has at least one valid DKIM or DK signature DKIM_VALID_AU -0.1 Message has a valid DKIM or DK signature from author's domain DKIM_VALID_EF -0.1 Message has a valid DKIM or DK signature from envelope-from domain DMARC_PASS -0.1 DMARC pass policy FREEMAIL_FROM 0.001 Sender email is commonly abused enduser mail provider HTML_MESSAGE 0.001 HTML included in message KAM_NUMSUBJECT 0.5 Subject ends in numbers excluding current years RCVD_IN_DNSWL_NONE -0.0001 Sender listed at https://www.dnswl.org/, no trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: VPRO3RJ2KGVEVFOGR5XLQEFISERNCKZ7 X-Message-ID-Hash: VPRO3RJ2KGVEVFOGR5XLQEFISERNCKZ7 X-MailFrom: plantulli@gmail.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pbs-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --0000000000006afc750658b741fd Content-Type: text/plain; charset="UTF-8" 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 > > > > --0000000000006afc750658b741fd Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi,

thank you for the quick and thorough review!
> For it to be considered please send the signed contributors licen= se
> 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<= br>> 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 chun= ks
> to process. Sorting the list by digest and performing the readdi= r()
> calls on that ordered list should give similar benefits if mult= iple
> chunks are to be touched within the same directory AFAIU, but<= br>> without having to read empty directories and with the benefit of> warming the cache when needed.

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

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

So on a store where ph= ase 1 runs for hours, neither one warming pass
up front nor periodic ful= l re-walks solve it: the warming has to
happen right before use, which i= s exactly what your deferred-batch
design does - and it makes the surviv= al question disappear entirely.
It should also compose nicely
with th= e 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 furth= er nice win on top.

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

> If deferring utimensat() calls as suggest= ed 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 an= d sort-by-inode becomes almost free to try.

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

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

2. An= y preference on the list bound - a fixed count, or derived from
=C2=A0 = =C2=A0gc-cache-capacity? One observation: sorted by digest, a flush of N=C2=A0 =C2=A0entries spans min(N, 65536) directories, so the bound also se= ts
=C2=A0 =C2=A0the readdir amplification - N/65536 chunks actually used= per
=C2=A0 =C2=A0directory read. A large, LRU-sized bound (~8M entries = is only a
=C2=A0 =C2=A0few hundred MB of digests) gives ~128 used chunks= per readdir,
=C2=A0 =C2=A0while a small bound would warm ~1100 dnodes p= er directory to use
=C2=A0 =C2=A0a handful.

Thanks,
Enrico
=

Il giorno lun 10 ago 2026 alle ore 12:13 Christian Eb= ner <c.ebner@proxmox.com> = ha scritto:
Hi E= nrico,

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.proxm= ox.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 cal= ls
> utimensat() on it directly, so it never iterates the chunk directories= .
> On a cold store that means every chunk costs an independent, serialize= d
> 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 >=C2=A0 =C2=A0 cache until phase 1 consumes it. On ZFS that is roughly 5= 12 bytes of
>=C2=A0 =C2=A0 dnode per chunk: about 2.5 GiB for the 5.2M chunks sample= d from
>=C2=A0 =C2=A0 datastore B (twice that for the full store) and about 34 = GiB for
>=C2=A0 =C2=A0 datastore A. On a store whose dnodes do not fit in the AR= C the pass
>=C2=A0 =C2=A0 does not pay off. And even where they fit, whether the ta= il of the
>=C2=A0 =C2=A0 prefetch survives until a phase 1 that runs for hours rea= ches it -
>=C2=A0 =C2=A0 against the index files phase 1 itself reads, the dnodes = it dirties
>=C2=A0 =C2=A0 and concurrent backup traffic - is an open question on a = store of A's
>=C2=A0 =C2=A0 size. If it does not, the better design would be prefetch= windows
>=C2=A0 =C2=A0 interleaved with marking, which this simple one-pass vers= ion does not
>=C2=A0 =C2=A0 attempt. Treat the datastore A figures as motivation, not= as a
>=C2=A0 =C2=A0 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 ever= y
>=C2=A0 =C2=A0 chunk's dnode through utimensat(), and in digest orde= r two touches
>=C2=A0 =C2=A0 that share a 16 KiB metadnode block are millions of opera= tions apart,
>=C2=A0 =C2=A0 so copy-on-write rewrites the block once per touch instea= d of once
>=C2=A0 =C2=A0 per block: on datastore A that is ~72M block rewrites, on= the order
>=C2=A0 =C2=A0 of 1 TiB of metadnode churn plus a multiple of that in di= rtied
>=C2=A0 =C2=A0 indirect blocks. This is the same volume phase 1 writes t= oday, only
>=C2=A0 =C2=A0 compressed into less wall time - a faster phase 1 even co= alesces the
>=C2=A0 =C2=A0 indirect block updates better. But it does cap the markin= g rate well
>=C2=A0 =C2=A0 below what the lstat() figure alone would suggest: with t= he default
>=C2=A0 =C2=A0 4 GiB zfs_dirty_data_max the ZFS write throttle starts to= shape the
>=C2=A0 =C2=A0 rate in the low thousands of utimensat() per second. So t= he end
>=C2=A0 =C2=A0 state I expect on datastore A is phase 1 bound by metadat= a
>=C2=A0 =C2=A0 write-back at a few thousand chunks per second - two orde= rs of
>=C2=A0 =C2=A0 magnitude above the 39.2 chunks/s it does today, but not = the tens of
>=C2=A0 =C2=A0 thousands the warm read numbers alone might suggest. On d= atastore B
>=C2=A0 =C2=A0 the end-to-end run stayed below any throttling (~13700 ch= unks/s with
>=C2=A0 =C2=A0 dmu_tx_dirty_delay at 0), so the cap was not reached ther= e; for A
>=C2=A0 =C2=A0 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-orde= r
>=C2=A0 =C2=A0 would not be conservative at all: chunk-order defaults to= `inode`, so
>=C2=A0 =C2=A0 that would effectively enable the pass everywhere. If you= want it on
>=C2=A0 =C2=A0 by default, I would rather make the option default to tru= e than key it
>=C2=A0 =C2=A0 off chunk-order.
>
> * The pass is not free, but it is also not a whole extra pass: GC phas= e
>=C2=A0 =C2=A0 2 already walks the same directories with the same iterat= or, so the
>=C2=A0 =C2=A0 prefetch can warm phase 2 as well - though after an hours= -long phase 1
>=C2=A0 =C2=A0 that rewrites the dnodes it touches, how much of that war= mth is left
>=C2=A0 =C2=A0 for phase 2 is equally open.
>
> * The pass is best effort: a failure is logged and ignored, except for=
>=C2=A0 =C2=A0 abort and shutdown requests, which are re-checked in the = error path so
>=C2=A0 =C2=A0 that cancelling the task still stops the collection.
>
> * It reuses get_chunk_store_iterator(), the same iterator phase 2 uses= ,
>=C2=A0 =C2=A0 so there is no second implementation of the chunk directo= ry walk. The
>=C2=A0 =C2=A0 per-entry hex filtering is redundant for this use, but re= using the
>=C2=A0 =C2=A0 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 que= ue
>=C2=A0 =C2=A0 depth of the pool low - the same limitation that makes ph= ase 1 slow in
>=C2=A0 =C2=A0 the first place. Parallelising it over ranges of the 6553= 6
>=C2=A0 =C2=A0 subdirectories would likely help further on wide pools, b= ut it would
>=C2=A0 =C2=A0 need a second walk implementation instead of reusing
>=C2=A0 =C2=A0 get_chunk_store_iterator(), so I left it out of this firs= t version.
>=C2=A0 =C2=A0 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<= br> >=C2=A0 =C2=A0 commit removes redundant atime updates, this one makes th= e remaining
>=C2=A0 =C2=A0 ones cheap. It does not change what phase 1 marks, only w= hat it has to
>=C2=A0 =C2=A0 wait for.
>
> * A larger version of the same idea would be to have phase 1 itself wa= lk
>=C2=A0 =C2=A0 in inode order, the way chunk-order=3Dinode already does = for verify.
>=C2=A0 =C2=A0 That is a much more invasive change and I did not attempt= it; the
>=C2=A0 =C2=A0 readdir pass gets most of the benefit for a fraction of t= he 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=3Dproxmox-backup.git;a=3Dcommit;h=3D= 03143eee0a59cf319be0052e139f7e20e124d572



--0000000000006afc750658b741fd--