Re: [pbs-devel] [PATCH v2 proxmox-backup 3/4] garbage collection: allow to keep track of already touched chunks

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

> Christian Ebner <c.ebner@proxmox.com> hat am 18.03.2025 17:53 CET geschrieben:
> 
>  
> On 3/17/25 16:39, Christian Ebner wrote:
> > On 3/17/25 15:55, Fabian Grünbichler wrote:
> >>
> >> this could avoid the memory allocation (and for bigger
> >> indices/snapshots, probably multiple reallocations via the inserts) by
> >> switching to `retain` (which gets a `&mut V` and can thus flip the value
> >> of touched while filtering in a single pass).
> >>
> >> despite the performance warning about it visiting empty buckets as well,
> >> this seems to be (slightly) faster for my test datastore (would be
> >> interesting if your test cases agree?) when benchmarking with a warmed
> >> up cache.
> >>
> >> I used the following on-top of your series (the shrink_to is to reduce
> >> memory usage in case of outliers after they've been processed):
> >>
> >> ```
> >> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/ 
> >> datastore.rs
> >> index a80343d9b..d3c3f831f 100644
> >> --- a/pbs-datastore/src/datastore.rs
> >> +++ b/pbs-datastore/src/datastore.rs
> >> @@ -1650,13 +1650,12 @@ impl TouchedChunks {
> >>       // Clear untouched chunks and reset the touched marker for others.
> >>       fn reset(&mut self) {
> >> -        let mut new_list = HashMap::new();
> >> -        for (digest, touched) in self.list.drain() {
> >> -            if touched {
> >> -                new_list.insert(digest, false);
> >> -            }
> >> -        }
> >> -        self.list = new_list;
> >> +        self.list.retain(|_digest, touched| {
> >> +            *touched = !*touched;
> >> +            !*touched
> >> +        });
> >> +        let max_capacity = 
> >> self.list.len().saturating_add(self.list.len() / 3);
> >> +        self.list.shrink_to(max_capacity);
> >>       }
> >>       // Insert the digest in the list of touched chunks.
> >> ```
> >>
> >> if the above is slower for your test inputs, then at least initializing
> >> the second HashMap with some initial capacity (the len of the previous
> >> list?) is probably sensible..
> > 
> > Okay, will check and incorporate these suggestions, thanks!
> 
> Did some testing on this, above changes do result in a slight improvement:e
> 
> Without above I do get about 26s with 1147680 utimensat calls on a 
> datastore located on spinnig disk, with it drops to about 24s (checked 
> the same number of utimensat calls).
> 
> On the datastore located on SSD I get about 15s with 566341 utimensat 
> calls without the modifications and about 14s with.
> 
> Note: These values are not comparable to the tests results listed in the 
> coverletter of v2, as the datastore contents changed since.
> 
> So this helps also for my testcase. However, see below...

memory usage and allocations would of course also be an interesting metric
- but probably a bit harder to get "clean" numbers..

> >>
> >> alternatively, we could also explore (maybe as a follow-up to
> >> immediately realize the performance gains we already know we get from
> >> the current aproach?) some sort of LRU-based approach?
> > 
> > Yeah, had something like that in mind as well because of the recent work 
> > on that. This would allow to better control the overall memory 
> > requirements for the garbage collection task as well, as it then does 
> > not depend on the index files. Keeping the cache capacity large is of 
> > course a pre-condition for that.
> 
> I did test this as well storing up to 1024 * 1024 * 32 bytes in digests 
> as keys in the LRU cache, and while there is no significant change in 
> runtime (?, this needs some double checking) as compared to the patched 
> version as shown above, the utimensat call count dropped to 1078946 for 
> my particular test case on the spinning disk and 560833 on the SSD. 
> Therefore I will further pursue this approach given the already 
> mentioned advantages.
> 
> The question then remains, do we want to keep the changes to the image 
> list iteration, or even drop that as well in favor of reduced complexity 
> and slight increase in performance?

it still has a few advantages I think:
- it would allow us to integrate a "mark backups with missing chunks as
  incomplete" mode for GC "for free" (light-weight/logical verification)
- it makes it a lot easier to distinguish weird index files or other
  misconfigurations that the user might not otherwise become aware of (a
  warning at the end of each GC run that there were index files in strange
  places should hopefully worry many people?)

> I do not see much gain from that anymore...


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