-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: min packsize flag #3731
Feature: min packsize flag #3731
Conversation
2ef6b1c
to
14bbd80
Compare
0629472
to
dc673d1
Compare
cmd/restic/cmd_prune.go
Outdated
@@ -362,7 +364,7 @@ func prune(opts PruneOptions, gopts GlobalOptions, repo restic.Repository, usedB | |||
// if this is a data pack and --repack-cacheable-only is set => keep pack! | |||
keep(p) | |||
|
|||
case p.unusedBlobs == 0 && p.duplicateBlobs == 0 && p.tpe != restic.InvalidBlob: | |||
case p.unusedBlobs == 0 && p.duplicateBlobs == 0 && p.tpe != restic.InvalidBlob && (!opts.RepackSmall || packSize >= int64(repo.MinPackSize())): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like the --repack-small
option to primarily focus on migrating to a larger pack size. Collecting too many small files should just happen automatically. My idea would be to repack all packs which are less than 10% of the expected pack size. But only if at least one hundredth of the files in a repository fall into that category. That would require collecting a separate list of packs in addition to repackCandidates
which then has to be checked to contain a sufficient number of packs to be useful. What do you think?
Hello everyone, and thanks to @metalsp0rk for this great job that I would love to make its way into the main branch. In the following examples I will use official restic package from FreeBSD This is the output of official restic:
So far so good. Now i'm switching to custom built restic from this PR, I'm setting by default a greater pack size
Exact same output, as expected, the unused space is under threshold, and I'm not telling him to repack. Now I'm telling to repack:
This is where the unexcpected comes. To trigger the repack of everything i must force max-unused to 0
Am I missing something? Thank you if you had the patience to read this wall of text :) |
tl;dr While thinking about the Why not store individual blobs?
Why not put everything into a single large pack file?
In a nutshell, the pack size should be as large as possible while only requiring a short time to upload and while keeping the local resource usage limited. During So I think we should for now allow a range of 4-128MB with a default of 16MB for the min pack size. 4MB is the current hardcoded pack size, which seems to limit backup throughput e.g. due to frequent flushing using the local backend. 16MB is a somewhat random size, but should already drastically reduce flushing related performance problems. In addition it is small enough that 5 (the default connection count) of these pack files can be uploaded in a bit over a minute using a network connection with roughly 1MB/s upload (which still isn't that uncommon). The upper limit of 128MB can be uploaded in a second via a Gigabit connection but still doesn't consume gigantic amounts of memory. In addition it is already larger than the current pack size by a factor 32, such that I think that even larger limits are not a good idea for now. [Edit]The downsides of storing individual blobs are essentially all solved by packs of about a hundred MB. The only thing remaining is that it would be possible to reduce the number of files a bit more. Thus, even larger pack sizes primarily increase the resource consumption of restic without much of a benefit[/Edit] I'm also aware of two potential problems which make larger pack files somewhat problematic: Until #3489 is merged, it is possible to end up with several small packs from each |
257ae5c
to
f1d127e
Compare
I've rebased the PR such that now there's compression and large pack support. Setting the min pack size is now done when creating a new repository object which also ensures that the test suite uses a min pack size larger than 0 bytes. @GilGalaad |
e46b1dc
to
2299a9f
Compare
2299a9f
to
7b86435
Compare
a5089ee
to
af527eb
Compare
@Slind14 Thanks for testing. The numbers are quite unexpected. What was the exact test setup? Is "without this PR", restic 0.13.1 or the current master? Judging from the 2G best-case, I guess you're uploading to the rest-server? Does setting This PR now also includes #3489 and #3611 which probably changes some of the performance tradeoffs. But nevertheless the graph sort of looks like the uploads are stalling at some point. I've no real idea why that would happen. Just as a wild guess, is $TMPDIR (or |
I think I was unclear/misleading. We are using the async PR and merged this one into it. So we compared the async version with async + packsize. The temp dir is on an NVME raid. Disk util is not the bottleneck. |
With setting it to 4 it looks about the same. Slightly less but that is not significant enough and could be from other factors. |
af527eb
to
cff146a
Compare
@Slind14 Could you test whether the current code still has the upload performance problem with 16MB chunks? For comparison chunk sizes of 4 and 8 MB would also be highly interesting. Did you run the upload performance tests using the rest-server or the S3 backend? For the latter I've noticed that file chunks above 16MB lead to multipart uploads which could be detrimental to performance. These multipart uploads are now disabled below 200MB. |
1917a40
to
f8ceff1
Compare
71f99d3
to
b8cbef9
Compare
Rebased to resolve conflicts. |
Are there plans to include this in the next release? Would be very great because when converting to repo v2 with compression you could also increase packsize. This probably would also make conversation/compression of uncompressed packs a bit faster with bigger packsizes. |
6f1fcd6
to
987e80d
Compare
For large pack sizes we might be only interested in the first and last blob of a pack file. Thus stream a pack file in multiple parts if the gaps between requested blobs grow too large.
987e80d
to
7266f07
Compare
Rebased to fix merge conflicts. |
Thanks Michael for working on getting this merged. As reported in this page by multiple people, different setup lead to different performance. I don't think this is avoidable and kind of the user's responsibility if changing these parameters. That said, I have been using Restic with a MaxPackSize of 1024MB for a while, as I have very large repos (1TB to >30TB). Is there a reason to limit MaxPackSize to 128MB? |
The part that's bugging me is that I don't see a reason why 4MB packfiles could be faster during a backup than 16MB ones. My hope is/was that the increase will lead to improvements across the board or at least maintain the performance. That's why I'd prefer to see a few more test results first to get a good idea whether performance regressions are likely to occur for some users or not. Depending on these results it might be useful to add a corresponding note to the documentation. But yes, the fall back is to ask those affected by regressions to just specify
The reason to limit it to 128MB for now is that it is already larger than the current target pack size by a factor of 32. Usually increasing some parameter by more than an order of magnitude can lead to surprises or scaling problems. In particular, I'd like to get rid of the temporary files currently created by |
Data:
Server to Backup:
Server to Backup to:
|
Only tested with S3 in the same datacenter park with a build on the latest commit of this branch from the 1st of August:
Is it possible that this branch does not support |
Yep, the file read concurrency parameter is only include in #2750. This PR was intended to discuss and implement just the pack-size configuration. I'll trim down #2750 after merging this one. @Slind14 Thanks a lot for testing. The results look what I was hoping for, so that we can go ahead and merge the pack-size flag 🥳 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks a lot for the discussions, testing and tons of patience to get this merged!
this is a great news, we have been waiting for so long <3 |
Thanks for merging this into master. set RESTIC_PACK_SIZE=64 This should at least be documented, because you should know that you wear your SSD when backing up with large packfiles. I've checked with 4 MB packsize. There also some data is written to temp directory, but much less than with bigger packsize. On linux with a small machine (Raspberry PI) same behaviour, except that for 4 MB packsize much less is written to temp directory compared to windows. So I would add in the docs
Or something like that. |
@JsBergbau The writes with 4MB packsize are the same as the writes with 16MB or 128MB packsize. The only difference is the number of files written. Because the temp directory stores the packs only while the pack is in flight, it may appear that the total writes are lower, but that is not the case. (in fact, with a 4MB packsize, there might be a minuscule increase in writes due to more file metadata being written, due to the larger file count) I thought I had that in the documentation somewhere, at least in the original #2750 PR there was mention of it. @MichaelEischer Thank you very much for merging this! |
@metalsp0rk Sorry thats no criticism on your pullrequest. I'm really glad that it finally made it into master. When files are smaller they don't seem to get written to disk. Here a screenshot from Windows taskmanager using 16 MB and 4 MB packsize. Note taskmanager caps data transfer rate, so in fact it is much higher than 250 MB/s, so in fact there is much more data written to SSD with 16 MB packs than 4 MB packs. Backup data is read and written to D: EDIT: Also you see "system" process and not restic writing these pack files. |
That direct relationship only holds when ignoring the page cache. Disk I/O is first buffered in memory and either written after some delay or when flushing. Restic doesn't call For Windows we explicitly ask the OS not to write the data to disk (#3610) and also ensure that the file is deleted no matter what happens. But apparently Windows has decided to nevertheless write the temp files to disk. So it looks like we should try to keep the whole pack file in memory if possible. |
I think something has changed in the behaviour of windows. Maybe this is even a bug in Windows? I remember when I began using restic I was checking if restic writes out the data to disk and this didn't happen. I knew that behaviour from Duplicati https://forum.duplicati.com/t/attention-ssd-users-if-your-temp-directory-isnt-ram-backed-duplicati-will-prematurely-wear-your-ssd/5779 so I explicitly checked if restic is having the same behaviour and needs a RAM disk like Duplicati needed. But that wasn't the case. I also downloaded restic 0.9.6 to verify, but also with 0.9.6 data gets written to SSD. So I'm quite sure that there has been some change in the behaviour of Windows. Edit: Found this post #2679 (comment) where I confirmed that restic doesn't write the data to temp drive. |
Some side note: When you've forgot to set compression=max and you want to compress later with maximum you can use this PR to increase min packsize. That also leads to re-compression at maximum level. Is this behaviour intented or by accident? If it is by intent I would create a pullrequest and update the documentation accordingly, because this feature is really useful. On the downside just changing packsize requires some time because compression is also re-applied. But probably you can't just byte-copy the packs but you have to uncompress and recompress the content. |
Some data while converting v1 repo to v2 with pruning with maximum compression and 64 MB packsize on linux via First graph is where I forgot According to datarate about 8 % of tempfiles were written out to SSD. System has 32 GB of RAM where only 3,3 GB are used with restic using more than 2 GB of this memory. It might be possible the more RAM is available the less tempfiles get written to disk. |
@Slind14 First thank you for showing a test to S3. Currently, we are trying to tune restic performance to backup 85TB of data into S3 (Scality ring). Our files are all images with an average size of 60MB. Would you mind explaining what s3.connection is? Is this an I'm also wondering if we can use a bigger |
@junqfisica Yes you would specify the connections parameters as |
@MichaelEischer Thank you so much for your reply. Do you know when a stable version (0.14.0) will be released on |
We first have to release 0.14.0, which shouldn't be too far out anymore. I can't tell how long it will take to be available in other repositories. |
I just wanted to confirm, that windows seems to be very eager to write these temp files to disk. On my 64GB RAM machine with only 10GB used, while backing up I see that almost all temp files are written to disk, even though they only live for less than a second (backup to external disk with ~100MB/s and I see about 100MB/s written to my internal NVME also). The pack size seems to not make a noticeable difference here. For now I'm creating a RAM-disk pre-backup, but of course it would be nice if we could tell restic to keep the packs in memory itself. Thanks to all the devs for all the great work so far! |
What is the purpose of this change? What does it change?
Would like to add the following in order to tune restic rather than hardcoding it:
--pack-size
Was the change discussed in an issue or in the forum before?
https://forum.restic.net/t/control-the-minimal-pack-files-size/617
Supersedes #2750
Closes #2291
Checklist
changelog/unreleased/
that describes the changes for our users (template here)gofmt
on the code in all commits