Add zfs zstd support to loader

[mmacy]

stand/libsa/zfs/Makefile.inc

28

Not my proudest hour.

[tsoome]

stand/libsa/zfs/zstd_shim.c

13

Can we use malloc() and free() there? That would allow us to use memory management as defined in stand.h, and we can pick userland malloc for extra debugging features (in userboot).

[imp]

stand/libsa/zfs/Makefile.inc

17

_KERNEL, KERNEL and KERNEL__? Does OpenZFS base use all of those for real?

18

KLD_MODULE? Why?

28

Yea, this has to be done in a different way. This isn't acceptable. A simple include file would do that trick....

stand/libsa/zfs/zstd_shim.c

22

extra white space

sys/cddl/boot/zfs/zfssubr.c

113

extra white space

sys/contrib/openzfs/include/os/freebsd/spl/sys/cmn_err.h

55

You can peel this off and commit this file separately to reduce the size of this diff.

[mmacy]

stand/libsa/zfs/Makefile.inc

28

Yea, this has to be done in a different way. This isn't acceptable. A simple include file would do that trick....

Uh no it wouldn't. I didn't spend 8 hours beating my head against this because it was easy. I assume you're offering to write a patch to this effect since it's "simple" - thanks.

[mmacy]

stand/libsa/zfs/Makefile.inc

17

_KERNEL, KERNEL and KERNEL__? Does OpenZFS base use all of those for real?

mmacy@anarchy [~/devel/ZoF|11:17|12] find . -name "*.[ch]" | xargs grep _KERNEL | grep -v __KERNEL | wc

535    1584   29073

mmacy@anarchy [~/devel/ZoF|11:17|13] find . -name "*.[ch]" | xargs grep KERNEL | wc

3       9     203

It looks like I can live without the _KERNEL

18

KLD_MODULE? Why?

To avoid the need for offset.inc when including systm.h

stand/libsa/zfs/zstd_shim.c

13

Can we use malloc() and free() there? That would allow us to use memory management as defined in stand.h, and we can pick userland malloc for extra debugging features (in userboot).

Sure. I didn't see those in the sources.

[mmacy]

• rebase
• remove __KERNEL

[mmacy]

stand/libsa/zfs/zstd_shim.c

13

Can we use malloc() and free() there? That would allow us to use memory management as defined in stand.h, and we can pick userland malloc for extra debugging features (in userboot).

No. We can use free(). But malloc is just a define for Malloc in stand.h which I can't include.

[mmacy]

• whitespace

[imp]

stand/libsa/zfs/zstd_shim.c

13

I hit the same issue...

[mmacy]

rebase

[imp]

Much of the problematic elements of this patch can be fixed by using the proper facilities, and/or extending a couple.
I've taken the liberty of doing this work for you and posting the review to https://reviews.freebsd.org/D26218

Briefly, it adds the CFLAGS_EARLY to allow proper stacking of include paths.
It tweaks a few places to use _STANDALONE in addition to KERNEL (While leaving out a large number of them), though many of the sys/sys/*.h ones might need a little more refinement still.
It tries very hard to minimize the defines as well. Once you eliminate -DKERNEL, most of the need goes away anyway leaving only a few places to tweak (another pass over what I've done to see if they are all really needed might allow the changes to be shrunk too).

We can take up discussing my patches there, or you can revise this one.

stand/libsa/zfs/Makefile.inc

17

I should have communicated that no use of -DKERNEL is permitted in the boot loader. You're not allowed to compile with that defined at all. You need to condition things on -D_STANDALONE. It's a third environment to compile in. There are big issues defining KERNEL elsewhere in the boot loader (which is why the practice was banned and all other issues corrected). The number of places needing ifdef _STANDALONE is small here.

18

OK. It too should be under -D_STANDALONE. Although it turns out not to be needed if you don't define KERNEL and make a small tweak to systm.h's confused use of KERNEL.

28

The long list of includes isn't permitted. It's unmaintainable.

Instead, you need to create a facility in defs.mk to allow you to create the proper stack of include files. It's one line there and lets you move these all to #include inside of zstd_shim.c. Most of the defines to 'turn off' include files go away too. I'll add that so there's a regular way to have early args in CFLAGS on a per-target basis.

If you do that, you'll be able to #include these files in zstd_shim.c and have them work as intended.

With these changes, list.c needs no special treatment other than the early include.

nvlist.c runs into the bug in the zfs code where two different boolean_t definitions exist. This should be resolved separately, however. Just adding the freebsd spl directory to CFLAGS is enough to get by, though should be revisited. zfs.c has the same issue. It also doesn't need lz4 to compile.

stand/libsa/zfs/zstd_shim.c

13

We should include stand.h here, or sort out the malloc stuff to its own file that stand.h or this includes. But that need not be done here.

Actually, a lot needs to change here. It's in the review I've listed below.

sys/contrib/openzfs/include/os/freebsd/spl/sys/taskq.h

33

This isn't needed and can be omitted entirely.

sys/contrib/openzfs/module/os/freebsd/spl/list.c

33

This change shouldn't be needed, but this winds up including libkern.h which uses KASSERT and that fails... it's also a good chance, though, for other reasons: it includes less stuff.

[grahamperrin]

D26218 was closed with a commit, does there remain a need to review this D26207?