Supercedes PR #2671

I was playing a bit with larger pack size (128MB) and found one thing.
Restic (internal/pack/pack.go) has internal limit for header rsize + internal estimate on how much entries is usually placed in pack:

	maxHeaderSize = 16 * 1024 * 1024
	// number of header enries to download as part of header-length request
	eagerEntries = 15

I think that maxHeaderSize is still ok with 128MB pack, but probably can be hitted with some insane pack size like 1GB.

At the same time I think that it's better to adjust eagerEntries proportionally to --min-packsize. restic uses this value during rebuild-index to download tail of pack file from backend. And if such estimate was wrong (not whole header is downloaded), restic will download it gain (now with correct size obtained from already downloaded part of pack)

Even with --min-pack-size 32 currently rebuild-index uses two requests instead of one. I understand that in any case total requests number will be smaller (due to reduced number of pack files), but small fix for this will reduce it two times more...

Even better:

// readHeader reads the header at the end of rd. size is the length of the
// whole data accessible in rd.
func readHeader(rd io.ReaderAt, size int64) ([]byte, error) {

We've actual pack size here. So just some formula should work.
I think that current eagerEntries value (15) is very close to 4*(packSize / averageblobSize). For current 4MB pack and 1MB blob size it'll be 16.

So something like eagerEntries := 4*size/1024*1024 should be ok.
I can create additional pull request for this if needed or feel free to integrate to this one.

While I definitely think this is a great improvement, and I want to implement it, there seems to be a complete rework of the prune functionality, which will likely change the way rebuild-index works. I'm not sure what exactly is happening with it, since I haven't really looked through it much, but I did read the head of it.

#2718 for reference.

I'll sleep on it, but it's probably not a bad idea to implement this now, seeing how it might take a while to get that other PR into master.

While I definitely think this is a great improvement, and I want to implement it, there seems to be a complete rework of the prune functionality, which will likely change the way rebuild-index works.

More precisely, this re-implementation of prune does not use the rebuild-index functionality (i.e. reading all pack headers) any longer.

However, rebuild-index is not changed and there might be still some (rare) circumstances where you do want to use it.

Moreover, reading the pack header is also used by check (when set to checking pack files) and the repack algorithm used within the current and new prune implementation to repack packs also reads the pack header.

So I think it is a good idea to also change the handling of pack-header-reading within this PR and independently from prune changes.

EDIT: I forgot to mention that the new prune implementation allows to repack "small" packs, where small is exactly defined by repository.MinPackSize - so together with this PR it would allow to even more fine-tune the pruning process if needed.

Ah, excellent. That makes it more clear about the use case of rebuild-index.

I definitely agree that it's useful to implement it here. I'll go ahead and do that, when I get some time. I've suddenly found myself with a huge amount of work on my hands.

good work @metalsp0rk

So I would suggest to merge this PR without changing eagerEntries but open a new issue.

I'm learning more about the internals of Restic every day. :)

Now that you've given that explainer, I definitely think that it's probably a bit too much scope creep and agree with your assessment of saving that for a new issue.

Everything's been resolved, I bolstered the docs a bit more as well, I think this is probably ready to merge, if the tests pass.

Unless I've missed something (or a backend is horribly memory inefficient), restic shouldn't load full pack files into memory. So where does this change actually reduce memory usage? In my opinion the number of SaveBlob workers should probably rather line up with the backend connection limit.

Oh that was my fault - I wrongly gave that hint. In fact you are right and it is not the full pack file. However restic holds chunk size in memory for each chunker - and this is one per concurrent writer. So for machines with many CPUs and little RAM it may make sense to reduce the number of concurrent writers.

The initial info I'm getting on benchmarking this (on a spinning disk only, so far) is that larger pack sizes are very helpful. I've found that the sweet spot is somewhere between 24MB and 32MB on my USB 5400rpm disk.

At the moment, I'm considering a recommendation to increase the default min pack size to 8MB or 12MB. Is there any specific reason we've chosen 4MB?

I'll share my results when I've finished the benchmarks.

Here is a link with @fd0 comment why 4MB was used:
https://forum.restic.net/t/slow-speeds-of-restic-with-google-drive-rclone-backend/818/15

Hi,

I'm looking forward for this feature.
On Borg I was used to tuneup the size of a chunk. By default, it is something like 500MB, which causes problems with Backblaze. Sometimes this doesn't feat to the server I send the chunk.
So I set the config in the repository to 100MB.

My backups on borg contain around than 5K files, which feat really well in terms of exchange and speed with Backblaze.

I switch to restic, which do a really good job for the simplicity and the speed. I also like the tagging, and the cleanup by tag. I can do 1 backup with different path, tag them (documents, configs, ...) and apply prune by tags.

For now the same backups on backblaze use almost 200K files, which I would like to reduce with bigger chunk.
a 24M or 32M chunk would reduce the number of files to 25K, which would be awesome!

I'm aware of memory usage that could be higher. May be something like a warning:
"You need 1GB / 4Mb chunk for this to work, ensure all your machine have this"
Or something like that.

Also if this setting can be saved on the repository himself, so we can set this once and use the same value from all the machines we have? "restic -r XXX set chunkMinSize=32"

Okay, so. I've been busy with renovations; so I lost track of this. What follows are my notes:

Repo Total Size: 14.8G

CPU: i5-8300h, mitigations on, ht on.
Source Disk: Samsung 970 Pro 512GB
Dest Disk: WD EasyStore 8TB External USB 3.0
Backup Contents: 12G of git packs, source code, image assets. 2G of video files, 365355 total files

Notes: Dropping caches after every run.

So upon quick analysis, it appears we can significantly improve performance by increasing the file read concurrency. I suspect this is due to an inefficiency in the way that the core chunk pipeline works, however, I do not have time to confirm that at the moment.

Now, I've done additional testing (but did not collect metrics on it) which showed that increasing file read concurrency negatively effects spinning rust. So to that end, I do not recommend changing the global default at this time.

@vincent-ogury to answer your questions:

I'm aware of memory usage that could be higher. May be something like a warning

A warning is a good idea. I'm not sure what the exact memory usage will correlate to, which is why I have elected to exclude it for the time being.

For now the same backups on backblaze use almost 200K files, which I would like to reduce with bigger chunk.

I'm going to do some benchmarking on backblaze with larger chunks. I suspect there will be some inefficiencies when fetching chunks for restores of small files, if you choose to go up much past 32M chunks.

Also if this setting can be saved on the repository himself, so we can set this once and use the same value from all the machines we have?

Hmmm, that's a good idea. At the moment, I do not know if we have the capability to save options like this. Maybe someone with more experience can chime in? I'd love to include this as an option.

All in all, I think this is ready for merge or at least very close. At time of writing, there are merge conflicts, so I'll give it a rebase when I get some time. In the interim, @MichaelEischer do these benchmarks satisfy your curiosity on the file-read-concurrency question? I'm happy to do more if need be, but I think I'll automate them in the future.

Some of the labels in the last sixth of your table seem to be broken. '1 default' in the SaveBlob column can't be right.

Is VmPeak the same as maximum resident set size? Or does this refer to the maximum virtual memory usage? In the latter case it is not meaningful. A higher file read concurrency means that go has to spawn more os threads and these allocate e.g. on linux amd64 by default 8MB or more. However, only the part of the stack that is actually used, will be backed by real memory. Therefore it's no problem to allocate dozens of GB of virtual memory even if your system just has 1GB; well, that is until you try to use it.

It appears that Restic's archiver does not efficiently write out packs at large pack sizes. Some funny-business happening here.

From which rows in your measurements do you derive this? The row with that remark looks pretty much similar to the previous ones. That is from my point of view the first 6 row say that for the default concurrency it pretty much doesn't matter which packsize is used.

So upon quick analysis, it appears we can significantly improve performance by increasing the file read concurrency. I suspect this is due to an inefficiency in the way that the core chunk pipeline works, however, I do not have time to confirm that at the moment.

The optimal file read concurrency depends on the filesystem and HDD/SSD. The current value '2' for the file read concurrency is the result of some quite extensive tests and discussion (can't remember in which issue that discussion took place). The inefficiency in the pipeline is pretty sure #2696 which causes file savers to wait for a pack upload to complete before continuing and therefore cause pipeline "stalls". You could test for that case by only backing up a few large files.

The blob savers currently have three tasks: Calculate the block checksum, assemble packs and upload them to the backend. A single blob saver therefore means that no uploads and pack assembly happen concurrently. So it's expected that you need a few blob savers for reasonable performance.

That is a file read concurrency of 2 should be fine in general once #2696 is fixed. Manually increasing the number of BlobSavers beyond number of CPU cores + maximum backend concurrency won't be useful and in the worst case can only increase memory usage. But I'm not sure whether we actually have a memory usage problem here at all.

Also if this setting can be saved on the repository himself, so we can set this once and use the same value from all the machines we have?

Hmmm, that's a good idea. At the moment, I do not know if we have the capability to save options like this. Maybe someone with more experience can chime in? I'd love to include this as an option.

There's currently no way to store configuration in the repository.

Pack files have a size limit for their header of maxHeaderSize = 16MB. With the increased pack size it's actually possible to hit that limit for some pathological cases in which case a pack file would become unreadable. We need a check to complete a pack file once / before it hits that limit.

Is VmPeak the same as maximum resident set size? Or does this refer to the maximum virtual memory usage?

VmPeak is defined as Peak virtual memory size. I think I snagged the wrong metric.

I may have been more interested in VmHWM, since that's the "peak Resident Set Size"

From which rows in your measurements do you derive this? The row with that remark looks pretty much similar to the previous ones. That is from my point of view the first 6 row say that for the default concurrency it pretty much doesn't matter which packsize is used.

I'm looking at the diminishing returns of the increased pack size. 5:58 to 5:54 is much smaller than 5:43 -> 5:20 or 5:20 -> 5:06

Some of the labels in the last sixth of your table seem to be broken. '1 default' in the SaveBlob column can't be right.

Yes, that can't be right. It should have simply been "1"

That is a file read concurrency of 2 should be fine in general once #2696 is fixed. Manually increasing the number of BlobSavers beyond number of CPU cores + maximum backend concurrency won't be useful and in the worst case can only increase memory usage. But I'm not sure whether we actually have a memory usage problem here at all.

I'm inclined to agree with you here. I think I may be seeing symptoms of that issue in my test results.

The blob savers currently have three tasks: Calculate the block checksum, assemble packs and upload them to the backend. A single blob saver therefore means that no uploads and pack assembly happen concurrently. So it's expected that you need a few blob savers for reasonable performance.

Agreed.

I'm coming late into this particular thread, however for reference I'm the person that originally put a very naive PoC together that @madsi1m then turned into this original PR, subsequently taken up by @metalsp0rk. The original desire is worth sharing in the particular context of it being adjustable.

Thank you for all the discussion on this, I appreciate the thought being put into it, especially given I've not been able to give it the due effort myself.

The optimal file read concurrency depends on the filesystem and HDD/SSD. The current value '2' for the file read concurrency is the result of some quite extensive tests and discussion (can't remember in which issue that discussion took place).

I can entirely appreciate a relatively low number for direct attached storage in a single machine, and why we'd want to discourage many users from turning this number up too high.

In my particular use case, I have dedicated client nodes attached to a large distributed namespace holding nearly a billion files and just shy of 10PB, and growing. The limiting factor on any IO event is usually either an individual disk on a storage node in the case of a single file, or in the aggregate case, the CPU or network of a reader node. Statistically I'm extremely unlikely to access the same storage disk across several thousand threads for a read on any random file.

Other backup tools might be more suited if we were using some filesystem products, however the restic paradigm and method fits exceptionally well in the context of what a useful backup actually constitutes to us. We don't backup whole filesystems, we instead run nearly 100,000 individual jobs defined programmatically, as these constitute the atomic units that matter from the perspective of an end user. These do range from a handful of files, to upwards of 500TB and tens of millions of files, and we have very little to no control over what the structured layout is.

In experiments using my naive PoC, the thread count was incredibly useful, especially when using either NVMe or a RAM disk as the temporary directory, and I was easily saturating the 10Gbps network interfaces into my particular development node. Given the previously mentioned namespace design, as expensive as it maybe, it was near linear throughput benefit by adding more CPUs and more network bandwidth across the job set.

Pack files have a size limit for their header of maxHeaderSize = 16MB. With the increased pack size it's actually possible to hit that limit for some pathological cases in which case a pack file would become unreadable. We need a check to complete a pack file once / before it hits that limit.

The interest in larger packs was to reduce the pack count stored within any restic repository. As I mentioned above, there are nearly a billion files being written onto offline archival storage, and the operational time cost for retrieving an offline file for access is the robot loading time and the tape seek. The cost of reading many thousands of files potentially scattered across tapes was far higher in terms of time and wear on the system, than reading large packs of a gigabyte or so even if we threw away a lot of the read as unwanted data.

In experiments using my naive PoC, the thread count was incredibly useful, especially when using either NVMe or a RAM disk as the temporary directory, and I was easily saturating the 10Gbps network interfaces into my particular development node. Given the previously mentioned namespace design, as expensive as it maybe, it was near linear throughput benefit by adding more CPUs and more network bandwidth across the job set.

Well, that use case seems rather different from what's been discussed in this PR so far (at least the throughput is an order of magnitude larger) and this also leads to new bottlenecks: With two just two file readers (= CPU cores) is definitely not possible to saturate a 10Gbps network link. The chunker implementation alone is currently capable of processing "just" 500-600 MB/s and with additional overhead from the file access and other processing steps the throughput will probably stay well below 1GB/s.

Pack files have a size limit for their header of maxHeaderSize = 16MB. With the increased pack size it's actually possible to hit that limit for some pathological cases in which case a pack file would become unreadable. We need a check to complete a pack file once / before it hits that limit.

The interest in larger packs was to reduce the pack count stored within any restic repository. As I mentioned above, there are nearly a billion files being written onto offline archival storage, and the operational time cost for retrieving an offline file for access is the robot loading time and the tape seek. The cost of reading many thousands of files potentially scattered across tapes was far higher in terms of time and wear on the system, than reading large packs of a gigabyte or so even if we threw away a lot of the read as unwanted data.

The 16MB header size limit allows roughly 450k blobs per pack file (one pack header entry has 37 bytes). To fill up a 1GB pack file this would require an average filesize of over (1024−16)×1024^2/(16×1024^2/37)−32 = 2299 bytes (the pack header counts into the pack size limit and each blob in the pack has an overhead of 32bytes in addition to the pack header entry). As you apparently did not encounter the pack-header size limit (at least I haven't read of any complaints ;-) ), I assume the average filesize to be above that limit. In pathological cases the header and data part of a pack file could have about the same size (aka. both are half a GB in size). And I doubt that the repository index handles such a large pack header well. So the pack-header size limit is just a safeguard that shouldn't apply during normal use.

Well, that use case seems rather different from what's been discussed in this PR so far (at least the throughput is an order of magnitude larger) and this also leads to new bottlenecks: With two just two file readers (= CPU cores) is definitely not possible to saturate a 10Gbps network link. The chunker implementation alone is currently capable of processing "just" 500-600 MB/s and with additional overhead from the file access and other processing steps the throughput will probably stay well below 1GB/s.

I propose it matches the linked forum discussion, and while I know there has been a lot of experimentation about what works great on direct attached storage, distributed storage is common now days and it does behave quite differently. I did have to increase the reader count, but I'd have to go digging in our ticket system to find the exactly numbers I ended up using. In the general case, 500-600MB/s was more than sufficient, the goal really being to use aggregate throughput to hide the latency of fetching a lot of small files. CPU is cheaper than my sanity when someone asks again why a data set isn't archived yet :)

The 16MB header size limit allows roughly 450k blobs per pack file (one pack header entry has 37 bytes). To fill up a 1GB pack file this would require an average filesize of over (1024−16)×1024^2/(16×1024^2/37)−32 = 2299 bytes (the pack header counts into the pack size limit and each blob in the pack has an overhead of 32bytes in addition to the pack header entry). As you apparently did not encounter the pack-header size limit (at least I haven't read of any complaints ;-) ),

I did read the discussion on that with some concern, and I think you're probably right, it was luck I didn't hit that limit. I did experiment with some much larger pack sizes and had some failures but did not dig deeply, it being a naive experiment. From a hazy memory the pack sizes I did test up to and failed using, would have lined up with what you calculate.

Just my 2c, I am using this patch for my backups and it works great.

My use case is storing files on Google Cloud Storage using the Archive storage class, which is very cheap, with storage priced at just $1.20/TB/month and free ingress bandwidth, but API requests are expensive at $0.50/10,000 operations. Restic is perfect for this use case and superior to borg, since its local cache means it normally only needs to write to the cloud and no subsequent reads are needed. However, with the default 5MB pack size, the cost of the API PUT requests for an initial backup add up to $10.00/TB. I'm using a 500MB pack size, which reduces the cost to $0.10/TB.

I second that it would be nice to be able to configure this once at the repository level, in addition to the existing command-line and environment variable additions.

Any status updates ?

Reworked the check. Doing some E2E tests on my workstations now. I think it's good, but I'd like to spend a bit of time letting it marinate on my machines. Either way, the check has now landed in my branch.

Now that #3484 and #3475 are merged, let's move forward with some parts of this PR.

--save-blob-concurrency will be obsoleted by #3611 so let's get rid of that option. --file-read-concurrency has to wait at least until #3489 is merged, which will probably (hopefully) makes that option obsolete for most use cases, so we'll have to reevaluate that option later on.

The remaining interesting part is --min-packsize. I'd like to separate the code to properly support larger pack sizes (including the prune parts) into a separate PR. That PR should also increase the hard-coded (!) pack size to something in the range between 10-20MB. The discussion whether we want to add the option to customize the pack size or not should be handled separately.

Sounds good, I'll rip the --min-packsize components out into a new PR, and sticking with the base2 convention, I'll set a 16MB default pack size for now.

New PR's in; #3731. I'm happy to close this one, or leave it open.

Rebased to only include backup --file-read-concurrency. --save-blob-concurrency was obsoleted by #3611. And --pack-size was merged as part of #3731.

I've noticed when using compression=max restic adds badly compressible files (tested with mp3s) quite slow to the repo. Without compression this is done in a few seconds (SSD source and target) compared to about 90 seconds in my testcase. That is of course somehow expected. However CPU usage is clearly below 50 % (12 Core CPU, 6 physical cores). As far as I understand restic currently backups two files in parallel. So if more files would be backed-up in parallel restic should be much faster in this case because it uses more of the available CPUs.

Or to say it in other words: With compression support this feature is even much more important and should be included in the next release.

Sorry to be such a party pooper, but --file-read-concurrency is pretty long, and the "file" part is kind of pretty pointless as it's already mentioned in the description and also somewhat given in the context of backing up files. What about we change it to just --read-concurrency? Or is there a potential need for another type of read concurrency setting in the future?

Or is there a potential need for another type of read concurrency setting in the future?

The only such flag I can think of right now would be reading multiple directories in parallel.

The only such flag I can think of right now would be reading multiple directories in parallel.

Wouldn't that be handled by reading multiple files in parallel? So if read-concurrency is 4 and there are 4 directories with one file each also all 4 directories would be read in parallel.

Wouldn't that be handled by reading multiple files in parallel? So if read-concurrency is 4 and there are 4 directories with one file each also all 4 directories would be read in parallel.

In that special case yes. Tough for backups containing several million of files, it would be possible to speed up the operation by listing multiple directory concurrently.

I've renamed the flag and rework the flag / environment variable handling to be more in line with the other commands.

The documentation for the flag now recommends it to improve performance for fast disks (see #3731 for measurements). The previous claim that reducing the read concurrency can improve throughput was never backed up with measurements as far as I see from scrolling through the comments.