Add lz4hc compression type

[vozhyk-]

Adds LZ4HC compression, enabled with a compression=lz4hc | lz4hc-{1..16} property.

Still doesn't add any feature flags and doesn't update the man-pages, as it hasn't been decided on the way it should be done. Currently 2 possibilities have been suggested:

• Adding a feature flag
• Using compression=lz4 and adding a dataset property to choose between lz4/lz4hc (lz4mode, compressionlevel, etc.)

How the code can be further cleaned up:

• By replacing lz4 code with the new upstream code, which would decrease the amount of duplicated code (lz4.c and lz4hc.c have many common bits, but lz4.c has them as macros while in lz4hc.c they are functions).
• By unifying lz4[hc]_compress_zfs functions, which contain almost the same code.

Issue #1900

[vozhyk-]

I've written a hack that allows to use LZ4HC while retaining full compatibility with existing implementations (lz4hc-hack branch).
It uses an lz4mode = lz4 | lz4hc | lz4hc-{1..16} property for choosing between LZ4/LZ4HC-*.
Better ways to do that might be:

• to make a generic way to write a different compression value for selected algorithms (e.g. to add another field to zio_compress_table)
• to use a different property than lz4mode (a property used only for one compression algorithm doesn't look good)

[ahrens]

Unfortunately, this is not the best way to do it (GZIP_# is also not great). I assume that (like gzip), any lz4hc_# can be decompressed by one algorithm. Adding all these values eats into the limited namespace of BP_GET_COMPRESS() (128 values). We should differentiate between the values that are set on the "compression" property (which could be lz4hc-2 or lz3hc-3, etc) and the values that are stored in the BP (where we would add at most one value).

[vozhyk-]

Doing this would be great, and would provide a generic way (instead of the hack that works only for LZ4HC) to store e.g. 'LZ4' instead of 'LZ4HC-n' value in the BP.
Is having many enum values like this still good if they are not stored in the BP, but used for the property values?

[ahrens]

@gerty3000 do you want to take a look at this, and/or counter-propose your hackathon results? (see also my existing comments)

[ahrens]

The lz4hc-hack branch seems correct, although as you said a little hacky. If it's valuable to have full backwards compatibility, I think something along those lines would be the way to go. An alternative that might be a cleaner implementation would be to only have readonly backwards compatibility. Then you could add the new "compression" property values, while not changing the values in the BP. Essentially you would replace the single "enum zio_compress" with 2 enums: one for values in the BP and one for values of the property. Then you just have to find the right place to translate from the prop val to the BP val. zio_write_bp_init (where you're essentially doing it now in the lz4hc-hack branch) seems reasonable.

[vozhyk-]

@ahrens
Thank you for the advice. I'm planning on doing it this way.
As for the feature flag (for the compression=lz4hc* property values), it can also be activated when the property is set to lz4hc* and deactivated when it's set to a different value.

[vozhyk-]

Updated; now the compression/decompression enums/tables are separate, and BP_COMPRESS_LZ4 is written for ZIO_COMPRESS_LZ4HC_* automatically.

--- BEGIN low-level code comments
I also thought of naming the "new" values ZIO_DECOMPRESS_* instead; both variants look bad in some parts of code (i.e. it's not readily apparent that BP_COMPRESS_* is used for decompression; or that you have to use BP_SET_COMPRESS(ZIO_UNCOMPRESS_*) (for the second variant)).

I also could try to retain a single zio_compress_table instead of splitting it in two, but it would result in either

• slower decompression function lookups (walking the table to find a matching decompression value) or
• forcing any new compression algorithm added after lz4hc to break the existing on-disk format (in case I append decompress_table + compress_table), or
• limiting the number of compression algorithms (in case the compression values start at some bigger offset of the table than BP_COMPRESS_VALUES).

ZIO_COMPRESS_LEGACY_FUNCTIONS hasn't been renamed, since it's for both compression and decompression.
--- END low-level code comments

There is one issue with the current first commit - it seems to me that L2ARC writes the compression value to disk, but I haven't found the place in code where it happens / this can be changed. It shouldn't currently be a problem, since BP_COMPRESS_LZ4 == ZIO_COMPRESS_LZ4, but it's still wrong.

Now only the feature flag needs to be added.

[ahrens]

@vozhyk- wrote:

it seems to me that L2ARC writes the compression value to disk, but I haven't found the place in code where it happens

That's because it doesn't happen :-) L2ARC stores the compression function in memory only (remember, L2ARC is not persistent). See l2arc_buf_hdr_t:b_compress.

[vozhyk-]

@ahrens
Good to hear :-) . Then I'll update the commit to make the changes more minimal (to get rid of the added L2ARC_IS_VALID_DECOMPRESS).

[vozhyk-]

My lz4hc branch now has the LZ4HC_COMPRESS feature flag, which is now
required for setting compression=lz4hc* and activated on the dataset when
the property is set (the latter untested due to my Thinkpad T61 refusing to
boot after a reboot to test this (likely due to the Nvidia card breaking)).
Some more work remains to be done - making zfs deactivate the feature when
compression is set to a different value, adding zio_compress_to_feature(),
and updating the documentation (and doing some of what the first post says).

[athei]

Any updates here since last year? I would really like to see it in 0.7.

[vozhyk-]

None, except for some in https://github.com/vozhyk-/zfs/commits/lz4hc . I'll try to find some time to finish this soon then.

[ahrens]

@dankimmel Care to review this? I think you implemented something similar at a hackathon.

[vozhyk-]

