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

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

[Enrico Plantulli]

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.

How uncorrelated: on datastore A below, the 1059 chunks in a single
.chunks/ subdirectory map to 1059 *distinct* 16 KiB metadnode blocks,
zero shared, with a median gap of 77966 between consecutive object ids.

Most of the numbers below come from two datastores, please keep them
apart:

  Datastore A: 72M chunks, 159 TiB. This is the one that motivates the
    patch: phase 1 has been running at 39.2 chunks/s, which extrapolates
    to more than 21 days for phase 1 alone; full GC cycles on this store
    have taken between 21 and 66 days. No number below is a before/after
    for this patch on A.
  Datastore B: 65536 chunk directories, of which the 32768 sampled below
    hold 5231473 chunks - about half the store. All readdir/lstat
    numbers below are from B.

Measured on ZFS on rotational disks (7x raidz1 of 4 HDDs, ZFS
2.4.3-pve1, PBS 4.2.5, cold cache):

  readdir over those 32768 directories:  270.9 s,  869726 physical reads
  lstat() over the same 5231473 entries:  80.1 s,       0 physical reads

That is 0.166 physical reads per chunk brought in. A pass over all 65536
directories costs about twice the readdir figure; the pass pays an
openat+getdents even for empty directories, so its fixed cost is always
on 65536 directories.

Physical reads are the sum of the completed-read counters in
/proc/diskstats over the 28 pool members, so one 16 KiB logical metadata
read shows up as more than one device read: 869726 physical reads
against 634549 ARC misses, 1.37 per miss. Of those misses 600605 were
prefetch and 33944 demand (94.7% prefetch); the demand figure is within
4% of the 32768 ZAP headers getdents has to read anyway. The readdir
pass was run twice from a cold ARC and the two physical read counts
agreed to within 0.01%. lstat() is used as a stand-in for the read side
of the utimensat() that phase 1 does; it does not perform the atime
write. zfs_readdir() calls dmu_prefetch_dnode() for the entries it
returns, which is where the win comes from - getdents itself needs only
the directory entry, not the inode.

The same effect showed up on a third store: after a readdir-only pass
over 6979381 chunks, an lstat() over all of them cost zero physical
reads.

Patch 1 (proxmox) adds a gc-chunk-metadata-prefetch tuning option.
Patch 2 (proxmox-backup) implements the pass, wires it up and documents
it. Patch 3 (proxmox-backup) adds the GUI field.

Patches 2 and 3 depend on patch 1, so proxmox-backup needs a
pbs-api-types release and a dependency bump before it builds. I did not
touch debian/changelog or Cargo.toml.

Testing status: the series is compile-tested (cargo build, clippy, fmt,
test of the touched crates) against current master of both
repositories, using the pbs-api-types path override already present in
Cargo.toml, and I have since run it end-to-end once on datastore B in
production, with binaries rebuilt from the 4.2.5-1 tag plus this
series: the prefetch walked 10462700 entries in 27m40s (about half of
them already cached from an earlier experiment; a fully cold pass
extrapolates to roughly 55m), phase 1 then completed in 11m59s against
a 2h14m baseline measured on the same store four days earlier - about
13700 chunks/s warm against 440-563 chunks/s cold - and the ZFS
dirty-data throttle never engaged (dmu_tx_dirty_delay stayed at 0).
The full collection, including a phase 2 that swept 9.85M chunks
through the nightly backup window, finished in 6h21m with TASK OK and
removed 436 GiB. I have not tested the S3 code path, and datastore A
has not run with the option yet.

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.

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

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

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

[0] https://git.proxmox.com/?p=proxmox-backup.git;a=commit;h=03143eee0a59cf319be0052e139f7e20e124d572

Diffstat over the whole series:

proxmox:

 pbs-api-types/src/datastore.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

