[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

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

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

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

[Christian Ebner]

On 8/10/26 10:32 PM, Enrico Plant wrote:
> Hi,
> 
> thank you for the quick and thorough review!
> 
>> For it to be considered please send the signed contributors license
>> agreement
> 
> Already done - I sent the signed individual CLA to office@proxmox.com
> on Aug 10, our mails probably crossed. Happy to resend if it did not
> arrive.

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

>> I think there might be additional performance improvements to be
>> gained from this: In particular, instead of performing the atime
>> updates on the chunks on first encounter right away, these could be
>> kept in a list with upper boundary, the utimensat() call deferred
>> until the list reached it's maximum or there are not further chunks
>> to process. Sorting the list by digest and performing the readdir()
>> calls on that ordered list should give similar benefits if multiple
>> chunks are to be touched within the same directory AFAIU, but
>> without having to read empty directories and with the benefit of
>> warming the cache when needed.
> 
> I can offer strong empirical support for exactly this design. Since
> sending the series I have run the one-pass version on datastore A
> (72M chunks) in production, and the one-pass approach shows its limit
> there:
> 
> - the prefetch itself was fast: 81.8M directory entries in 59m32s;
> - phase 1 then started fast, but after ~4 hours collapsed to
>    ~100-150 chunks/s. Kernel stack samples of the GC worker showed it
>    blocked in zio_wait <- dbuf_read <- zap_get_leaf_byblk (and
>    dnode_hold_impl), i.e. re-reading from disk the very metadata the
>    prefetch had loaded hours earlier;
> - the ARC had recycled those blocks: it was sitting at its adaptive
>    target (c = 138G) even though c_max was 250G, so the 7-hour-old
>    prefetched blocks were evicted long before phase 1 reached them;
> - re-running the same directory walk externally, concurrently with
>    phase 1, recovered the rate only modestly: about +30% on the rate
>    of first-touched chunks, with cold demand reads persisting at
>    ~55/s even right after the walk. With the ARC sitting at its
>    adaptive target, blocks warmed minutes earlier are already being
>    recycled by the time the marker reaches them.
> 
> So on a store where phase 1 runs for hours, neither one warming pass
> up front nor periodic full re-walks solve it: the warming has to
> happen right before use, which is exactly what your deferred-batch
> design does - and it makes the survival question disappear entirely.
> It should also compose nicely
> with the existing LRU dedup cache: the deferred entries are precisely
> the cache misses, so bounding the list relative to the LRU capacity
> sounds right to me. And taking the parent directory handle for
> utimensat() is a further nice win on top.
> 
> For scale: on datastore B, where phase 1 fits well inside the
> eviction horizon, the steady state with the one-pass version is
> prefetch 12.4s + phase 1 in 12m38s daily (down from 2h14m for
> phase 1 alone before).
> 
>> If deferring utimensat() calls as suggested above, this could most
>> likely be implemented without much hustle, requiring sorting by
>> inode instead of by digest.
> 
> Agreed - once the touches go through a sorted batch, the sort key is
> pluggable and sort-by-inode becomes almost free to try.
> 
> I will rework the series (v2) along these lines: deferred bounded
> list of pending touches, sorted flush with readdir on the directories
> actually needed, parent-dir file handles for utimensat(), and the
> iterator's hex filtering made optional as you suggested. Two
> questions before I start:
> 
> 1. With the deferred design the warming happens on demand, so the
>     original gc-chunk-metadata-prefetch tuning option loses most of
>     its meaning. Would you prefer the batched behaviour to be
>     unconditional (no new option), or should it stay behind a knob?

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

> 2. Any preference on the list bound - a fixed count, or derived from
>     gc-cache-capacity? One observation: sorted by digest, a flush of N
>     entries spans min(N, 65536) directories, so the bound also sets
>     the readdir amplification - N/65536 chunks actually used per
>     directory read. A large, LRU-sized bound (~8M entries is only a
>     few hundred MB of digests) gives ~128 used chunks per readdir,
>     while a small bound would warm ~1100 dnodes per directory to use
>     a handful.

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


[..]

[PATCH v2 proxmox proxmox-backup 0/3] datastore: gc: defer and batch chunk atime updates

[Enrico Plantulli]

Hi,

this is v2 of the chunk metadata prefetch series, reworked along the
lines suggested in the review of v1: instead of one whole-store
readdir pass before phase 1, the chunk atime updates themselves are
now deferred into a bounded list and flushed sorted by digest, opening
and iterating each chunk directory right before its chunks are
touched, with the utimensat() calls going through the open directory
file descriptor.

Changes since v1:

* dropped the whole-store prefetch pass and the
  prefetch_chunk_metadata() helper entirely;
* added deferred, batched atime updates behind the same opt-in
  gc-chunk-metadata-prefetch tuning option (kept opt-in as requested:
  backends that do not prefetch on readdir, e.g. network attached
  storage, may regress);
* added a separate gc-prefetch-batch-size tuning option (default
  1048576, range 1024 - 16M) as an independent knob, as suggested, to
  make benchmarking on different backends easy;
* the batch is used for filesystem backed datastores only; S3 keeps
  the immediate path, since its in-use markers need per-chunk
  handling anyway;
* a chunk found missing at flush time is handled as before (touch the
  .bad companions, warn), but the warning can only name the digest,
  not the referencing index file - noted in patch 2;
* GUI patch now covers both options.

Why the rework was needed, empirically: I ran the v1 one-pass version
on the 72M chunk production datastore (A in the v1 thread). The pass
itself was fast (81.8M directory entries in 59m32s), but the warmed
metadata did not survive until a phase 1 that runs for hours reached
it: the GC worker was observed blocked in
zio_wait <- dbuf_read <- zap_get_leaf_byblk (and dnode_hold_impl),
re-reading from disk the very blocks the pass had loaded hours
earlier, with the ARC sitting at its adaptive target far below
c_max. External re-walks recovered the rate only modestly (~+30%).
Warming right before use removes that window entirely, which is what
this v2 does.

The batch size bound also controls the readdir amplification: a flush
of N sorted entries spans up to min(N, 65536) directories, so about
N/65536 chunks are served per directory read. The default of 1M gives
~16 used chunks per directory read on a full store; benchmarking
larger values is exactly what the separate option is for.

Testing: compile-tested (cargo build, clippy with no new warnings,
fmt, cargo test) against current master of both repositories. I have
not yet run this exact v2 end-to-end in production; I will follow up
with figures from datastore A and B once it has. NAS-backed figures
would be very welcome from anyone with such a setup, as discussed.

CLA: signed and sent to office@proxmox.com on Aug 10; office has been
in touch.

proxmox:

Enrico Plantulli (1):
  pbs-api-types: add gc chunk metadata prefetch tuning options

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

proxmox-backup:

Enrico Plantulli (2):
  datastore: gc: optionally defer and batch chunk atime updates
  ui: tuning: add GC chunk metadata prefetch options

 docs/storage.rst                 |  20 ++++++++
 pbs-datastore/src/chunk_store.rs | 102 +++++++++++++++++++++++++++++++++++++++
 pbs-datastore/src/datastore.rs   |  93 +++++++++++++++++++++++++++++++++-----
 www/Utils.js                     |  12 +++++
 www/datastore/OptionView.js      |  27 +++++++++++
 5 files changed, 245 insertions(+), 9 deletions(-)

Thanks,
Enrico Plantulli

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

[Enrico Plantulli]

Add an opt-in option to defer and batch chunk access time updates in
garbage collection phase 1, plus a separate option for the batch size
to ease benchmarking on different storage backends.

Batched updates are flushed sorted by digest, so the chunk directories
involved can be warmed via readdir right before their chunks are
touched. Keeping the switch opt-in follows the review of the first
version of this series: storage backends that do not prefetch inode
metadata on readdir, such as some network attached storages, may see
no benefit or a regression.

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

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 93ccaf0..79ccff9 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -256,6 +256,14 @@ pub const GC_CACHE_CAPACITY_SCHEMA: Schema =
         .default(1024 * 1024)
         .schema();
 
+pub const GC_PREFETCH_BATCH_SIZE_SCHEMA: Schema = IntegerSchema::new(
+    "Batch size for deferred chunk access time updates in garbage collection phase 1",
+)
+.minimum(1024)
+.maximum(16 * 1024 * 1024)
+.default(1024 * 1024)
+.schema();
+
 #[api(
     properties: {
         "chunk-order": {
@@ -278,6 +286,18 @@ pub const GC_CACHE_CAPACITY_SCHEMA: Schema =
             schema: GC_CACHE_CAPACITY_SCHEMA,
             optional: true,
         },
+        "gc-chunk-metadata-prefetch": {
+            description:
+                "Defer and batch chunk access time updates in garbage collection phase 1, \
+                warming each chunk directory via readdir right before its chunks are touched",
+            optional: true,
+            default: false,
+            type: bool,
+        },
+        "gc-prefetch-batch-size": {
+            schema: GC_PREFETCH_BATCH_SIZE_SCHEMA,
+            optional: true,
+        },
         "default-verification-workers": {
             schema: VERIFY_JOB_VERIFY_THREADS_SCHEMA,
             optional: true,
@@ -304,6 +324,10 @@ 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 gc_prefetch_batch_size: Option<usize>,
+    #[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

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

[Christian Ebner]

On 8/11/26 11:37 AM, Enrico Plantulli wrote:
> Add an opt-in option to defer and batch chunk access time updates in
> garbage collection phase 1, plus a separate option for the batch size
> to ease benchmarking on different storage backends.

comment: The same option can be used here for both, opting in and 
defining the batch size. If the optional parameter is not set, this 
should simply imply fallback to the default atime update logic. This 
helps reduce code lines, config sizes and parsing overhead.

> Batched updates are flushed sorted by digest, so the chunk directories
> involved can be warmed via readdir right before their chunks are
> touched. Keeping the switch opt-in follows the review of the first
> version of this series: storage backends that do not prefetch inode
> metadata on readdir, such as some network attached storages, may see
> no benefit or a regression.

nit: The commit message does not need to contain review history and best 
kept on-point. I would rather prefer an additional reasoning for the 
chosen default, minimum and maximum values here.

> Signed-off-by: Enrico Plantulli <plantulli@gmail.com>
> ---
>   pbs-api-types/src/datastore.rs | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 93ccaf0..79ccff9 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -256,6 +256,14 @@ pub const GC_CACHE_CAPACITY_SCHEMA: Schema =
>           .default(1024 * 1024)
>           .schema();
>   
> +pub const GC_PREFETCH_BATCH_SIZE_SCHEMA: Schema = IntegerSchema::new(
> +    "Batch size for deferred chunk access time updates in garbage collection phase 1",
> +)
> +.minimum(1024)
> +.maximum(16 * 1024 * 1024)

nit: adding some reasoning to the commit

> +.default(1024 * 1024)
> +.schema();
> +
>   #[api(
>       properties: {
>           "chunk-order": {
> @@ -278,6 +286,18 @@ pub const GC_CACHE_CAPACITY_SCHEMA: Schema =
>               schema: GC_CACHE_CAPACITY_SCHEMA,
>               optional: true,
>           },
> +        "gc-chunk-metadata-prefetch": {
> +            description:
> +                "Defer and batch chunk access time updates in garbage collection phase 1, \
> +                warming each chunk directory via readdir right before its chunks are touched",

nit: this is an user facing description, how exactly the caches are 
warmed is an implementation detail so I suggest to not include that 
part, but rather keep this a bit more concise, but this unneeded option
should be dropped in favor of the other option anyways.

> +            optional: true,
> +            default: false,
> +            type: bool,
> +        },
> +        "gc-prefetch-batch-size": {
> +            schema: GC_PREFETCH_BATCH_SIZE_SCHEMA,
> +            optional: true,
> +        },
>           "default-verification-workers": {
>               schema: VERIFY_JOB_VERIFY_THREADS_SCHEMA,
>               optional: true,
> @@ -304,6 +324,10 @@ 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 gc_prefetch_batch_size: Option<usize>,
> +    #[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

[PATCH v2 proxmox-backup 2/3] datastore: gc: optionally defer and batch chunk atime updates

[Enrico Plantulli]

Phase 1 of garbage collection updates each chunk's access time on
first encounter, resolving the chunk directly from its digest. On a
cold store every update then pays an independent, serialized metadata
lookup, 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.

Instead of touching a chunk on first encounter, queue the digest in a
bounded list and flush the list sorted: each chunk directory involved
is opened and iterated once right before its chunks are touched, so
file systems that prefetch inode metadata on readdir serve the
following atime updates from cache, and the updates go through
utimensat() relative to the open directory file descriptor, avoiding
repeated path traversal. This supersedes the whole-store readdir pass
of the first version of this series: measured on a production ZFS
datastore with 72M chunks, a single upfront pass did not survive in
the ARC until a multi-hour phase 1 consumed it (the GC worker was
observed blocked on zio_wait re-reading ZAP leaves and dnode blocks
the pass had loaded hours earlier), while batched warming reads the
metadata seconds before it is used.

The batch is only used for filesystem backed datastores; the S3
backend keeps the immediate path, since its in-use markers need the
per-chunk handling anyway. A chunk missing at flush time is handled
as before (touch the .bad companions, log a warning), but the warning
can no longer name the referencing index file, only the digest.

Signed-off-by: Enrico Plantulli <plantulli@gmail.com>
---
 docs/storage.rst                 |  20 ++++++
 pbs-datastore/src/chunk_store.rs | 102 +++++++++++++++++++++++++++++++
 pbs-datastore/src/datastore.rs   |  93 +++++++++++++++++++++++++---
 3 files changed, 206 insertions(+), 9 deletions(-)

diff --git a/docs/storage.rst b/docs/storage.rst
index 2c6ba0a..c190aeb 100644
--- a/docs/storage.rst
+++ b/docs/storage.rst
@@ -686,6 +686,26 @@ 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``: Defer and batch chunk atime updates in GC phase 1:
+  Phase 1 normally updates each chunk's access time on first encounter, reaching the
+  chunk directly by digest. On a cold store every update then pays an independent,
+  serialized metadata lookup, in an order uncorrelated with the on-disk layout. With
+  this option the updates are instead queued and flushed in sorted batches: each chunk
+  directory involved is opened and iterated once right before its chunks are touched,
+  so file systems that prefetch inode metadata on readdir (such as ZFS) serve the
+  updates from cache, and the updates use the open directory handle, avoiding repeated
+  path lookups. This can reduce phase 1 runtime substantially on large datastores on
+  local rotational disks, at the cost of the extra directory reads. Storage backends
+  that do not prefetch on readdir (for example some network attached storages) may see
+  no benefit or a regression, which is why the option is disabled by default. It has no
+  effect on S3 backed datastores.
+
+* ``gc-prefetch-batch-size``: Batch size for the deferred atime updates:
+  Number of queued chunk atime updates per sorted flush, default 1048576. Larger
+  batches group more chunks per directory read: a flush of N entries spans up to
+  min(N, 65536) directories, so N/65536 chunks are served per directory read on
+  average. Mostly useful for benchmarking the option above.
+
 * ``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..70ee49f 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -302,6 +302,108 @@ impl ChunkStore {
         Ok(is_bad)
     }
 
+    /// Update the atime of a sorted batch of chunks, warming each chunk directory right
+    /// before its chunks are touched.
+    ///
+    /// The digests are sorted in place, so consecutive entries share their chunk directory.
+    /// Every time a new directory is entered it is opened and its entries are iterated once,
+    /// discarding the results: on file systems that prefetch inode metadata on readdir this
+    /// faults the chunk metadata in right before the atime updates need it. The updates then
+    /// go through utimensat() relative to the open directory file descriptor, avoiding the
+    /// full path lookup. The chunk store mutex is taken per touch, mirroring
+    /// cond_touch_chunk(); the readdir warming happens outside the lock.
+    ///
+    /// Clears the batch and returns the digests of chunks that did not exist, for the caller
+    /// to handle.
+    pub(super) fn touch_chunk_batch(
+        &self,
+        digests: &mut Vec<[u8; 32]>,
+        worker: &dyn WorkerTaskContext,
+    ) -> Result<Vec<[u8; 32]>, Error> {
+        // unwrap: only `None` in unit tests
+        assert!(self.locker.is_some());
+
+        use nix::dir::Dir;
+        use nix::fcntl::OFlag;
+        use nix::sys::stat::Mode;
+
+        let times: [libc::timespec; 2] = [
+            // access time -> update to now
+            libc::timespec {
+                tv_sec: 0,
+                tv_nsec: libc::UTIME_NOW,
+            },
+            // modification time -> keep as is
+            libc::timespec {
+                tv_sec: 0,
+                tv_nsec: libc::UTIME_OMIT,
+            },
+        ];
+
+        digests.sort_unstable();
+
+        let mut missing = Vec::new();
+        let mut current_prefix = std::path::PathBuf::new();
+        let mut current_dir: Option<Dir> = None;
+        let mut first = true;
+
+        for digest in digests.iter() {
+            worker.check_abort()?;
+            worker.fail_on_shutdown()?;
+
+            let prefix = digest_to_prefix(digest);
+            if first || prefix != current_prefix {
+                first = false;
+                let mut path = self.chunk_dir.clone();
+                path.push(&prefix);
+                current_dir = match Dir::open(&path, OFlag::O_RDONLY, Mode::empty()) {
+                    Ok(mut dir) => {
+                        // warm the directory: iterate the entries once, discarding them
+                        for entry in dir.iter() {
+                            if entry.is_err() {
+                                break;
+                            }
+                        }
+                        Some(dir)
+                    }
+                    Err(nix::errno::Errno::ENOENT) => None,
+                    Err(err) => bail!("unable to open chunk directory {path:?} - {err}"),
+                };
+                current_prefix = prefix;
+            }
+
+            let dir = match current_dir.as_ref() {
+                Some(dir) => dir,
+                None => {
+                    missing.push(*digest);
+                    continue;
+                }
+            };
+
+            let digest_str = hex::encode(digest);
+            let filename = std::ffi::CString::new(digest_str)?;
+            let res = {
+                let _lock = self.mutex.lock().unwrap();
+                unsafe {
+                    nix::errno::Errno::result(libc::utimensat(
+                        dir.as_raw_fd(),
+                        filename.as_ptr(),
+                        &times[0],
+                        libc::AT_SYMLINK_NOFOLLOW,
+                    ))
+                }
+            };
+            match res {
+                Ok(_) => (),
+                Err(nix::errno::Errno::ENOENT) => missing.push(*digest),
+                Err(err) => bail!("update atime failed for chunk {filename:?} - {err}"),
+            }
+        }
+
+        digests.clear();
+        Ok(missing)
+    }
+
     fn get_chunk_store_iterator(
         &self,
     ) -> Result<
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index c3db522..6b3601a 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -84,6 +84,25 @@ const S3_CLIENT_RATE_LIMITER_BASE_PATH: &str = pbs_buildcfg::rundir!("/s3/shmem/
 const NAMESPACE_MARKER_FILENAME: &str = ".namespace";
 // s3 put request times out after upload_size / 1 Kib/s, so about 2.3 hours for 8 MiB
 const CHUNK_LOCK_TIMEOUT: Duration = Duration::from_secs(3 * 60 * 60);
+
+/// Deferred chunk access time updates for garbage collection phase 1.
+///
+/// On a miss of the chunk digest LRU cache the digest is queued here instead of being
+/// touched right away; full batches are sorted and flushed via
+/// [`ChunkStore::touch_chunk_batch`], which warms each chunk directory via readdir right
+/// before its chunks are touched.
+struct GcTouchBatch {
+    digests: Vec<[u8; 32]>,
+    capacity: usize,
+}
+
+/// Per-run state of garbage collection phase 1 chunk marking.
+struct GcMarkState {
+    /// avoid multiple expensive atime updates for the same chunk
+    chunk_lru_cache: Option<LruCache<[u8; 32], ()>>,
+    /// deferred, batched atime updates (see [`GcTouchBatch`])
+    touch_batch: Option<GcTouchBatch>,
+}
 // s3 deletion batch size to avoid 1024 open files soft limit
 const S3_DELETE_BATCH_LIMIT: usize = 100;
 // max defer time for s3 batch deletions
@@ -2110,11 +2129,34 @@ impl DataStore {
     }
 
     // mark chunks  used by ``index`` as used
+    fn gc_touch_batch_flush(
+        &self,
+        batch: &mut GcTouchBatch,
+        worker: &dyn WorkerTaskContext,
+    ) -> Result<(), Error> {
+        if batch.digests.is_empty() {
+            return Ok(());
+        }
+        let missing = self
+            .inner
+            .chunk_store
+            .touch_chunk_batch(&mut batch.digests, worker)?;
+        for digest in missing {
+            // touch any corresponding .bad files to keep them around, mirroring the direct
+            // path below; the referencing index file is no longer known at this point, so
+            // the warning only carries the digest
+            self.inner.chunk_store.cond_touch_bad_chunks(&digest)?;
+            let hex = hex::encode(digest);
+            warn!("warning: unable to access non-existent chunk {hex} (deferred atime update)");
+        }
+        Ok(())
+    }
+
     fn index_mark_used_chunks(
         &self,
         index: Box<dyn IndexFile>,
         file_name: &Path, // only used for error reporting
-        chunk_lru_cache: &mut Option<LruCache<[u8; 32], ()>>,
+        mark_state: &mut GcMarkState,
         status: &mut GarbageCollectionStatus,
         worker: &dyn WorkerTaskContext,
         s3_client: Option<Arc<S3Client>>,
@@ -2128,7 +2170,7 @@ impl DataStore {
             let digest = index.index_digest(pos).unwrap();
 
             // Avoid multiple expensive atime updates by utimensat
-            if let Some(chunk_lru_cache) = chunk_lru_cache {
+            if let Some(chunk_lru_cache) = &mut mark_state.chunk_lru_cache {
                 if chunk_lru_cache.insert(*digest, (), |_| Ok(()))? {
                     if let Some(cache_stats) = status.cache_stats.as_mut() {
                         cache_stats.hits += 1;
@@ -2140,6 +2182,14 @@ impl DataStore {
                 }
             }
 
+            if let Some(batch) = &mut mark_state.touch_batch {
+                batch.digests.push(*digest);
+                if batch.digests.len() >= batch.capacity {
+                    self.gc_touch_batch_flush(batch, worker)?;
+                }
+                continue;
+            }
+
             if !self.inner.chunk_store.cond_touch_chunk(digest, false)? {
                 // touch any corresponding .bad files to keep them around, meaning if a chunk is
                 // rewritten correctly they will be removed automatically, as well as if no index
@@ -2174,6 +2224,7 @@ impl DataStore {
         status: &mut GarbageCollectionStatus,
         worker: &dyn WorkerTaskContext,
         cache_capacity: usize,
+        touch_batch: Option<GcTouchBatch>,
         s3_client: Option<Arc<S3Client>>,
     ) -> Result<(), Error> {
         // Iterate twice over the datastore to fetch index files, even if this comes with an
@@ -2190,10 +2241,13 @@ impl DataStore {
         let mut unprocessed_index_list = self.list_index_files()?;
         let mut index_count = unprocessed_index_list.len();
 
-        let mut chunk_lru_cache = if cache_capacity > 0 {
-            Some(LruCache::new(cache_capacity))
-        } else {
-            None
+        let mut mark_state = GcMarkState {
+            chunk_lru_cache: if cache_capacity > 0 {
+                Some(LruCache::new(cache_capacity))
+            } else {
+                None
+            },
+            touch_batch,
         };
         let mut processed_index_files = 0;
         let mut last_percentage: usize = 0;
@@ -2265,7 +2319,7 @@ impl DataStore {
                             self.index_mark_used_chunks(
                                 index,
                                 &path,
-                                &mut chunk_lru_cache,
+                                &mut mark_state,
                                 status,
                                 worker,
                                 s3_client.as_ref().cloned(),
@@ -2308,7 +2362,7 @@ impl DataStore {
             self.index_mark_used_chunks(
                 index,
                 &path,
-                &mut chunk_lru_cache,
+                &mut mark_state,
                 status,
                 worker,
                 s3_client.as_ref().cloned(),
@@ -2330,13 +2384,17 @@ impl DataStore {
             self.index_mark_used_chunks(
                 index,
                 path,
-                &mut chunk_lru_cache,
+                &mut mark_state,
                 status,
                 worker,
                 s3_client.as_ref().cloned(),
             )
         })?;
 
+        if let Some(batch) = mark_state.touch_batch.as_mut() {
+            self.gc_touch_batch_flush(batch, worker)?;
+        }
+
         Ok(())
     }
 
@@ -2459,12 +2517,29 @@ impl DataStore {
             1024 * 1024
         };
 
+        let touch_batch = if tuning.gc_chunk_metadata_prefetch.unwrap_or(false) {
+            if s3_client.is_some() {
+                info!("Deferred chunk atime updates not supported for S3 backed datastore.");
+                None
+            } else {
+                let capacity = tuning.gc_prefetch_batch_size.unwrap_or(1024 * 1024);
+                info!("Using deferred chunk atime updates with batch size {capacity}.");
+                Some(GcTouchBatch {
+                    digests: Vec::with_capacity(capacity),
+                    capacity,
+                })
+            }
+        } else {
+            None
+        };
+
         info!("Start GC phase1 (mark used chunks)");
 
         self.mark_used_chunks(
             &mut gc_status,
             worker,
             gc_cache_capacity,
+            touch_batch,
             s3_client.as_ref().cloned(),
         )
         .context("marking used chunks failed")?;

base-commit: 5d95fc20eeae6df1936216b1ab1ab6952472fdcf
-- 
2.47.3

Re: [PATCH v2 proxmox-backup 2/3] datastore: gc: optionally defer and batch chunk atime updates

[Christian Ebner]

On 8/11/26 11:38 AM, Enrico Plantulli wrote:
> Phase 1 of garbage collection updates each chunk's access time on
> first encounter, resolving the chunk directly from its digest. On a
> cold store every update then pays an independent, serialized metadata
> lookup, 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.
> 
> Instead of touching a chunk on first encounter, queue the digest in a
> bounded list and flush the list sorted: each chunk directory involved
> is opened and iterated once right before its chunks are touched, so
> file systems that prefetch inode metadata on readdir serve the
> following atime updates from cache, and the updates go through
> utimensat() relative to the open directory file descriptor, avoiding
> repeated path traversal. This supersedes the whole-store readdir pass
> of the first version of this series: measured on a production ZFS
> datastore with 72M chunks, a single upfront pass did not survive in
> the ARC until a multi-hour phase 1 consumed it (the GC worker was
> observed blocked on zio_wait re-reading ZAP leaves and dnode blocks
> the pass had loaded hours earlier), while batched warming reads the
> metadata seconds before it is used.
> 
> The batch is only used for filesystem backed datastores; the S3
> backend keeps the immediate path, since its in-use markers need the
> per-chunk handling anyway. A chunk missing at flush time is handled
> as before (touch the .bad companions, log a warning), but the warning
> can no longer name the referencing index file, only the digest.

comment: as mentioned also below, I think the S3 backend case can be 
handled as well here as it does not really require the s3-client and the 
per-file locking required for marker creation can be handled as well.

> 
> Signed-off-by: Enrico Plantulli <plantulli@gmail.com>
> ---
>   docs/storage.rst                 |  20 ++++++
>   pbs-datastore/src/chunk_store.rs | 102 +++++++++++++++++++++++++++++++
>   pbs-datastore/src/datastore.rs   |  93 +++++++++++++++++++++++++---
>   3 files changed, 206 insertions(+), 9 deletions(-)
> 
> diff --git a/docs/storage.rst b/docs/storage.rst
> index 2c6ba0a..c190aeb 100644
> --- a/docs/storage.rst
> +++ b/docs/storage.rst
> @@ -686,6 +686,26 @@ 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``: Defer and batch chunk atime updates in GC phase 1:
> +  Phase 1 normally updates each chunk's access time on first encounter, reaching the
> +  chunk directly by digest. On a cold store every update then pays an independent,
> +  serialized metadata lookup, in an order uncorrelated with the on-disk layout. With
> +  this option the updates are instead queued and flushed in sorted batches: each chunk
> +  directory involved is opened and iterated once right before its chunks are touched,
> +  so file systems that prefetch inode metadata on readdir (such as ZFS) serve the
> +  updates from cache, and the updates use the open directory handle, avoiding repeated
> +  path lookups. This can reduce phase 1 runtime substantially on large datastores on
> +  local rotational disks, at the cost of the extra directory reads. Storage backends
> +  that do not prefetch on readdir (for example some network attached storages) may see
> +  no benefit or a regression, which is why the option is disabled by default. It has no
> +  effect on S3 backed datastores.

comment: as mentioned in review of patch 1, these are mostly 
implementation details. I would suggest to rather not go into so much 
detail but rather be on-point.

> +
> +* ``gc-prefetch-batch-size``: Batch size for the deferred atime updates:
> +  Number of queued chunk atime updates per sorted flush, default 1048576. Larger
> +  batches group more chunks per directory read: a flush of N entries spans up to
> +  min(N, 65536) directories, so N/65536 chunks are served per directory read on
> +  average. Mostly useful for benchmarking the option above.

question: why should atime updates on N chunks cover min(N, 65536) 
directories? If all these chunks share by chance the same prefix, only 
one directory needs to be accessed.

comment: As already stated in review of version 1, a separate patch for 
the documentation changes is preferred.

>   * ``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..70ee49f 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -302,6 +302,108 @@ impl ChunkStore {
>           Ok(is_bad)
>       }
>   
> +    /// Update the atime of a sorted batch of chunks, warming each chunk directory right
> +    /// before its chunks are touched.
> +    ///
> +    /// The digests are sorted in place, so consecutive entries share their chunk directory.
> +    /// Every time a new directory is entered it is opened and its entries are iterated once,
> +    /// discarding the results: on file systems that prefetch inode metadata on readdir this
> +    /// faults the chunk metadata in right before the atime updates need it. The updates then
> +    /// go through utimensat() relative to the open directory file descriptor, avoiding the
> +    /// full path lookup. The chunk store mutex is taken per touch, mirroring
> +    /// cond_touch_chunk(); the readdir warming happens outside the lock.
> +    ///
> +    /// Clears the batch and returns the digests of chunks that did not exist, for the caller
> +    /// to handle.
> +    pub(super) fn touch_chunk_batch(
> +        &self,
> +        digests: &mut Vec<[u8; 32]>,
> +        worker: &dyn WorkerTaskContext,
> +    ) -> Result<Vec<[u8; 32]>, Error> {
> +        // unwrap: only `None` in unit tests
> +        assert!(self.locker.is_some());
> +
> +        use nix::dir::Dir;
> +        use nix::fcntl::OFlag;
> +        use nix::sys::stat::Mode;
> +
> +        let times: [libc::timespec; 2] = [
> +            // access time -> update to now
> +            libc::timespec {
> +                tv_sec: 0,
> +                tv_nsec: libc::UTIME_NOW,
> +            },
> +            // modification time -> keep as is
> +            libc::timespec {
> +                tv_sec: 0,
> +                tv_nsec: libc::UTIME_OMIT,
> +            },
> +        ];
> +
> +        digests.sort_unstable();
> +
> +        let mut missing = Vec::new();
> +        let mut current_prefix = std::path::PathBuf::new();
> +        let mut current_dir: Option<Dir> = None;
> +        let mut first = true;

nit: this flag could be avoided AFAICT, current_prefix will be different 
than prefix also for the first iteration?

> +
> +        for digest in digests.iter() {
> +            worker.check_abort()?;
> +            worker.fail_on_shutdown()?;
> +
> +            let prefix = digest_to_prefix(digest);
> +            if first || prefix != current_prefix {
> +                first = false;
> +                let mut path = self.chunk_dir.clone();
> +                path.push(&prefix);

comment: it would make sense to open the .chunk dir outside the digest 
loop and open the respective prefix directories relative to that with 
openat(). And rather call utimensat() directly if it is the only 
occurrence with that prefix, since then there is no need to to the 
dedicated directory open()/readdir(), only adding overhead in that case.

> +                current_dir = match Dir::open(&path, OFlag::O_RDONLY, Mode::empty()) {
> +                    Ok(mut dir) => {
> +                        // warm the directory: iterate the entries once, discarding them
> +                        for entry in dir.iter() {
> +                            if entry.is_err() {
> +                                break;
> +                            }
> +                        }

comment: the whole readdir part here is actually not required at all to 
obtain the speedup in my testing. What did bring the performance 
improvements for my case was the opening and keeping the file handle 
open while iterating for atime updates, see diff below for more details!

> +                        Some(dir)
> +                    }
> +                    Err(nix::errno::Errno::ENOENT) => None,

comment: a missing chunk directory is unexpected and should be treated 
as error here, only a missing chunk file can happend during regular 
operation (e.g. due to a .{i}.bad rename by verification).

> +                    Err(err) => bail!("unable to open chunk directory {path:?} - {err}"),
> +                };
> +                current_prefix = prefix;
> +            }
> +
> +            let dir = match current_dir.as_ref() {
> +                Some(dir) => dir,
> +                None => {
> +                    missing.push(*digest);
> +                    continue;
> +                }
> +            };

comment: this is no longer needed when missing digest prefix directory 
is treated as error as suggested above.

> +
> +            let digest_str = hex::encode(digest);
> +            let filename = std::ffi::CString::new(digest_str)?;
> +            let res = {
> +                let _lock = self.mutex.lock().unwrap();
> +                unsafe {
> +                    nix::errno::Errno::result(libc::utimensat(
> +                        dir.as_raw_fd(),
> +                        filename.as_ptr(),
> +                        &times[0],
> +                        libc::AT_SYMLINK_NOFOLLOW,
> +                    ))
> +                }
> +            };
> +            match res {
> +                Ok(_) => (),
> +                Err(nix::errno::Errno::ENOENT) => missing.push(*digest),

comment: I see no reason why the code path could not cover the S3 case 
as well, if the missing chunk handling is factored out into a method and 
reused for both cases. Also pushing the missing to yet another list is 
not ideal, this should rather handle that case right away, maybe via a 
callback function provided to this method?

> +                Err(err) => bail!("update atime failed for chunk {filename:?} - {err}"),
> +            }
> +        }
> +
> +        digests.clear();
> +        Ok(missing)
> +    }
> +
>       fn get_chunk_store_iterator(
>           &self,
>       ) -> Result<
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index c3db522..6b3601a 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -84,6 +84,25 @@ const S3_CLIENT_RATE_LIMITER_BASE_PATH: &str = pbs_buildcfg::rundir!("/s3/shmem/
>   const NAMESPACE_MARKER_FILENAME: &str = ".namespace";
>   // s3 put request times out after upload_size / 1 Kib/s, so about 2.3 hours for 8 MiB
>   const CHUNK_LOCK_TIMEOUT: Duration = Duration::from_secs(3 * 60 * 60);
> +
> +/// Deferred chunk access time updates for garbage collection phase 1.
> +///
> +/// On a miss of the chunk digest LRU cache the digest is queued here instead of being
> +/// touched right away; full batches are sorted and flushed via
> +/// [`ChunkStore::touch_chunk_batch`], which warms each chunk directory via readdir right
> +/// before its chunks are touched.
> +struct GcTouchBatch {
> +    digests: Vec<[u8; 32]>,

comment: this is not ideal, as it will basically double the memory 
consumption (if batch size and lru capacity are the same) since the 
chunk digest is stored 2 times now, once in the batch here, once in the 
lru cache. Unfortunately not easy to avoid though since the digest is 
also the key for the LRU cache below.

> +    capacity: usize,
> +}
> +
> +/// Per-run state of garbage collection phase 1 chunk marking.
> +struct GcMarkState {
> +    /// avoid multiple expensive atime updates for the same chunk
> +    chunk_lru_cache: Option<LruCache<[u8; 32], ()>>,
> +    /// deferred, batched atime updates (see [`GcTouchBatch`])
> +    touch_batch: Option<GcTouchBatch>,
> +}
>   // s3 deletion batch size to avoid 1024 open files soft limit
>   const S3_DELETE_BATCH_LIMIT: usize = 100;
>   // max defer time for s3 batch deletions
> @@ -2110,11 +2129,34 @@ impl DataStore {
>       }
>   
>       // mark chunks  used by ``index`` as used
> +    fn gc_touch_batch_flush(
> +        &self,
> +        batch: &mut GcTouchBatch,
> +        worker: &dyn WorkerTaskContext,
> +    ) -> Result<(), Error> {
> +        if batch.digests.is_empty() {
> +            return Ok(());
> +        }
> +        let missing = self
> +            .inner
> +            .chunk_store
> +            .touch_chunk_batch(&mut batch.digests, worker)?;
> +        for digest in missing {
> +            // touch any corresponding .bad files to keep them around, mirroring the direct
> +            // path below; the referencing index file is no longer known at this point, so
> +            // the warning only carries the digest
> +            self.inner.chunk_store.cond_touch_bad_chunks(&digest)?;
> +            let hex = hex::encode(digest);
> +            warn!("warning: unable to access non-existent chunk {hex} (deferred atime update)");
> +        }
> +        Ok(())
> +    }
> +
>       fn index_mark_used_chunks(
>           &self,
>           index: Box<dyn IndexFile>,
>           file_name: &Path, // only used for error reporting
> -        chunk_lru_cache: &mut Option<LruCache<[u8; 32], ()>>,
> +        mark_state: &mut GcMarkState,
>           status: &mut GarbageCollectionStatus,
>           worker: &dyn WorkerTaskContext,
>           s3_client: Option<Arc<S3Client>>,
> @@ -2128,7 +2170,7 @@ impl DataStore {
>               let digest = index.index_digest(pos).unwrap();
>   
>               // Avoid multiple expensive atime updates by utimensat
> -            if let Some(chunk_lru_cache) = chunk_lru_cache {
> +            if let Some(chunk_lru_cache) = &mut mark_state.chunk_lru_cache {
>                   if chunk_lru_cache.insert(*digest, (), |_| Ok(()))? {
>                       if let Some(cache_stats) = status.cache_stats.as_mut() {
>                           cache_stats.hits += 1;
> @@ -2140,6 +2182,14 @@ impl DataStore {
>                   }
>               }
>   
> +            if let Some(batch) = &mut mark_state.touch_batch {
> +                batch.digests.push(*digest);
> +                if batch.digests.len() >= batch.capacity {
> +                    self.gc_touch_batch_flush(batch, worker)?;
> +                }
> +                continue;
> +            }
> +
>               if !self.inner.chunk_store.cond_touch_chunk(digest, false)? {
>                   // touch any corresponding .bad files to keep them around, meaning if a chunk is
>                   // rewritten correctly they will be removed automatically, as well as if no index
> @@ -2174,6 +2224,7 @@ impl DataStore {
>           status: &mut GarbageCollectionStatus,
>           worker: &dyn WorkerTaskContext,
>           cache_capacity: usize,
> +        touch_batch: Option<GcTouchBatch>,
>           s3_client: Option<Arc<S3Client>>,
>       ) -> Result<(), Error> {
>           // Iterate twice over the datastore to fetch index files, even if this comes with an
> @@ -2190,10 +2241,13 @@ impl DataStore {
>           let mut unprocessed_index_list = self.list_index_files()?;
>           let mut index_count = unprocessed_index_list.len();
>   
> -        let mut chunk_lru_cache = if cache_capacity > 0 {
> -            Some(LruCache::new(cache_capacity))
> -        } else {
> -            None
> +        let mut mark_state = GcMarkState {
> +            chunk_lru_cache: if cache_capacity > 0 {
> +                Some(LruCache::new(cache_capacity))
> +            } else {
> +                None
> +            },
> +            touch_batch,
>           };
>           let mut processed_index_files = 0;
>           let mut last_percentage: usize = 0;
> @@ -2265,7 +2319,7 @@ impl DataStore {
>                               self.index_mark_used_chunks(
>                                   index,
>                                   &path,
> -                                &mut chunk_lru_cache,
> +                                &mut mark_state,
>                                   status,
>                                   worker,
>                                   s3_client.as_ref().cloned(),
> @@ -2308,7 +2362,7 @@ impl DataStore {
>               self.index_mark_used_chunks(
>                   index,
>                   &path,
> -                &mut chunk_lru_cache,
> +                &mut mark_state,
>                   status,
>                   worker,
>                   s3_client.as_ref().cloned(),
> @@ -2330,13 +2384,17 @@ impl DataStore {
>               self.index_mark_used_chunks(
>                   index,
>                   path,
> -                &mut chunk_lru_cache,
> +                &mut mark_state,
>                   status,
>                   worker,
>                   s3_client.as_ref().cloned(),
>               )
>           })?;
>   
> +        if let Some(batch) = mark_state.touch_batch.as_mut() {
> +            self.gc_touch_batch_flush(batch, worker)?;
> +        }
> +
>           Ok(())
>       }
>   
> @@ -2459,12 +2517,29 @@ impl DataStore {
>               1024 * 1024
>           };
>   
> +        let touch_batch = if tuning.gc_chunk_metadata_prefetch.unwrap_or(false) {
> +            if s3_client.is_some() {
> +                info!("Deferred chunk atime updates not supported for S3 backed datastore.");

nit: should be covered as well as it basically acts the same manner as 
for regular datastores. The s3 client does not need to be passed along, 
the DataStoreImpl's backend_config.ty checked instead.

> +                None
> +            } else {
> +                let capacity = tuning.gc_prefetch_batch_size.unwrap_or(1024 * 1024);
> +                info!("Using deferred chunk atime updates with batch size {capacity}.");
> +                Some(GcTouchBatch {
> +                    digests: Vec::with_capacity(capacity),
> +                    capacity,
> +                })
> +            }
> +        } else {
> +            None
> +        };
> +
>           info!("Start GC phase1 (mark used chunks)");
>   
>           self.mark_used_chunks(
>               &mut gc_status,
>               worker,
>               gc_cache_capacity,
> +            touch_batch,
>               s3_client.as_ref().cloned(),
>           )
>           .context("marking used chunks failed")?;
> 
> base-commit: 5d95fc20eeae6df1936216b1ab1ab6952472fdcf

As mentioned above, this exemplary diff on top already brings the 
speedups since the actual gain stems from not having to re-lookup the 
directory over and over again during the utimensat() calls.

I therefore suggest to rework and optimize based on the exemplary chages 
below (still to be fine tuned and optimized).


diff --git a/pbs-datastore/src/chunk_store.rs 
b/pbs-datastore/src/chunk_store.rs
index 70ee49f27..904b0738e 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -345,48 +345,34 @@ impl ChunkStore {
          let mut missing = Vec::new();
          let mut current_prefix = std::path::PathBuf::new();
          let mut current_dir: Option<Dir> = None;
-        let mut first = true;
+
+        let chunk_dir = Dir::open(&self.chunk_dir, OFlag::O_RDONLY, 
Mode::empty())?;

          for digest in digests.iter() {
              worker.check_abort()?;
              worker.fail_on_shutdown()?;

              let prefix = digest_to_prefix(digest);
-            if first || prefix != current_prefix {
-                first = false;
-                let mut path = self.chunk_dir.clone();
-                path.push(&prefix);
-                current_dir = match Dir::open(&path, OFlag::O_RDONLY, 
Mode::empty()) {
-                    Ok(mut dir) => {
-                        // warm the directory: iterate the entries 
once, discarding them
-                        for entry in dir.iter() {
-                            if entry.is_err() {
-                                break;
-                            }
-                        }
-                        Some(dir)
-                    }
-                    Err(nix::errno::Errno::ENOENT) => None,
-                    Err(err) => bail!("unable to open chunk directory 
{path:?} - {err}"),
+            if prefix != current_prefix {
+                current_dir = match Dir::openat(
+                    Some(chunk_dir.as_raw_fd()),
+                    &prefix,
+                    OFlag::O_RDONLY,
+                    Mode::empty(),
+                ) {
+                    Ok(dir) => Some(dir),
+                    Err(err) => bail!("unable to open chunk directory - 
{err}"),
                  };
                  current_prefix = prefix;
              }

-            let dir = match current_dir.as_ref() {
-                Some(dir) => dir,
-                None => {
-                    missing.push(*digest);
-                    continue;
-                }
-            };
-
              let digest_str = hex::encode(digest);
              let filename = std::ffi::CString::new(digest_str)?;
              let res = {
                  let _lock = self.mutex.lock().unwrap();
                  unsafe {
                      nix::errno::Errno::result(libc::utimensat(
-                        dir.as_raw_fd(),
+                        current_dir.as_ref().map(|dir| 
dir.as_raw_fd()).unwrap(),
                          filename.as_ptr(),
                          &times[0],
                          libc::AT_SYMLINK_NOFOLLOW,

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

[Enrico Plantulli]

The datastore option view rebuilds the tuning property string from
the fields of the form, so a tuning option without a field is dropped
as soon as any other tuning option is edited in the GUI. Add the
fields for gc-chunk-metadata-prefetch and gc-prefetch-batch-size, and
render them explicitly in the summary.

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

diff --git a/www/Utils.js b/www/Utils.js
index d6bfd45..014d0c9 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -923,6 +923,18 @@ 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 gc_prefetch_batch_size = tuning['gc-prefetch-batch-size'];
+        delete tuning['gc-prefetch-batch-size'];
+        options.push(
+            `${gettext('GC prefetch batch size')}: ${gc_prefetch_batch_size ?? Proxmox.Utils.defaultText}`,
+        );
+
         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..5d3e476 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -338,6 +338,33 @@ 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(
+                                    'Defer and batch chunk atime updates in GC phase 1 to load chunk metadata in bulk',
+                                ),
+                            },
+                            value: 0,
+                            uncheckedValue: 0,
+                            defaultValue: 0,
+                            deleteDefaultValue: true,
+                        },
+                        {
+                            xtype: 'proxmoxintegerfield',
+                            name: 'gc-prefetch-batch-size',
+                            fieldLabel: gettext('GC Prefetch Batch Size'),
+                            labelWidth: 200,
+                            emptyText: '1048576',
+                            minValue: 1024,
+                            maxValue: 16 * 1024 * 1024,
+                            deleteEmpty: true,
+                            step: 1024,
+                        },
                         {
                             xtype: 'proxmoxintegerfield',
                             name: 'default-verification-readers',
-- 
2.47.3

Re: [PATCH v2 proxmox proxmox-backup 0/3] datastore: gc: defer and batch chunk atime updates

[Christian Ebner]

On 8/11/26 11:37 AM, Enrico Plantulli wrote:
> Hi,
> 
> this is v2 of the chunk metadata prefetch series, reworked along the
> lines suggested in the review of v1: instead of one whole-store
> readdir pass before phase 1, the chunk atime updates themselves are
> now deferred into a bounded list and flushed sorted by digest, opening
> and iterating each chunk directory right before its chunks are
> touched, with the utimensat() calls going through the open directory
> file descriptor.
> 
> Changes since v1:
> 
> * dropped the whole-store prefetch pass and the
>    prefetch_chunk_metadata() helper entirely;
> * added deferred, batched atime updates behind the same opt-in
>    gc-chunk-metadata-prefetch tuning option (kept opt-in as requested:
>    backends that do not prefetch on readdir, e.g. network attached
>    storage, may regress);
> * added a separate gc-prefetch-batch-size tuning option (default
>    1048576, range 1024 - 16M) as an independent knob, as suggested, to
>    make benchmarking on different backends easy;
> * the batch is used for filesystem backed datastores only; S3 keeps
>    the immediate path, since its in-use markers need per-chunk
>    handling anyway;
> * a chunk found missing at flush time is handled as before (touch the
>    .bad companions, warn), but the warning can only name the digest,
>    not the referencing index file - noted in patch 2;
> * GUI patch now covers both options.
> 
> Why the rework was needed, empirically: I ran the v1 one-pass version
> on the 72M chunk production datastore (A in the v1 thread). The pass
> itself was fast (81.8M directory entries in 59m32s), but the warmed
> metadata did not survive until a phase 1 that runs for hours reached
> it: the GC worker was observed blocked in
> zio_wait <- dbuf_read <- zap_get_leaf_byblk (and dnode_hold_impl),
> re-reading from disk the very blocks the pass had loaded hours
> earlier, with the ARC sitting at its adaptive target far below
> c_max. External re-walks recovered the rate only modestly (~+30%).
> Warming right before use removes that window entirely, which is what
> this v2 does.
> 
> The batch size bound also controls the readdir amplification: a flush
> of N sorted entries spans up to min(N, 65536) directories, so about
> N/65536 chunks are served per directory read. The default of 1M gives
> ~16 used chunks per directory read on a full store; benchmarking
> larger values is exactly what the separate option is for.
> 
> Testing: compile-tested (cargo build, clippy with no new warnings,
> fmt, cargo test) against current master of both repositories. I have
> not yet run this exact v2 end-to-end in production; I will follow up
> with figures from datastore A and B once it has. NAS-backed figures
> would be very welcome from anyone with such a setup, as discussed.
> 
> CLA: signed and sent to office@proxmox.com on Aug 10; office has been
> in touch.

Thanks for v2 of the patches, they have seen an improvement over the 
previous version but still require major revision. I left more details 
on the patches, but here some general comments as well.

In general it would be desirable to only have one optional parameter to
control the GC behavior for this, see comments on patch 1 for details.

Also, while the patches improve GC performance in case of cold caches, 
they do also introduce a performance regression for hot cache case. Some 
results from my testing (5 GC runs each, dropped caches via `echo 3 > 
/proc/sys/vm/drop_caches`), measuring just phase 1 of GC:

ZFS on SDD backed datastore:
     Chunk cache: hits 2671816, misses 924604 (hit ratio 74.29%)
     On-Disk usage: 2.077 TiB
     On-Disk chunks: 924604
     Deduplication factor: 5.32

     no readdir (caches dropped):                        54.54 ± 0.28s
     no readdir (caches kept):                            6.72 ± 0.02s
     with readdir (caches dropped, default batch size):  32.66 ± 0.16s
     with readdir (caches kept, default batch size):      9.82 ± 0.97s
     with readdir (caches dropped, max batch size):      32.65 ± 0.09s

A particular observation is that the speedup for GC is observed even 
when the readdir() is not performed (result not included above, see 
comment and diff on patch 2).
The major speed gain is therefore attributed to opening the chunk prefix 
directory in a structured way, allowing the utimensat() calls to be 
relative to that open file handle brings additional gains. This could 
further be optimized by only doing the additional directory open call
if more than 1 chunk have to be touched within it, therefore reducing 
number of syscalls if not required.

Another observation is that progress logging is currently broken, since 
now the index file reading is strongly decoupled from the atime updates. 
This should be improved upon.

Also, please do provide some testing results from your side as well, 
getting additional datapoints here is desired.