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

[Enrico Plantulli <plantulli@gmail.com>]

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