proxmox-backup:

 docs/storage.rst                 | 12 ++++++++++++
 pbs-datastore/src/chunk_store.rs | 38 ++++++++++++++++++++++++++++++++++++++
 pbs-datastore/src/datastore.rs   | 23 +++++++++++++++++++++++
 www/Utils.js                     |  6 ++++++
 www/datastore/OptionView.js      | 16 ++++++++++++++++
 5 files changed, 95 insertions(+)

Thanks,
Enrico Plantulli

[PATCH proxmox 1/3] pbs-api-types: add gc chunk metadata prefetch tuning option

[Enrico Plantulli]

Garbage collection phase 1 reaches each chunk by building its path from
the digest, so it never iterates the chunk directories. On file systems
that prefetch inode metadata while iterating a directory, that leaves a
large win unused: a single readdir pass over .chunks/ brings in the
metadata in bulk, while phase 1 on its own faults it in one chunk at a
time, in an order uncorrelated with the on-disk layout.

Add an opt-in tuning option so that garbage collection can do that pass
before phase 1. It defaults to false because the pass costs time on file
systems that do not prefetch inodes on readdir, and on datastores whose
metadata already fits in the cache.

Signed-off-by: Enrico Plantulli <plantulli@gmail.com>
---
 pbs-api-types/src/datastore.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 93ccaf0..76ecc5b 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -278,6 +278,14 @@ pub const GC_CACHE_CAPACITY_SCHEMA: Schema =
             schema: GC_CACHE_CAPACITY_SCHEMA,
             optional: true,
         },
