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 3F8651FF0E1 for ; Mon, 10 Aug 2026 12:13:24 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id ACB2521593; Mon, 10 Aug 2026 12:13:23 +0200 (CEST) Message-ID: <96d49b8d-5049-473d-989c-c98060810cc2@proxmox.com> Date: Mon, 10 Aug 2026 12:13:17 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox proxmox-backup 0/3] datastore: gc: prefetch chunk metadata before phase 1 To: Enrico Plantulli , pbs-devel@lists.proxmox.com References: <20260810050201.347124-1-plantulli@gmail.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20260810050201.347124-1-plantulli@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1786356785958 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.650 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) KAM_NUMSUBJECT 0.5 Subject ends in numbers excluding current years RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium 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: 2YND3UZCNFZ75O6WTEWADHG5RR3G3ZRB X-Message-ID-Hash: 2YND3UZCNFZ75O6WTEWADHG5RR3G3ZRB X-MailFrom: c.ebner@proxmox.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 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: 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