Page MenuHomeFreeBSD

remove duplicate lz4 implementations
ClosedPublic

Authored by tsoome on Oct 15 2019, 9:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 16 2024, 5:39 AM
Unknown Object (File)
Feb 16 2024, 5:39 AM
Unknown Object (File)
Feb 16 2024, 5:39 AM
Unknown Object (File)
Feb 16 2024, 5:38 AM
Unknown Object (File)
Feb 16 2024, 4:23 AM
Unknown Object (File)
Jan 11 2024, 11:03 PM
Unknown Object (File)
Jan 5 2024, 3:40 AM
Unknown Object (File)
Dec 23 2023, 2:09 PM

Details

Summary

Port illumos change: https://www.illumos.org/issues/11667

Move lz4.c out of zfs tree to opensolaris/common/lz4, adjust it to be
usable from kernel/stand/userland builds, so we can use just one single
source. Add lz4.h to declare lz4_compress() and lz4_decompress().

Test Plan

build/install/boot using lz4 with zfs.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 27041
Build 25329: arc lint + arc unit

Event Timeline

I like the direction in general.

By the way, (unrelated to this change itself), have you considered updating lz4 code with the upstream ( https://github.com/lz4/lz4 )? I'm mostly concerned about getting more diverged from the upstream would make it harder for us to re-sync the code in the future, especially when we have already missed some of the optimization that happened upstream. There are also some other interesting changes upstream, for example, it used a set of macros to avoid downstream consumers to make changes like #ifdef _KERNEL in the code that would spread all over places, and that would make it easier for future upgrade too.

The change looks good to me as-is, but please consider these comments.

sys/cddl/boot/zfs/zfssubr.c
164

Note that the boot/ copy of lz4 was stripped down and #included to give the compiler to liberty to optimize away unused code. This practice is probably no longer relevant today (lz4 footprint is small anyways), but I'd suggest comparing the size before and after this change.

sys/cddl/contrib/opensolaris/common/lz4/lz4.h
2

Could you please consider licensing this file under the same license that LZ4 uses?

update the header for lz4.h, rebase on latest current.

I like the direction in general.

By the way, (unrelated to this change itself), have you considered updating lz4 code with the upstream ( https://github.com/lz4/lz4 )? I'm mostly concerned about getting more diverged from the upstream would make it harder for us to re-sync the code in the future, especially when we have already missed some of the optimization that happened upstream. There are also some other interesting changes upstream, for example, it used a set of macros to avoid downstream consumers to make changes like #ifdef _KERNEL in the code that would spread all over places, and that would make it easier for future upgrade too.

The change looks good to me as-is, but please consider these comments.

I do understand the temptation, but I rather keep the current code as is for this change. The lz4 update to more recent source will need a lot of testing and I do not have time to do that - not now at least. Here we are operating on the current source and we only do apply syntactic sugar (so to speak) to keep compilers happy.

tsoome added inline comments.
sys/cddl/boot/zfs/zfssubr.c
164

We do use function-sections and data sections with build and let ld to strip away the unused sections, the bios version is losing the compression bits this way. The UEFI version does not get clean - there we do build shared library, and I havent figured out exact steps how to reduce things like lz4_compress. I think we would need to get those symbols demoted to local via mapfile, then ld can strip them off.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 2 2019, 12:28 PM
This revision was automatically updated to reflect the committed changes.