+        "gc-chunk-metadata-prefetch": {
+            description:
+                "Iterate the chunk directories once before garbage collection phase 1, so the \
+                file system can fault in chunk metadata in bulk instead of one chunk at a time",
+            optional: true,
+            default: false,
+            type: bool,
+        },
         "default-verification-workers": {
             schema: VERIFY_JOB_VERIFY_THREADS_SCHEMA,
             optional: true,
@@ -304,6 +312,8 @@ pub struct DatastoreTuning {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub gc_cache_capacity: Option<usize>,
     #[serde(skip_serializing_if = "Option::is_none")]
+    pub gc_chunk_metadata_prefetch: Option<bool>,
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub default_verification_workers: Option<usize>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub default_verification_readers: Option<usize>,

base-commit: e3e3ff11b9b92fe1ace89b84c1e15c150e2db660
-- 
2.47.3

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

[Enrico Plantulli]

Phase 1 of garbage collection resolves each chunk from its digest and
calls utimensat() on it directly. It never iterates the chunk
directories, so on a cold store every chunk costs an independent,
serialized metadata read, 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.

On file systems that prefetch inode metadata while iterating a directory
this can be avoided almost entirely: iterating the chunk directories
once brings in that metadata in one go, and phase 1 then finds it
cached.

Measured on a ZFS datastore on spinning disks (7x raidz1 of 4 HDDs, ZFS
2.4.3, cold cache), over 32768 of its 65536 chunk directories, holding
5231473 chunks:

  readdir over those 32768 directories:  270.9 s,  869726 physical reads
  lstat() over the same 5231473 entries:  80.1 s,       0 physical reads

That is 0.166 physical reads per chunk brought in. 94.7% of the
resulting ARC misses were prefetch misses, and zfs_readdir() calls
dmu_prefetch_dnode() for the entries it returns, which is where the win
comes from - getdents itself needs only the directory entry, not the
inode. The pass only helps as long as that metadata is still cached when
phase 1 uses it.

Note that the pass removes the read stalls only: phase 1 still
dirties every chunk's dnode via utimensat(), so once the reads are
cached the marking rate is expected to cap at the pool's metadata
write-back rate, not at readdir/lstat speed.

The pass is not free and it does not help everywhere, so it is opt-in
via the new gc-chunk-metadata-prefetch tuning option, and it is skipped
for S3 backed datastores: there the local chunk store only holds the
cached chunks and the in-use markers, not the full chunk set, so a full
pass over the local chunk directories is not what phase 1 has to wait
for. A failing prefetch is logged and ignored, except for abort and
shutdown requests, which are re-checked in the error path: the pass is
an optimization and must never become a new way for garbage collection
to fail, but it must not swallow a cancellation either.

Signed-off-by: Enrico Plantulli <plantulli@gmail.com>
---
 docs/storage.rst                 | 12 ++++++++++
 pbs-datastore/src/chunk_store.rs | 38 ++++++++++++++++++++++++++++++++
 pbs-datastore/src/datastore.rs   | 23 +++++++++++++++++++
 3 files changed, 73 insertions(+)

diff --git a/docs/storage.rst b/docs/storage.rst
index 2c6ba0a..575ece9 100644
--- a/docs/storage.rst
+++ b/docs/storage.rst
@@ -686,6 +686,18 @@ There are some tuning related options for the datastore that are more advanced:
   cache slots, 1048576 (= 1024 * 1024) being the default, 8388608 (= 8192 *
   1024) the maximum value.
 
+* ``gc-chunk-metadata-prefetch``: Prefetch chunk metadata before GC phase 1:
+  Phase 1 reaches each chunk directly by digest and never iterates the chunk
+  directories. On file systems that prefetch inode metadata while iterating a
+  directory, such as ZFS, a single pass over the chunk directories before
+  phase 1 brings in that metadata in one go, instead of leaving phase 1 to
+  fault it in one chunk at a time. This can reduce phase 1 runtime
+  substantially on large datastores on rotational disks, at the cost of one
+  extra pass. The pass only pays off if the metadata is still cached once
+  phase 1 uses it: on ZFS that is roughly 512 bytes of dnode per chunk, so a
+  datastore whose dnodes do not fit into the ARC will not benefit. It is
+  disabled by default and has no effect on S3 backed datastores.
+
 * ``default-verification-workers`` and ``default-verification-readers``:
   Define the default number of threads used for verification and reading of chunks,
   respectively. By default, 4 threads are used for verification and 1 thread is used
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 6d2ffdb..b595d15 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -302,6 +302,44 @@ impl ChunkStore {
         Ok(is_bad)
     }
 
+    /// Iterate all chunk directories once, discarding the entries.
+    ///
+    /// No chunk file is opened, read, written or stat'ed, so no chunk access time is touched.
+    /// The readdir does update the access time of the up to 65536 `.chunks/` subdirectories,
+    /// which garbage collection never looks at: phase 2 only ever unlinks regular files, and
+    /// the atime cutoff applies to chunks, not to directories.
+    ///
+    /// The only purpose is to let the file system fault in the chunk inode metadata in bulk, so
+    /// that garbage collection phase 1 finds it cached instead of faulting it in one chunk at a
+    /// time in digest order, which is uncorrelated with the on-disk layout of the inodes.
+    ///
+    /// Returns the number of chunk directory entries seen (chunks, bad chunks and markers).
+    pub fn prefetch_chunk_metadata(&self, worker: &dyn WorkerTaskContext) -> Result<u64, Error> {
+        let mut last_percentage = 0;
+        let mut chunk_count = 0;
+
+        for (entry, percentage, _chunk_ext) in self.get_chunk_store_iterator()? {
+            if last_percentage != percentage {
+                last_percentage = percentage;
+                info!("prefetched {percentage}% ({chunk_count} entries)");
+            }
+
+            worker.check_abort()?;
+            worker.fail_on_shutdown()?;
+
+            if let Err(err) = entry {
+                bail!(
+                    "chunk iterator on chunk store '{}' failed - {err}",
+                    self.name,
+                );
+            }
+
+            chunk_count += 1;
+        }
+
+        Ok(chunk_count)
+    }
+
     fn get_chunk_store_iterator(
         &self,
     ) -> Result<
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index c3db522..da77134 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -2459,6 +2459,29 @@ impl DataStore {
             1024 * 1024
         };
 
+        if tuning.gc_chunk_metadata_prefetch.unwrap_or(false) {
+            if s3_client.is_some() {
+                info!("Chunk metadata prefetch not supported for S3 backed datastore, skipping.");
+            } else {
+                info!("Start GC chunk metadata prefetch");
+                let start = std::time::Instant::now();
+                // Best effort only: this is an optimization, it must never fail the collection.
+                match self.inner.chunk_store.prefetch_chunk_metadata(worker) {
+                    Ok(chunk_count) => info!(
+                        "Chunk metadata prefetch done, {chunk_count} entries in {}",
+                        TimeSpan::from(start.elapsed()),
+                    ),
+                    Err(err) => {
+                        // an abort or shutdown request must not be swallowed by the
+                        // best effort handling below
+                        worker.check_abort()?;
+                        worker.fail_on_shutdown()?;
+                        warn!("Chunk metadata prefetch failed, continuing without it - {err}");
+                    }
+                }
+            }
+        }
+
         info!("Start GC phase1 (mark used chunks)");
 
         self.mark_used_chunks(

base-commit: 5d95fc20eeae6df1936216b1ab1ab6952472fdcf
-- 
2.47.3

[PATCH proxmox-backup 3/3] ui: tuning: add GC chunk metadata prefetch option

[Enrico Plantulli]

The datastore option view rebuilds the tuning property string from the
fields of the form, so a tuning option that has no field is dropped as
soon as any other tuning option is edited in the GUI. Add the field for
gc-chunk-metadata-prefetch, and render it explicitly in the summary
instead of letting it fall through to the generic loop.

Signed-off-by: Enrico Plantulli <plantulli@gmail.com>
---
 www/Utils.js                |  6 ++++++
 www/datastore/OptionView.js | 16 ++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/www/Utils.js b/www/Utils.js
index d6bfd45..d09e02d 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -923,6 +923,12 @@ Ext.define('PBS.Utils', {
             `${gettext('GC cache capacity')}: ${gc_cache_capacity ?? Proxmox.Utils.defaultText}`,
         );
 
+        let gc_chunk_metadata_prefetch = tuning['gc-chunk-metadata-prefetch'];
+        delete tuning['gc-chunk-metadata-prefetch'];
+        options.push(
+            `${gettext('GC chunk metadata prefetch')}: ${gc_chunk_metadata_prefetch ?? false}`,
+        );
+
         let verification_workers = tuning['default-verification-workers'];
         delete tuning['default-verification-workers'];
         options.push(`${gettext('Default verification workers')}: ${verification_workers ?? 4}`);
diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
index 42a6104..a385633 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -338,6 +338,22 @@ Ext.define('PBS.Datastore.Options', {
                             deleteEmpty: true,
                             step: 1024,
                         },
+                        {
+                            xtype: 'proxmoxcheckbox',
+                            name: 'gc-chunk-metadata-prefetch',
+                            fieldLabel: gettext('GC Chunk Metadata Prefetch'),
+                            labelWidth: 200,
+                            autoEl: {
+                                tag: 'div',
+                                'data-qtip': gettext(
+                                    'Iterate chunk directories before GC phase 1 to load chunk metadata in bulk',
+                                ),
+                            },
+                            value: 0,
+                            uncheckedValue: 0,
+                            defaultValue: 0,
+                            deleteDefaultValue: true,
+                        },
                         {
                             xtype: 'proxmoxintegerfield',
                             name: 'default-verification-readers',
-- 
2.47.3

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

[Christian Ebner]

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

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

[Christian Ebner]

On 8/10/26 7:02 AM, Enrico Plantulli wrote:
> Phase 1 of garbage collection resolves each chunk from its digest and
> calls utimensat() on it directly. It never iterates the chunk
> directories, so on a cold store every chunk costs an independent,
> serialized metadata read, 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.
> 
> On file systems that prefetch inode metadata while iterating a directory
> this can be avoided almost entirely: iterating the chunk directories
> once brings in that metadata in one go, and phase 1 then finds it
> cached.
> 
> Measured on a ZFS datastore on spinning disks (7x raidz1 of 4 HDDs, ZFS
> 2.4.3, cold cache), over 32768 of its 65536 chunk directories, holding
> 5231473 chunks:
> 
>    readdir over those 32768 directories:  270.9 s,  869726 physical reads
>    lstat() over the same 5231473 entries:  80.1 s,       0 physical reads
> 
> That is 0.166 physical reads per chunk brought in. 94.7% of the
> resulting ARC misses were prefetch misses, and zfs_readdir() calls
> dmu_prefetch_dnode() for the entries it returns, which is where the win
> comes from - getdents itself needs only the directory entry, not the
> inode. The pass only helps as long as that metadata is still cached when
> phase 1 uses it.
> 
> Note that the pass removes the read stalls only: phase 1 still
> dirties every chunk's dnode via utimensat(), so once the reads are
> cached the marking rate is expected to cap at the pool's metadata
> write-back rate, not at readdir/lstat speed.
> 
> The pass is not free and it does not help everywhere, so it is opt-in
> via the new gc-chunk-metadata-prefetch tuning option, and it is skipped
> for S3 backed datastores: there the local chunk store only holds the
> cached chunks and the in-use markers, not the full chunk set, so a full
> pass over the local chunk directories is not what phase 1 has to wait

comment: This is however not correct, for S3 the marker file is present 
for each chunk known to be present on the backend. So the same logic and 
gain can be expected for these as well. The difference in handling is 
here mostly in phase 2, where the chunks as present on the S3 backend 
must be listed and deleted if no longer required.

> for. A failing prefetch is logged and ignored, except for abort and
> shutdown requests, which are re-checked in the error path: the pass is
> an optimization and must never become a new way for garbage collection
> to fail, but it must not swallow a cancellation either.
> 
> Signed-off-by: Enrico Plantulli <plantulli@gmail.com>
> ---
>   docs/storage.rst                 | 12 ++++++++++
>   pbs-datastore/src/chunk_store.rs | 38 ++++++++++++++++++++++++++++++++
>   pbs-datastore/src/datastore.rs   | 23 +++++++++++++++++++
>   3 files changed, 73 insertions(+)
> 
> diff --git a/docs/storage.rst b/docs/storage.rst
> index 2c6ba0a..575ece9 100644
> --- a/docs/storage.rst
> +++ b/docs/storage.rst
> @@ -686,6 +686,18 @@ There are some tuning related options for the datastore that are more advanced:
>     cache slots, 1048576 (= 1024 * 1024) being the default, 8388608 (= 8192 *
>     1024) the maximum value.
>   
> +* ``gc-chunk-metadata-prefetch``: Prefetch chunk metadata before GC phase 1:
> +  Phase 1 reaches each chunk directly by digest and never iterates the chunk
> +  directories. On file systems that prefetch inode metadata while iterating a
> +  directory, such as ZFS, a single pass over the chunk directories before
> +  phase 1 brings in that metadata in one go, instead of leaving phase 1 to
> +  fault it in one chunk at a time. This can reduce phase 1 runtime
> +  substantially on large datastores on rotational disks, at the cost of one
> +  extra pass. The pass only pays off if the metadata is still cached once
> +  phase 1 uses it: on ZFS that is roughly 512 bytes of dnode per chunk, so a
> +  datastore whose dnodes do not fit into the ARC will not benefit. It is
> +  disabled by default and has no effect on S3 backed datastores.

comment: please move the docs to a standalone patch, independent on how 
the final implementation will look like.

> +
>   * ``default-verification-workers`` and ``default-verification-readers``:
>     Define the default number of threads used for verification and reading of chunks,
>     respectively. By default, 4 threads are used for verification and 1 thread is used
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index 6d2ffdb..b595d15 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -302,6 +302,44 @@ impl ChunkStore {
>           Ok(is_bad)
>       }
>   
> +    /// Iterate all chunk directories once, discarding the entries.
> +    ///
> +    /// No chunk file is opened, read, written or stat'ed, so no chunk access time is touched.
> +    /// The readdir does update the access time of the up to 65536 `.chunks/` subdirectories,
> +    /// which garbage collection never looks at: phase 2 only ever unlinks regular files, and
> +    /// the atime cutoff applies to chunks, not to directories.
> +    ///
> +    /// The only purpose is to let the file system fault in the chunk inode metadata in bulk, so
> +    /// that garbage collection phase 1 finds it cached instead of faulting it in one chunk at a
> +    /// time in digest order, which is uncorrelated with the on-disk layout of the inodes.
> +    ///
> +    /// Returns the number of chunk directory entries seen (chunks, bad chunks and markers).
> +    pub fn prefetch_chunk_metadata(&self, worker: &dyn WorkerTaskContext) -> Result<u64, Error> {
> +        let mut last_percentage = 0;
> +        let mut chunk_count = 0;
> +
> +        for (entry, percentage, _chunk_ext) in self.get_chunk_store_iterator()? {
> +            if last_percentage != percentage {
> +                last_percentage = percentage;
> +                info!("prefetched {percentage}% ({chunk_count} entries)");
> +            }
> +
> +            worker.check_abort()?;
> +            worker.fail_on_shutdown()?;
> +
> +            if let Err(err) = entry {
> +                bail!(
> +                    "chunk iterator on chunk store '{}' failed - {err}",
> +                    self.name,
> +                );
> +            }
> +
> +            chunk_count += 1;
> +        }
> +
> +        Ok(chunk_count)
> +    }
> +
>       fn get_chunk_store_iterator(
>           &self,
>       ) -> Result<
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index c3db522..da77134 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -2459,6 +2459,29 @@ impl DataStore {
>               1024 * 1024
>           };
>   
> +        if tuning.gc_chunk_metadata_prefetch.unwrap_or(false) {
> +            if s3_client.is_some() {
> +                info!("Chunk metadata prefetch not supported for S3 backed datastore, skipping.");
> +            } else {
> +                info!("Start GC chunk metadata prefetch");
> +                let start = std::time::Instant::now();
> +                // Best effort only: this is an optimization, it must never fail the collection.
> +                match self.inner.chunk_store.prefetch_chunk_metadata(worker) {
> +                    Ok(chunk_count) => info!(
> +                        "Chunk metadata prefetch done, {chunk_count} entries in {}",
> +                        TimeSpan::from(start.elapsed()),
> +                    ),
> +                    Err(err) => {
> +                        // an abort or shutdown request must not be swallowed by the
> +                        // best effort handling below
> +                        worker.check_abort()?;
> +                        worker.fail_on_shutdown()?;
> +                        warn!("Chunk metadata prefetch failed, continuing without it - {err}");
> +                    }
> +                }
> +            }
> +        }
> +
>           info!("Start GC phase1 (mark used chunks)");
>   
>           self.mark_used_chunks(
> 
> base-commit: 5d95fc20eeae6df1936216b1ab1ab6952472fdcf

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

[Enrico Plantulli]

Already done yesterday !

Cheers

Enrico

> Il giorno 10 ago 2026, alle ore 12:19, Christian Ebner <c.ebner@proxmox.com> ha scritto:
> 
> On 8/10/26 7:02 AM, Enrico Plantulli wrote:
>> Phase 1 of garbage collection resolves each chunk from its digest and
>> calls utimensat() on it directly. It never iterates the chunk
>> directories, so on a cold store every chunk costs an independent,
>> serialized metadata read, 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.
>> On file systems that prefetch inode metadata while iterating a directory
>> this can be avoided almost entirely: iterating the chunk directories
>> once brings in that metadata in one go, and phase 1 then finds it
>> cached.
>> Measured on a ZFS datastore on spinning disks (7x raidz1 of 4 HDDs, ZFS
>> 2.4.3, cold cache), over 32768 of its 65536 chunk directories, holding
>> 5231473 chunks:
>>   readdir over those 32768 directories:  270.9 s,  869726 physical reads
>>   lstat() over the same 5231473 entries:  80.1 s,       0 physical reads
>> That is 0.166 physical reads per chunk brought in. 94.7% of the
>> resulting ARC misses were prefetch misses, and zfs_readdir() calls
>> dmu_prefetch_dnode() for the entries it returns, which is where the win
>> comes from - getdents itself needs only the directory entry, not the
>> inode. The pass only helps as long as that metadata is still cached when
>> phase 1 uses it.
>> Note that the pass removes the read stalls only: phase 1 still
>> dirties every chunk's dnode via utimensat(), so once the reads are
>> cached the marking rate is expected to cap at the pool's metadata
>> write-back rate, not at readdir/lstat speed.
>> The pass is not free and it does not help everywhere, so it is opt-in
>> via the new gc-chunk-metadata-prefetch tuning option, and it is skipped
>> for S3 backed datastores: there the local chunk store only holds the
>> cached chunks and the in-use markers, not the full chunk set, so a full
>> pass over the local chunk directories is not what phase 1 has to wait
> 
> comment: This is however not correct, for S3 the marker file is present for each chunk known to be present on the backend. So the same logic and gain can be expected for these as well. The difference in handling is here mostly in phase 2, where the chunks as present on the S3 backend must be listed and deleted if no longer required.
> 
>> for. A failing prefetch is logged and ignored, except for abort and
>> shutdown requests, which are re-checked in the error path: the pass is
>> an optimization and must never become a new way for garbage collection
>> to fail, but it must not swallow a cancellation either.
>> Signed-off-by: Enrico Plantulli <plantulli@gmail.com>
>> ---
>>  docs/storage.rst                 | 12 ++++++++++
>>  pbs-datastore/src/chunk_store.rs | 38 ++++++++++++++++++++++++++++++++
>>  pbs-datastore/src/datastore.rs   | 23 +++++++++++++++++++
>>  3 files changed, 73 insertions(+)
>> diff --git a/docs/storage.rst b/docs/storage.rst
>> index 2c6ba0a..575ece9 100644
>> --- a/docs/storage.rst
>> +++ b/docs/storage.rst
>> @@ -686,6 +686,18 @@ There are some tuning related options for the datastore that are more advanced:
>>    cache slots, 1048576 (= 1024 * 1024) being the default, 8388608 (= 8192 *
>>    1024) the maximum value.
>>  +* ``gc-chunk-metadata-prefetch``: Prefetch chunk metadata before GC phase 1:
>> +  Phase 1 reaches each chunk directly by digest and never iterates the chunk
>> +  directories. On file systems that prefetch inode metadata while iterating a
>> +  directory, such as ZFS, a single pass over the chunk directories before
>> +  phase 1 brings in that metadata in one go, instead of leaving phase 1 to
>> +  fault it in one chunk at a time. This can reduce phase 1 runtime
>> +  substantially on large datastores on rotational disks, at the cost of one
>> +  extra pass. The pass only pays off if the metadata is still cached once
>> +  phase 1 uses it: on ZFS that is roughly 512 bytes of dnode per chunk, so a
>> +  datastore whose dnodes do not fit into the ARC will not benefit. It is
>> +  disabled by default and has no effect on S3 backed datastores.
> 
> comment: please move the docs to a standalone patch, independent on how the final implementation will look like.
> 
>> +
>>  * ``default-verification-workers`` and ``default-verification-readers``:
>>    Define the default number of threads used for verification and reading of chunks,
>>    respectively. By default, 4 threads are used for verification and 1 thread is used
>> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
>> index 6d2ffdb..b595d15 100644
>> --- a/pbs-datastore/src/chunk_store.rs
>> +++ b/pbs-datastore/src/chunk_store.rs
>> @@ -302,6 +302,44 @@ impl ChunkStore {
>>          Ok(is_bad)
>>      }
>>  +    /// Iterate all chunk directories once, discarding the entries.
>> +    ///
>> +    /// No chunk file is opened, read, written or stat'ed, so no chunk access time is touched.
>> +    /// The readdir does update the access time of the up to 65536 `.chunks/` subdirectories,
>> +    /// which garbage collection never looks at: phase 2 only ever unlinks regular files, and
>> +    /// the atime cutoff applies to chunks, not to directories.
>> +    ///
>> +    /// The only purpose is to let the file system fault in the chunk inode metadata in bulk, so
>> +    /// that garbage collection phase 1 finds it cached instead of faulting it in one chunk at a
>> +    /// time in digest order, which is uncorrelated with the on-disk layout of the inodes.
>> +    ///
>> +    /// Returns the number of chunk directory entries seen (chunks, bad chunks and markers).
>> +    pub fn prefetch_chunk_metadata(&self, worker: &dyn WorkerTaskContext) -> Result<u64, Error> {
>> +        let mut last_percentage = 0;
>> +        let mut chunk_count = 0;
>> +
>> +        for (entry, percentage, _chunk_ext) in self.get_chunk_store_iterator()? {
>> +            if last_percentage != percentage {
>> +                last_percentage = percentage;
>> +                info!("prefetched {percentage}% ({chunk_count} entries)");
>> +            }
>> +
>> +            worker.check_abort()?;
>> +            worker.fail_on_shutdown()?;
>> +
>> +            if let Err(err) = entry {
>> +                bail!(
>> +                    "chunk iterator on chunk store '{}' failed - {err}",
>> +                    self.name,
>> +                );
>> +            }
>> +
>> +            chunk_count += 1;
>> +        }
>> +
>> +        Ok(chunk_count)
>> +    }
>> +
>>      fn get_chunk_store_iterator(
>>          &self,
>>      ) -> Result<
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>> index c3db522..da77134 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -2459,6 +2459,29 @@ impl DataStore {
>>              1024 * 1024
>>          };
>>  +        if tuning.gc_chunk_metadata_prefetch.unwrap_or(false) {
>> +            if s3_client.is_some() {
>> +                info!("Chunk metadata prefetch not supported for S3 backed datastore, skipping.");
>> +            } else {
>> +                info!("Start GC chunk metadata prefetch");
>> +                let start = std::time::Instant::now();
>> +                // Best effort only: this is an optimization, it must never fail the collection.
>> +                match self.inner.chunk_store.prefetch_chunk_metadata(worker) {
>> +                    Ok(chunk_count) => info!(
>> +                        "Chunk metadata prefetch done, {chunk_count} entries in {}",
>> +                        TimeSpan::from(start.elapsed()),
>> +                    ),
>> +                    Err(err) => {
>> +                        // an abort or shutdown request must not be swallowed by the
>> +                        // best effort handling below
>> +                        worker.check_abort()?;
>> +                        worker.fail_on_shutdown()?;
>> +                        warn!("Chunk metadata prefetch failed, continuing without it - {err}");
>> +                    }
>> +                }
>> +            }
>> +        }
>> +
>>          info!("Start GC phase1 (mark used chunks)");
>>            self.mark_used_chunks(
>> base-commit: 5d95fc20eeae6df1936216b1ab1ab6952472fdcf
> 
> 

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

[Enrico Plant]

[-- Attachment #1: Type: text/plain, Size: 11122 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 12893 bytes --]