-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
WIP: Adaptive compression [was: auto compression] #7560
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7560 +/- ##
==========================================
+ Coverage 77.41% 77.45% +0.04%
==========================================
Files 336 337 +1
Lines 107605 107725 +120
==========================================
+ Hits 83304 83440 +136
+ Misses 24301 24285 -16
Continue to review full report at Codecov.
|
I'll update this PR later today to bring the code in line with the style-check and check out the other tests. |
@ahrens I addressed your concerns raised about a incompability with Do you think that the allocation throttle still needs tweaking? |
@allanjude may be interested in this. |
cmd/dbufstat/dbufstat.py
Outdated
"ZIO_COMPRESS_GZIP_6", "ZIO_COMPRESS_GZIP_7", | ||
"ZIO_COMPRESS_GZIP_8", "ZIO_COMPRESS_GZIP_9", | ||
"ZIO_COMPRESS_ZLE", "ZIO_COMPRESS_LZ4", | ||
"ZIO_COMPRESS_ADAPTIVE", "ZIO_COMPRESS_FUNCTION"] |
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.
this looks like it's also missing LZ4
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.
It's there, on the right 😄
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.
@ahrens I don't see an issue here, the list just has been extended by one member. Have I overlooked something?
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 was mistaken, this looks right. I was thrown off by the two-column formatting.
@@ -3613,6 +3614,14 @@ vdev_stat_update(zio_t *zio, uint64_t psize) | |||
} | |||
|
|||
if (zio->io_delta && zio->io_delay) { | |||
int n = 1000; // average over 1000 zios |
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.
should this be a tunable?
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.
Well, do you think about a specific usecase where this might need tweaking?
I think 1000 I/Os is a nice value for averaging the delay introduced by compression to be still able to react on different loads on the system while still leveling out short spikes in latency.
But I haven't done any scientific approach to determine the optimal value for this. So this solely rely on guessing.
* CDDL HEADER END | ||
*/ | ||
|
||
#include <sys/compress_adaptive.h> |
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 appreciate seeing more comments in this file. Perhaps a big comment at the beginning explaining the theory behind this, and then some functions could also use comments describing their purpose. It looks like there are a bunch of concepts implicit in this file that could use explanation, e.g. "optimal delay", "faster/slower/optimal level".
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.
Thanks, I'll look into this. Sorry for the delay, I just have some weekdays to spare, currently.
A few thoughts (keeping in mind that I have read through the exchange rather quickly)
More in general:
|
@RJVB If nothing else, updating the embedded LZ4 implementation would probably run afoul of the same issues as updating zstd in #8044 - namely, that the checksums of blocks produced by the old version won't match what's produced by the new version, even though the decompressed data payloads are identical, which will (IIUC) require L2ARC changes to accommodate. (See here and the surrounding conversation for what I'm talking about.) |
namely, that the checksums of blocks produced by the old version won't match what's produced by the new version, even though the decompressed data payloads are identical
Hopefully not because the *compressed* data hasn't changed. At least, that's how I read Yann's answer here:
lz4/lz4#625 (comment)
If same raw data -> same compressed data, why would there be different checksums?
The unknown here is what effect the changes made to the LZ4 copy embedded in ZFS have had. Were it not for those I'd already have started tinkering (lz4 is 1 .c and 1.h file, after all).
|
@RJVB Tinker ahead! But let's move the discussion of updating lz4 to a different thread, as it's unrelated to this PR (except that maybe this PR would be less useful with better LZ4 in some cases). Could you open a new Issue to discuss updating lz4 code? |
leaving gzip-9 in would not be such a problem if we could decide the 'compressionbias' for each dataset/pool, in other words, do you prefer speed, or compressionratio?
Sometimes the one, sometimes the other, but I'm only prepared to trade in speed if compression really improves. The difference in compression between (g)zip-8 and (g)zip-9 is non-existent for (almost) all practical purposes, between levels 7 and 9 hardly better.
I see this with regular zip too, as used in HFS+ compression (which I use extensively on just about anything; there I use zip-8 because I can use an accelerated zip implementation).
Now, things are different if we're talking about an algorithm that tests different gzip levels, as long as the algorithm doesn't itself become too much slower for every level it tests.
|
@RJVB Tinker ahead!
I would if the current copy had less ZFS-specific changes and I understood them better...
(except that maybe this PR would be less useful with better LZ4 in some cases).
Exactly the reason why I mentioned it.
Could you open a new Issue to discuss updating lz4 code?
Do you think there's much point in doing that after the remarks here and on IRC, and that I open it given how little actual contribution I'll probably be able to make?
|
Yes. That, or the opposite (blacklisting compressors). Just use a common separator that doesn't require quoting in the shell and don't worry how it looks, it's not a beaty contest ;) |
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.
If anyone (or the original author) want to take this on, a not-complete list of todo's:
- Rebase
- Add comments where there are none
- Make this tuneable: WIP: Adaptive compression [was: auto compression] #7560 (comment)
- Add option to select the used compression algorithms: WIP: Adaptive compression [was: auto compression] #7560 (comment)
- Adding Adaptive compression to the current compression tests
- Add a test to test if adaptiveness is actually working
@BrainSlayer
This was what I was refering to in #7481
Once you get ZSTD merged this might actually have some merits in combination with #8941
Most of the work needed here is also just a rince and repeat of you work on ZSTD...
@BrainSlayer It might be interesting to look into the contrib included with ZSTD for adaptive compression: |
from the documentation it uses unused resources like current cpu load. but you have also take into account the much higher memory requirements for higher compression levels which cannot be ignored. lets say i decompress a tar file on a machine, the cpu is immediatly maxed out and the compression level will always stay low. so the approach here theoreticly works only for machine with rare file access. even if you just store a bigger file, the level will decrease after the first few blocks. so its questionable if there is any usefull case for that patch. and if we add zstd to the selection, gzip will never be used and can be removed from the list since zstd is faster in any way with much higher compression ratio at the higher levels. so rechecking this with zstd in a real life scenario at least makes sense |
@BrainSlayer Yeah, I agree its quite rare scenario. |
This Commit rebases and squashes openzfs#7560 Co-authored-by: Andy Lavr <andy.lavr@gmail.com> Co-authored-by: n1kl (bunge) <n1kl@users.noreply.github.com> Co-authored-by: RubenKelevra <ruben@vfn-nrw.de> Signed-off-by: n1kl (bunge) <n1kl@users.noreply.github.com> Signed-off-by: RubenKelevra <ruben@vfn-nrw.de> Signed-off-by: Andy Lavr <andy.lavr@gmail.com> Signed-off-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
@Ornias1993 @RubenKelevra there is a work in progress branch "adaptive" in my github tree. which resolves all conflicts. still working to check if all is working and usually need to fix api issues |
Great, thanks a lot! Was extremely busy, so haven't been able to look into it anymore. I'm sorry guys. Great to see this is going still forwards! |
Well, hope you can test this theory. The adaptive approach steals actually CPU time from other processes, since it's running in the kernel and also with a higher priority. But it will only steal from other processes, if the storage media can't accept the data fast enough. This could be due to other operations accessing the storage media or if the media is just slower than the CPU can compress. The idea is more about reducing the total I/O time, by trying to compress harder if the storage media is hitting a wall, to reduce the total data needed to be written - not to adapt to different CPU-loads. While obviously a higher CPU load might increase the compression time and thus reducing the chosen level for compression - due to lower boost levels in the CPU or blocked parts of the CPU by a concurrent thread on a different virtual CPU. Hope this makes sense :) |
@BrainSlayer I already got it building and passing most tests a few days ago, I think you could beter use that to continue from here on: I also don't get your comment ago, while I finished the rebase a few days ago?! :S There is only one test failure as I know of currently and I also added adaptive to the compression test suite. I also documented the failure thats fucking with FreeBSD, it's all already in #11002 ;) edit I added comments to some of the area's of your commit that are not going to work: |
@Ornias1993 not yet. still fixing api changes. just merged the original patches. the rest is in progress in background. in 60 minutes it should be done |
@BrainSlayer Why do the work double, I already fixed them all days ago, as pointed out. So:
Should be:
Dude, you are serieusly manually duplicating the work I already did, because you like to do extra needless affort or something? This isn't a school thing where you get extra credit for doing needless extra work, might I remind you of that? ;) (Not to be harsh, I totally don't get what the freak you are doing if everything is already done for you days ago and you could just take it and continue with the actually interesting stuff.) #11002 provide a clean start for you to work on, with all code attibution fixed for you (which isn't actually done right here either to be honest, signoffs etc are wrong) and everything rebased with tests ready to rock. |
Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
@Ornias1993 i did it because you asked for. my tree compiles now. but for zstd support it has to be reworked and if you did it too already. dont take it as competition. i just did it, because i felt so. i did not see that you already made the job. but you may compare your work with mine now. maybe its usefull |
@BrainSlayer uhmm, no I didn't ask you to rebase this... I said I was rebasing this and asked for you to check it. I literally (like literally-literally) asked you to take a look at my work in #11002
It isn't about competition, it's about not doing work twice ;)
Mistakes are only human 👍 Anyway:
Please read my PR, i'm not going to carry this. I only intended to rebase it and add some required tests. I'm not qualified to continue it any further than the rebase. So if you want to continue this, please use my work and crossreference accordingly. I'm not taking this PR, i've rebased it and added tests and thats all i'm going to do with iut.
Yes I cheated it to get it building, but it indeed has to be reworked. To be honest I'm debating if this should just be refactored into: edit |
i just set them static since declarations where missing. so i avoid compiler warnings. thats all. the level thing is still just working for gzip only. zstd handles them very different. this why i added a todo for it. i will look into it deeper and will switch to your code to see what i can enhance. but i'm still skeptical for the whole concept |
- This Commit rebases and squashes openzfs#7560 - Adds tests for Adaptive compression Co-authored-by: n1kl (bunge) <n1kl@users.noreply.github.com> Co-authored-by: Ruben Kelevra <RubenKelevra@users.noreply.github.com> Co-authored-by: Andy Lavr <andy.lavr@gmail.com> Co-authored-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl> Signed-off-by: n1kl (bunge) <n1kl@users.noreply.github.com> Signed-off-by: RubenKelevra <ruben@vfn-nrw.de> Signed-off-by: Andy Lavr <andy.lavr@gmail.com> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Signed-off-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
- This Commit rebases and squashes openzfs#7560 - Adds tests for Adaptive compression Co-authored-by: n1kl (bunge) <n1kl@users.noreply.github.com> Co-authored-by: Ruben Kelevra <RubenKelevra@users.noreply.github.com> Co-authored-by: Andy Lavr <andy.lavr@gmail.com> Co-authored-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl> Signed-off-by: n1kl (bunge) <n1kl@users.noreply.github.com> Signed-off-by: RubenKelevra <ruben@vfn-nrw.de> Signed-off-by: Andy Lavr <andy.lavr@gmail.com> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Signed-off-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
This fixes some of the rebase mistakes on the documentation It also adds some commenting to compress_adaptive Co-authored-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Signed-off-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl> (+1 squashed commits) Squashed commits: [8de8850] Add adaptive compression - This Commit rebases and squashes openzfs#7560 - Adds tests for Adaptive compression Co-authored-by: n1kl (bunge) <n1kl@users.noreply.github.com> Co-authored-by: Ruben Kelevra <RubenKelevra@users.noreply.github.com> Co-authored-by: Andy Lavr <andy.lavr@gmail.com> Co-authored-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl> Signed-off-by: n1kl (bunge) <n1kl@users.noreply.github.com> Signed-off-by: RubenKelevra <ruben@vfn-nrw.de> Signed-off-by: Andy Lavr <andy.lavr@gmail.com> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Signed-off-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
- This Commit rebases and squashes openzfs#7560 - Add tests for Adaptive compression - Add some commenting to compress_adaptive Co-authored-by: n1kl (bunge) <n1kl@users.noreply.github.com> Co-authored-by: Ruben Kelevra <RubenKelevra@users.noreply.github.com> Co-authored-by: Andy Lavr <andy.lavr@gmail.com> Co-authored-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl> Signed-off-by: n1kl (bunge) <n1kl@users.noreply.github.com> Signed-off-by: RubenKelevra <ruben@vfn-nrw.de> Signed-off-by: Andy Lavr <andy.lavr@gmail.com> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Signed-off-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
- This Commit rebases and squashes openzfs#7560 - Add tests for Adaptive compression - Add some commenting to compress_adaptive Co-authored-by: n1kl (bunge) <n1kl@users.noreply.github.com> Co-authored-by: Ruben Kelevra <RubenKelevra@users.noreply.github.com> Co-authored-by: Andy Lavr <andy.lavr@gmail.com> Co-authored-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl> Signed-off-by: n1kl (bunge) <n1kl@users.noreply.github.com> Signed-off-by: RubenKelevra <ruben@vfn-nrw.de> Signed-off-by: Andy Lavr <andy.lavr@gmail.com> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Signed-off-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
5e18cd1
to
40daae6
Compare
- This Commit rebases and squashes openzfs#7560 - Add tests for Adaptive compression - Add some commenting to compress_adaptive Co-authored-by: n1kl (bunge) <n1kl@users.noreply.github.com> Co-authored-by: RubenKelevra <cyrond@gmail.com> Co-authored-by: Andy Lavr <andy.lavr@gmail.com> Co-authored-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl> Signed-off-by: n1kl (bunge) <n1kl@users.noreply.github.com> Signed-off-by: RubenKelevra <cyrond@gmail.com> Signed-off-by: Andy Lavr <andy.lavr@gmail.com> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Signed-off-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
2b10d55
to
a0b72a9
Compare
Don't forget your own copyright lines too ;) |
The following files need my and @BrainSlayer copyright header: The following need only brainslayers copyright header: I think thats all that is copyrightable in the rebase... |
- This Commit rebases and squashes the original work done on openzfs#7560 Additionally to the original patch from openzfs#7560: - Add tests for Adaptive compression - Add some commenting to compress_adaptive - Small fixes - Copyright headers Co-authored-by: n1kl (bunge) <n1kl@users.noreply.github.com> Co-authored-by: RubenKelevra <cyrond@gmail.com> Co-authored-by: Andy Lavr <andy.lavr@gmail.com> Co-authored-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl> Signed-off-by: n1kl (bunge) <n1kl@users.noreply.github.com> Signed-off-by: RubenKelevra <cyrond@gmail.com> Signed-off-by: Andy Lavr <andy.lavr@gmail.com> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Signed-off-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl> Co-Authored-By: n1kl (bunge) <n1kl@users.noreply.github.com> Co-Authored-By: RubenKelevra <cyrond@gmail.com> Co-Authored-By: Andy Lavr <andy.lavr@gmail.com> Co-Authored-By: Sebastian Gottschall <s.gottschall@dd-wrt.com> Co-Authored-By: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
This Commit rebases and squashes the original work done on openzfs#7560 Additionally to the original patch from openzfs#7560: - Add tests for Adaptive compression - Add some commenting to compress_adaptive - Small fixes - Copyright headers Co-authored-by: n1kl (bunge) <n1kl@users.noreply.github.com> Co-authored-by: RubenKelevra <cyrond@gmail.com> Co-authored-by: Andy Lavr <andy.lavr@gmail.com> Co-authored-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl> Signed-off-by: n1kl (bunge) <n1kl@users.noreply.github.com> Signed-off-by: RubenKelevra <cyrond@gmail.com> Signed-off-by: Andy Lavr <andy.lavr@gmail.com> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Signed-off-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl> Co-Authored-By: n1kl (bunge) <n1kl@users.noreply.github.com> Co-Authored-By: RubenKelevra <cyrond@gmail.com> Co-Authored-By: Andy Lavr <andy.lavr@gmail.com> Co-Authored-By: Sebastian Gottschall <s.gottschall@dd-wrt.com> Co-Authored-By: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
8fc2f3f
to
54fc6d4
Compare
Co-Authored-By: n1kl (bunge) <n1kl@users.noreply.github.com> Co-Authored-By: RubenKelevra <cyrond@gmail.com> Co-Authored-By: Andy Lavr <andy.lavr@gmail.com> Co-Authored-By: Sebastian Gottschall <s.gottschall@dd-wrt.com> Co-Authored-By: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
@RubenKelevra It looks like you made some progress on this last year. Is this now complete and ready for a thorough code review? |
This is a successor of the auto compression PR (#5928). This PR stale without substantial criticism, so I decided to pick up the work.
Description
I did some performance measurements and it wasn't meeting my expectations. So I tweaked the algoritm and excluded the off-compression as an option since the algorithm actually isn't able to determine the additional latency resulting of the larger data size when no compression is applied. I added gzip-2 to gzip-9 as options for the algorithm to choose from.
The algorithm should adapt to different CPU load situations, since it's measuring the latency introduces over the last 1000 compression cycles (one cycle one block). If the load of the system change over time, it might choose different compression algorithms.
In the light of zstd, the adaptive compression keyword might be a good choice for an adaptive zstd mode in the future selecting different zstd compression rates and relying on the same mechanism to select those.
Motivation and Context
For the initial motivations, head over to the original PR #5928 .
How Has This Been Tested?
I ran a simple benchmark on a single HDD with different scenarios:
My corpus is /usr/lib from my system (5.9 G with 117,920 files in 17,777 folders), copied with
cp -ax
from an SSD to an HDD.All ZFS settings was set to default, except for checksum, which was
edonr
.System specs:
I understand that this test results might not be valid for a typical server application, but it should be a good measurement for an average notebook user. A use case for ZFS where latency and thruput is important too.
The workload scenario was a synthetic CPU/memory bound only user space program, with one thread per logical CPU core. The programm used for this was BOINC, with seti@home work units.
The load of the system was measured 75 seconds into the copy (on runs which was completed in less than 75 s the load value is somewhat inaccurate). Overall this value isn't really a hard prove, that one test result is better than a different one. I just wanted to show that the load of the system doesn't skyrocket, when using adaptive instead of lz4 or a gzip level.
In the original PR the author explained that dva_throttle might interfere with this adaptive compress algorithm selection. And I can confirm this, it might result in slightly less performance in compression ratio, but I could not find a distinctive drop in I/O performance which would hinder an inclusion into the master. Furthermore, with all compression algorithms, the performance impact was mixed with and without dva_throttle.
Overall those performance numbers for adaptive compression often look pretty good. I wasn't expecting a performance better than plain LZ4 compression, but it performed better in some scenarios.
I also like to point out that I used the filesystems without any parameters natively on the zvols. In my test the physical sector size is set by zfs to the recordsize, so the filesystems are aware of this (ext4 at least) and might use some (automatic) optimizations for those large physical sector sizes. This might lead to different results than in VMs, where the physical block size is usually 512 or 4096 for the filesystems inside the VM.
adaptive compression stats.pdf
adaptive compression stats zvol.pdf
Types of changes
Checklist:
Signed-off-by
.