[pbs-devel] [PATCH v2 proxmox-backup 0/4] add GC cache stats and fix disabled state

[pbs-devel] [PATCH v2 proxmox-backup 0/4] add GC cache stats and fix disabled state

[Christian Ebner]

Allows better fine-tuning of the garbage collection cache capacity by
providing the hit and miss count, as well as the hit ratio as output
to the garbage collection task log.

Further, fixes an issue with the cache not being disabled in case the
cache capacity was explicitly set to 0, by bypassing it altogether for
that case.

Changes since version 1 (thanks Lukas for feedback):
- Also display the cache hit ratio
- Fix the cache not being disabled when the capacity is set to 0,
  discovered while investigating the hit ratio for different
  capacities.

proxmox:

Christian Ebner (1):
  pbs api types: extend garbage collection status by cache stats

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

proxmox-backup:

Christian Ebner (3):
  tools: lru cache: document limitations for cache capacity
  garbage collection: bypass cache if gc-cache-capacity is 0
  garbage collection: track chunk cache stats and show in task log

 pbs-datastore/src/datastore.rs | 25 +++++++++++++++++++++----
 pbs-tools/src/lru_cache.rs     |  2 ++
 2 files changed, 23 insertions(+), 4 deletions(-)

-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v2 proxmox 1/4] pbs api types: extend garbage collection status by cache stats

[Christian Ebner]

Add the number of cache hits and cache misses encountered during
phase 1 of garbage collection in order to display this information
in the garbage collection task log summary.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- no changes

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

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 5bd953ac..4fb1eb80 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -1459,6 +1459,10 @@ pub struct GarbageCollectionStatus {
     pub removed_bad: usize,
     /// Number of chunks still marked as .bad after garbage collection.
     pub still_bad: usize,
+    /// Number of atime update cache hits
+    pub cache_hits: usize,
+    /// Number of atime update cache misses
+    pub cache_misses: usize,
 }
 
 #[api(
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v2 proxmox-backup 2/4] tools: lru cache: document limitations for cache capacity

[Christian Ebner]

Since commit 1e7639bf ("fixup minimum lru capacity") the minimum
cache capacity is forced to be 1 to bypass edge cases for it being 0.

Explicitly mention this in the doc comment, as this behavior can be
unexpected.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- not present in previous version

 pbs-tools/src/lru_cache.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/pbs-tools/src/lru_cache.rs b/pbs-tools/src/lru_cache.rs