Do I understand correctly that the compression values in dnodes can only be <LZ4 (so when some data on a dataset has been compressed with lz4hc and compress=lz4 is set, the LZ4HC values don't exist anywhere on the dataset)? If so, deactivating the LZ4HC pool feature when compress is set to a different value seems suitable.

[vozhyk-]

Rebased on master, added an LZ4_COMPRESS feature flag and some documentation.

[vozhyk-]

Updated - added a dependency on extensible_dataset to the lz4hc_compress feature flag. Turns out that "per-dataset" feature flags need it.

[sempervictus]

@vozhyk- I'd like to add this PR to several physical hosts in our environment which are running ABD, and at first glance, it looks like the merge conflict is "non-trivial."
Is there a chance we could ask you to review the possibility of merging a branch with ABD (or even better, #4439)? Thank you

[vozhyk-]

@sempervictus I'll look at it when I get the time - probably in 2 weeks (but it can be in 1-2 more weeks). I don't know much about ABD, so if it really is non-trivial, I won't be able to do much right away. It'll be interesting to learn about it and adapt my changes though.

[sempervictus]

@vozhyk-: thank you much

[jumbi77]

@vozhyk Can i ask the status? Did you find some time working on it? What's about Dan Kimmel's prototype? Much thanks.

[vozhyk-]

@jumbi77 So far I haven't. I also haven't asked @dankimmel about his prototype yet.

These are the things that remain to be done:

• Rebasing on master
• Investigating/fixing the buildbot test failures (the zpool_get_002_pos test)
• Finishing the work on deactivating the feature flag when no datasets have compression=lz4hc* set (my first attempt at making this work has failed)
• Rebasing on ABD, if possible

[vozhyk-]

A small update - added the new feature flag to zpool_get test's config, so now it doesn't fail from seeing an unexpected pool property.

A few buildbots still fail, and I don't know the reasons for this. This doesn't seem related to this pull request though.

[behlendorf]

The next time you rebase can you also extend the existing compression test cases so they cover this.

[jumbi77]

lz4 is now available in version 1.7.3 (https://github.com/lz4/lz4/releases).
On which version is your work based on? Since r131 there are over 350+ commits.
Do you plan to "update" lz4hc and lz4 to the new version? (@vozhyk-)

EDIT:
Anyway thanks for your work. Would be great to be able to choose a higher compression algorithm than current lz4. But according to the Github NEWS page lz4hc is really slow on compression (see: updated table with lz4hc -9 v1.7.3). Do you confirm that with your tests? As comparison zstandard is much faster on compression throughput and ratio. Decompression is still acceptable I guess. I saw that @skiselkov implemented zstd at Hackathon 2016, right? Are you planning to pulish your code, Saso? 👍

[sempervictus]

@vozhyk-: with ABD merged in, could you please rebase the patch against current master? Thank you

[vozhyk-]

@sempervictus Rebased. It compiles, but I haven't tested it at all.

I also have to look into how it interacts with compressed ARC. It seems I haven't introduced any regressions in compressed ARC, but I still have to verify it.

I've done no changes to the code or the tests besides rebasing.

[dankimmel]

I can't think of any reason compressed ARC would break as a result of adding a new algorithm, however you may need to add a new ZFS send feature flag for this compression algorithm if you want to use compressed send / receive with data in this format.

[vozhyk-]

I can't think of any reason compressed ARC would break as a result of

adding a new algorithm Here I also make compression (the `compression` option value) and decompression (stored in BPs) values separate enums. This could be a problem if the compressed ARC changes made L2ARC start storing the compression algorithm in the BP instead of keeping it in the ARC header in memory. From what I've seen, this is not the case, so it shouldn't be a problem.

however you may need to add a new ZFS send feature flag for this

compression algorithm if you want to use compressed send / receive with data in this format. Thanks, I'll look into this. The data can be decompressed with plain LZ4. The only difference between a dataset compressed with LZ4 and one compressed with LZ4HC is the `compression` option value. The rest of the data on disk says it's in LZ4. On Thu, Mar 2, 2017 at 8:31 AM Dan Kimmel <notifications@github.com> wrote: I can't think of any reason compressed ARC would break as a result of adding a new algorithm, however you may need to add a new ZFS send feature flag for this compression algorithm if you want to use compressed send / receive with data in this format. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3908 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC6QvG9drLvNoHTUFtt-6p2BQRWWdmvgks5rhnBfgaJpZM4GMjVE> .

[ahrens]

@vozhyk- @dankimmel @grwilson I think that separating the BP compression enum (which specifies the decompression function) from the compression property enum (which specifies the compression function) breaks arc_cksum_is_equal(), because the ARC only knows the decompression function (from the BP), but it needs to regenerate the exact data that is stored on disk by recompressing it.

@vozhyk- At a minimum you should change the ARC code to reflect the fact that it is storing the enum bp_compress rather than the enum zio_compress.

[ahrens]

@vozhyk- The best solution I can come to for the problem (uncompressed ARC + L2ARC + separate compression / decompression enums) is to keep the checksum of the uncompressed data in memory when the data is in L2ARC. This is similar to how it used to be before compressed ARC, the downside is that it uses more memory to manage the L2ARC. When the data is compressed on disk, uncompressed in the ARC, and stored in the L2ARC, we allocate a zio_cksum_t and have a new field in the arc_buf_hdr_t point to it.

Sorry the design I suggested (of separate compression/decompression enums) turns out to have some complications. Are you interested in implementing the scheme I described above?

[behlendorf]

Sorry the design I suggested (of separate compression/decompression enums) turns out to have some complications. Are you interested in implementing the scheme I described above?

For the moment, I'm going to close this PR out as stale though finalizing this functionality would still be welcome! @vozhyk- if you find the time and interest to revisit this please go ahead and open a new PR.