Page MenuHomeFreeBSD

Integrate zstd into boot loader
ClosedPublic

Authored by imp on Aug 28 2020, 11:46 AM.

Details

Summary

Integrate zstd support into the boot loader, from earlier work by Matt Macey.

Test Plan

Make sure the last minutes shuffling I did didn't break the kernel and/or userland
builds.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

imp requested review of this revision.Aug 28 2020, 11:46 AM
imp retitled this revision from Make this compile to Integrate zstd into boot loader.Aug 28 2020, 12:00 PM

Last second tweaks that I missed the need for before a full build.

this isn't needed, remove it.

this isn't needed either...

Ah, just use the standard definition of offsetof.

include/stddef.h
75 ↗(On Diff #76302)

Hmmm, this one might not be right... I'll have to look later today...

stand/libsa/zfs/zstd_shim.c
36 ↗(On Diff #76303)

If we get the other issues with boolean_t resolved, this can be eliminated.

Generally happy about the cleanup and the added flexibility in the build infrastructure.

Changes to the ifdefs/if defined could use a pass to check for consistency of the comments on the closing endif.

sys/contrib/openzfs/module/os/freebsd/spl/list.c
30 ↗(On Diff #76306)

This is normally included on the command line. I don't think we should start being inconsistent about that here.

sys/contrib/openzfs/module/zstd/zfs_zstd.c
46 ↗(On Diff #76306)

Is this order important or can the zio_compress.h include just be pulled out of the if and avoid the else?

84 ↗(On Diff #76306)

This seems like a weird place to do this.

sys/contrib/openzfs/module/os/freebsd/spl/list.c
30 ↗(On Diff #76306)

It's quite unusual wrt rest of FreeBSD and the boot loader to do this for anything but the global #define options in the kernel...

sys/contrib/openzfs/module/zstd/zfs_zstd.c
46 ↗(On Diff #76306)

Unsure. I'll try it.

84 ↗(On Diff #76306)

Yea, I didn't want to get into the tangled mess of where it's defined, but that may be a fair bit of feedback.

imp added reviewers: tsoome, mmacy.
sys/contrib/openzfs/lib/libspl/include/os/freebsd/sys/param.h
48 ↗(On Diff #77553)

This change from ifndef to ifdef is concerning. What's going on here?

sys/contrib/openzfs/lib/libspl/include/os/freebsd/sys/param.h
48 ↗(On Diff #77553)

That's likely a mistake. I'll bet it can be removed entirely.

imp edited the summary of this revision. (Show Details)

Update, per reviews from the PR

Rebase and rework after comments from the OpenZFS upstreaming
to reduce diffs in upstream files. I was able to flesh out
the _STANDALONE support in the spl for FreeBSD instead of
some surgery in upstream files.

update atomic.h to fully neutered for STANDALONE version, save for
atomic_add_uint64

rebase, and some last second refinement.

Once you can get ZTS to pass it LGTM.

stand/libsa/zfs/Makefile.inc
18 ↗(On Diff #78139)

Clever.

stand/libsa/zfs/zfsimpl.c
134 ↗(On Diff #78139)

Remove this one if you're going to move it up.

update with matt's suggestion

One more nit before upstreaming. Down to 2 defines in the shim.

stand/libsa/zfs/Makefile.inc
18 ↗(On Diff #78139)

thanks.

stand/libsa/zfs/zfsimpl.c
134 ↗(On Diff #78139)

nah, it was added independently of me, so I'll just delete the one I'm adding.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 12 2020, 10:19 PM
This revision was automatically updated to reflect the committed changes.