index 9e0112647..94757bbf7 100644
--- a/pbs-tools/src/lru_cache.rs
+++ b/pbs-tools/src/lru_cache.rs
@@ -121,6 +121,8 @@ impl<K, V> LruCache<K, V> {
 
 impl<K: std::cmp::Eq + std::hash::Hash + Copy, V> LruCache<K, V> {
     /// Create LRU cache instance which holds up to `capacity` nodes at once.
+    ///
+    /// Forces a minimum `capacity` of 1 in case of the given value being 0.
     pub fn new(capacity: usize) -> Self {
         let capacity = capacity.max(1);
         Self {
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v2 proxmox-backup 3/4] garbage collection: bypass cache if gc-cache-capacity is 0

[Christian Ebner]

Since commit 1e7639bf ("fixup minimum lru capacity") the LRU cache
capacity is set to a minimum value of 1 to avoid issues with the edge
case of 0 capacity.

In commit f1a711c8 ("garbage collection: set phase1 LRU cache
capacity by tuning option") this was not taken into account, allowing
to set values in the range [0, 8*1024*1024] via the datastores tuning
parameters.

Bypass the cache by making it optional and do not use it if the cache
capacity is set to 0, which implies it being disabled.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- not present in previous version

 pbs-datastore/src/datastore.rs | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index cbf78ecb6..dbff84bf3 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1072,7 +1072,7 @@ impl DataStore {
         &self,
         index: Box<dyn IndexFile>,
         file_name: &Path, // only used for error reporting
-        chunk_lru_cache: &mut LruCache<[u8; 32], ()>,
+        chunk_lru_cache: &mut Option<LruCache<[u8; 32], ()>>,
         status: &mut GarbageCollectionStatus,
         worker: &dyn WorkerTaskContext,
     ) -> Result<(), Error> {
@@ -1085,8 +1085,10 @@ impl DataStore {
             let digest = index.index_digest(pos).unwrap();
 
             // Avoid multiple expensive atime updates by utimensat
-            if chunk_lru_cache.insert(*digest, ()) {
-                continue;
+            if let Some(chunk_lru_cache) = chunk_lru_cache {
+                if chunk_lru_cache.insert(*digest, ()) {
+                    continue;
+                }
             }
 
             if !self.inner.chunk_store.cond_touch_chunk(digest, false)? {
@@ -1130,7 +1132,11 @@ impl DataStore {
         let mut unprocessed_index_list = self.list_index_files()?;
         let mut index_count = unprocessed_index_list.len();
 
-        let mut chunk_lru_cache = LruCache::new(cache_capacity);
+        let mut chunk_lru_cache = if cache_capacity > 0 {
+            Some(LruCache::new(cache_capacity))
+        } else {
+            None
+        };
         let mut processed_index_files = 0;
         let mut last_percentage: usize = 0;
 
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v2 proxmox-backup 4/4] garbage collection: track chunk cache stats and show in task log

[Christian Ebner]

Count the chunk cache hits and misses and display the resulting
values and the hit ratio in the garbage collection task log summary.

This allows to investigate possible issues and tune cache capacity,
also by being able to compare to other values in the summary such
as the on disk chunk count.

Exemplary output
```
2025-05-16T22:31:53+02:00: Chunk cache: hits 15817, misses 873 (hit ratio 94.77%)
2025-05-16T22:31:53+02:00: Removed garbage: 0 B
2025-05-16T22:31:53+02:00: Removed chunks: 0
2025-05-16T22:31:53+02:00: Original data usage: 64.961 GiB
2025-05-16T22:31:53+02:00: On-Disk usage: 1.037 GiB (1.60%)
2025-05-16T22:31:53+02:00: On-Disk chunks: 874
2025-05-16T22:31:53+02:00: Deduplication factor: 62.66
2025-05-16T22:31:53+02:00: Average chunk size: 1.215 MiB
```

Sidenote: the discrepancy between cache miss counter and on-disk
chunk count in the output shown above can be attributed to the all
zero chunk, inserted during the atime update check at the start of
garbage collection, however not being referenced by any index file in
this examplary case.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- add cache hit ratio to output

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

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index dbff84bf3..fcfa7e694 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1087,8 +1087,10 @@ impl DataStore {
             // Avoid multiple expensive atime updates by utimensat
             if let Some(chunk_lru_cache) = chunk_lru_cache {
                 if chunk_lru_cache.insert(*digest, ()) {
+                    status.cache_hits += 1;
                     continue;
                 }
+                status.cache_misses += 1;
             }
 
             if !self.inner.chunk_store.cond_touch_chunk(digest, false)? {
@@ -1355,6 +1357,15 @@ impl DataStore {
                 worker,
             )?;
 
+            let total_cache_counts = gc_status.cache_hits + gc_status.cache_misses;
+            if total_cache_counts > 0 {
+                let cache_hit_ratio =
+                    (gc_status.cache_hits as f64 * 100.) / total_cache_counts as f64;
+                info!(
+                    "Chunk cache: hits {}, misses {} (hit ratio {cache_hit_ratio:.2}%)",
+                    gc_status.cache_hits, gc_status.cache_misses,
+                );
+            }
             info!(
                 "Removed garbage: {}",
                 HumanByte::from(gc_status.removed_bytes),
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH v2 proxmox 1/4] pbs api types: extend garbage collection status by cache stats

[Fabian Grünbichler]

On May 19, 2025 7:55 am, Christian Ebner wrote:
> Add the number of cache hits and cache misses encountered during
> phase 1 of garbage collection in order to display this information
> in the garbage collection task log summary.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 1:
> - no changes
> 
>  pbs-api-types/src/datastore.rs | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 5bd953ac..4fb1eb80 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -1459,6 +1459,10 @@ pub struct GarbageCollectionStatus {
>      pub removed_bad: usize,
>      /// Number of chunks still marked as .bad after garbage collection.
>      pub still_bad: usize,
> +    /// Number of atime update cache hits
> +    pub cache_hits: usize,
> +    /// Number of atime update cache misses
> +    pub cache_misses: usize,

this breaks parsing the GC status file on upgrades:

Jun 04 14:58:00 yuna proxmox-backup-proxy[1285990]: error reading gc-status: missing field `cache-hits` at line 1 column 269

reverting the dedup factor to the default of 1..

do we want to mark them as optional?

>  }
>  
>  #[api(
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] partially-applied: (subset) [PATCH v2 proxmox-backup 0/4] add GC cache stats and fix disabled state

[Fabian Grünbichler]

On Mon, 19 May 2025 07:55:14 +0200, Christian Ebner wrote:
> Allows better fine-tuning of the garbage collection cache capacity by
> providing the hit and miss count, as well as the hit ratio as output
> to the garbage collection task log.
> 
> Further, fixes an issue with the cache not being disabled in case the
> cache capacity was explicitly set to 0, by bypassing it altogether for
> that case.
> 
> [...]

Applied patches 2 and 3, see comment on 1!

[2/4] tools: lru cache: document limitations for cache capacity
      commit: ee99748fa675f109a04eea740f269dce4e875ae3
[3/4] garbage collection: bypass cache if gc-cache-capacity is 0
      commit: 5285a859dc713bad83145b82c3f62bb9f5caf84b

Best regards,
-- 
Fabian Grünbichler <f.gruenbichler@proxmox.com>


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH v2 proxmox 1/4] pbs api types: extend garbage collection status by cache stats

[Christian Ebner]

On 6/4/25 15:01, Fabian Grünbichler wrote:
> On May 19, 2025 7:55 am, Christian Ebner wrote:
>> Add the number of cache hits and cache misses encountered during
>> phase 1 of garbage collection in order to display this information
>> in the garbage collection task log summary.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>> changes since version 1:
>> - no changes
>>
>>   pbs-api-types/src/datastore.rs | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
>> index 5bd953ac..4fb1eb80 100644
>> --- a/pbs-api-types/src/datastore.rs
>> +++ b/pbs-api-types/src/datastore.rs
>> @@ -1459,6 +1459,10 @@ pub struct GarbageCollectionStatus {
>>       pub removed_bad: usize,
>>       /// Number of chunks still marked as .bad after garbage collection.
>>       pub still_bad: usize,
>> +    /// Number of atime update cache hits
>> +    pub cache_hits: usize,
>> +    /// Number of atime update cache misses
>> +    pub cache_misses: usize,
> 
> this breaks parsing the GC status file on upgrades:
> 
> Jun 04 14:58:00 yuna proxmox-backup-proxy[1285990]: error reading gc-status: missing field `cache-hits` at line 1 column 269
> 
> reverting the dedup factor to the default of 1..
> 
> do we want to mark them as optional?

Oh right, will send a new version for this remaining patch making the 
counters optional


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] superseded: [PATCH v2 proxmox-backup 0/4] add GC cache stats and fix disabled state

[Christian Ebner]

superseded by version 3:
https://lore.proxmox.com/pbs-devel/20250604153449.482640-1-c.ebner@proxmox.com/T/


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel