Enable edonr in FreeBSD #12735

rincebrain · 2021-11-09T07:15:32Z

Motivation and Context

There's no reason to not enable it, as far as I know, so let's drop a platform difference.

Description

Dropped a lot of ifdefs, included edonr.c as a thing to stuff into openzfs.ko, removed the if linux around things in tests/

How Has This Been Tested?

$ sudo zfs get checksum mypool
NAME    PROPERTY  VALUE      SOURCE
mypool  checksum  edonr      local
$ uname -a
FreeBSD freebsd12box 12.2-RELEASE-p7 FreeBSD 12.2-RELEASE-p7 GENERIC  amd64
$

A round of enabling the feature, reading, writing, and reading with zdb to confirm it actually used it, went fine...

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

rincebrain · 2021-11-09T10:39:54Z

Huh, all of the {12,13,14} testbots die at the same spot with no logs.

Unfortunately, I nuked most of my FBSD VMs (which I foolishly had ZFS root on) in incomplete prior iterations of this patch, so I'll have to restore those from backup or reinstall them with non-ZFS root first...

(Nothing that exciting, incidentally - just unresolved symbols that I didn't find out about until load time, and load time was, on those systems, boot time...)

The code is integrated, builds fine, runs fine, there's not really any reason not to. Signed-off-by: Rich Ercolani <[email protected]>

rincebrain · 2021-11-09T11:37:05Z

Ah, heh, whoops.

I had the remnants of a prior build's objects laying around and for some reason it didn't rebuild them as expected, so when I thought I was testing the latest rev of the code, it was, in fact, not. (No ccache anywhere on the system, so I'm not really sure what happened.)

But on a clean tree, it very obviously was extremely on fire with a change I made later. Should be resolved.

Wow. That was quite the failure mode.

ghost · 2021-11-09T18:42:29Z

This might have complications when building in the base system, so we'll want to check that as well.

rincebrain · 2021-11-09T18:44:18Z

This might have complications when building in the base system, so we'll want to check that as well.

Which kinds of complications? Are you thinking of what might transpire if you're running new userland and old kernel bits, or something else?

ghost · 2021-11-09T19:34:21Z

I don't know why, but we haven't used any of the ICP code in FreeBSD so far. There were a lot of gymnastics making sure things build in the base system. I just want to be sure we don't break anything like the boot loader or architectures we haven't set up CI in OpenZFS for but are still supported by the base system. It's definitely promising that it builds and passes the tests here.

rincebrain · 2021-11-09T19:38:04Z

I don't know why, but we haven't used any of the ICP code in FreeBSD so far. There were a lot of gymnastics making sure things build in the base system. I just want to be sure we don't break anything like the boot loader or architectures we haven't set up CI in OpenZFS for but are still supported by the base system.

When I asked, I was told that it was just that nobody had wired it up, not any expectation of underlying issues.

Does FBSD have some CI that could be leveraged to run this through its paces?

ghost · 2021-11-09T20:30:41Z

I don't think there's much in the way of public CI for FreeBSD, I'm only aware of a periodic CI that runs for things that have already been committed. If you're inclined to test this yourself, you'd run make universe or make tinderbox in the FreeBSD source tree after merging the changes and updating the FreeBSD build system, just to make sure it builds. Otherwise I can do that, probably tomorrow at the soonest.

bghira · 2021-11-09T21:21:29Z

i think Rich was asking specifically about CI for non-common arch

rincebrain · 2021-11-09T21:23:50Z

I meant it more for uncommon arch, but more generally, since the statement sounded to me like "I do not think the OpenZFS CI is enough testing for this", I wanted to know if there was a set of testing this could be fed into to see if it was up to snuff.

I can probably run make universe, I do have a couple of x86 cores available. (Though I'll probably try buildkernel/buildworld first and run down issues with that...otherwise, I'd be happy to let you run it if you've got many more cores.)

As far as other arches, though, all my hardware for those is at least slow and possibly emulated and slow.

bghira · 2021-11-09T21:28:31Z

oh there's the freebsd-arch mailing list https://lists.freebsd.org/subscription/freebsd-arch

not sure how active it is anymore. but you can find some people who may be able to help test these changes especially if it's going to impact their ability to boot sometime in the future.

ghost

Tinderbox completed the builds for all the oddball architectures, so potential run time issues notwithstanding, this seems ok.

@mmatuska I applied this change to add the files to the FreeBSD build, FYI:

diff --git a/sys/modules/zfs/Makefile b/sys/modules/zfs/Makefile
index 983f0aa0e994..54d53c87a7c5 100644
--- a/sys/modules/zfs/Makefile
+++ b/sys/modules/zfs/Makefile
@@ -8,6 +8,7 @@ KMOD=	zfs
 .PATH:	${SRCDIR}/avl \
 	${SRCDIR}/lua \
 	${SRCDIR}/nvpair \
+	${SRCDIR}/icp/algs/edonr \
 	${SRCDIR}/os/freebsd/spl \
 	${SRCDIR}/os/freebsd/zfs \
 	${SRCDIR}/unicode \
@@ -44,6 +45,9 @@ SRCS=	vnode_if.h device_if.h bus_if.h
 # avl
 SRCS+=	avl.c
 
+# icp
+SRCS+=	edonr.c
+
 #lua
 SRCS+=	lapi.c \
 	lauxlib.c \
@@ -191,6 +195,7 @@ SRCS+=	abd.c \
 	dsl_scan.c \
 	dsl_synctask.c \
 	dsl_userhold.c \
+	edonr_zfs.c \
 	fm.c \
 	gzip.c \
 	lzjb.c \
@@ -313,6 +318,7 @@ CFLAGS.dmu_traverse.c= -Wno-cast-qual
 CFLAGS.dsl_dir.c= -Wno-cast-qual
 CFLAGS.dsl_deadlist.c= -Wno-cast-qual
 CFLAGS.dsl_prop.c= -Wno-cast-qual
+CFLAGS.edonr.c= -Wno-cast-qual
 CFLAGS.fm.c= -Wno-cast-qual
 CFLAGS.lz4.c= -Wno-cast-qual
 CFLAGS.spa.c= -Wno-cast-qual

ghost · 2021-11-11T19:41:39Z

Actually the LINT builds did fail because of this:

ld: error: undefined symbol: abd_checksum_edonr_native
>>> referenced by zio_checksum.c
>>>               zio_checksum.o:(zio_checksum_table)

ld: error: undefined symbol: abd_checksum_edonr_byteswap
>>> referenced by zio_checksum.c
>>>               zio_checksum.o:(zio_checksum_table)

ld: error: undefined symbol: abd_checksum_edonr_tmpl_init
>>> referenced by zio_checksum.c
>>>               zio_checksum.o:(zio_checksum_table)

ld: error: undefined symbol: abd_checksum_edonr_tmpl_free
>>> referenced by zio_checksum.c
>>>               zio_checksum.o:(zio_checksum_table)
*** [kernel] Error code 1

I'm not sure why only the LINT builds, maybe my integration of the edonr files into the FreeBSD build system is incomplete?

ghost · 2021-11-11T20:10:19Z

diff --git a/sys/conf/files b/sys/conf/files
index e1ee0e9fc9ef..712824bd637d 100644
--- a/sys/conf/files
+++ b/sys/conf/files
@@ -231,6 +231,7 @@ contrib/openzfs/module/unicode/uconv.c		optional zfs compile-with "${ZFS_C}"
 contrib/openzfs/module/unicode/u8_textprep.c		optional zfs compile-with "${ZFS_C}"
 
 #zfs checksums / zcommon
+contrib/openzfs/module/icp/algs/edonr/edonr.c		optional zfs compile-with "${ZFS_C}"
 contrib/openzfs/module/zcommon/cityhash.c		optional zfs compile-with "${ZFS_C}"
 contrib/openzfs/module/zcommon/zfeature_common.c		optional zfs compile-with "${ZFS_C}"
 contrib/openzfs/module/zcommon/zfs_comutil.c		optional zfs compile-with "${ZFS_C}"
@@ -283,6 +284,7 @@ contrib/openzfs/module/zfs/dsl_prop.c		optional zfs compile-with "${ZFS_C}"
 contrib/openzfs/module/zfs/dsl_scan.c		optional zfs compile-with "${ZFS_C}"
 contrib/openzfs/module/zfs/dsl_synctask.c		optional zfs compile-with "${ZFS_C}"
 contrib/openzfs/module/zfs/dsl_userhold.c		optional zfs compile-with "${ZFS_C}"
+contrib/openzfs/module/zfs/edonr_zfs.c		optional zfs compile-with "${ZFS_C}"
 contrib/openzfs/module/zfs/fm.c		optional zfs compile-with "${ZFS_C}"
 contrib/openzfs/module/zfs/gzip.c		optional zfs compile-with "${ZFS_C}"
 contrib/openzfs/module/zfs/lzjb.c		optional zfs compile-with "${ZFS_C}"

This might fix the LINT issues. Running tinderbox again now...

ghost · 2021-11-11T23:12:26Z

All good now.

behlendorf

It's nice to see this platform discrepancy removed. This makes sense to me as long as we can confirm with the FreeBSD folks they are fine with relying on the icp for this functionality (it sounds like that's the case). One nice upside is we'll no longer need different linux and freebsd zfs/cmd/zpool/compatibility.d files.

allanjude

Reviewed By: Allan Jude <[email protected]>

resolves: link_elf_obj: symbol abd_checksum_edonr_native undefined The required module-build bits were originally identified in the upstream pull request: openzfs/zfs#12735 But were missed when the code was imported (since they are not committed upstream). X-MFC-With: dae1713, 09cd634 Submitted by: freqlabs Sponsored by: Klara Inc.

This chases 269b5da (openzfs#12735), which touched the actual code but didn't fix the comment Signed-off-by: Ahelenia Ziemiańska <[email protected]>

This chases 269b5da (#12735), which touched the actual code but didn't fix the comment Additionally, ignore the name. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Rich Ercolani <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes #12823

resolves: link_elf_obj: symbol abd_checksum_edonr_native undefined The required module-build bits were originally identified in the upstream pull request: openzfs/zfs#12735 But were missed when the code was imported (since they are not committed upstream). X-MFC-With: dae1713, 09cd634 Submitted by: freqlabs Sponsored by: Klara Inc.

The code is integrated, builds fine, runs fine, there's not really any reason not to. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Allan Jude <[email protected]> Reviewed-by: Tony Nguyen <[email protected]> Signed-off-by: Rich Ercolani <[email protected]> Closes openzfs#12735

This chases 269b5da (openzfs#12735), which touched the actual code but didn't fix the comment Additionally, ignore the name. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Rich Ercolani <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#12823

The code is integrated, builds fine, runs fine, there's not really any reason not to. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Allan Jude <[email protected]> Reviewed-by: Tony Nguyen <[email protected]> Signed-off-by: Rich Ercolani <[email protected]> Closes openzfs#12735

This chases 269b5da (openzfs#12735), which touched the actual code but didn't fix the comment Additionally, ignore the name. Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Rich Ercolani <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#12823

Enable edonr in FreeBSD

15ed7b8

The code is integrated, builds fine, runs fine, there's not really any reason not to. Signed-off-by: Rich Ercolani <[email protected]>

rincebrain force-pushed the oops_edonr branch from c988e89 to 15ed7b8 Compare November 9, 2021 11:35

ahrens assigned tonynguien Nov 10, 2021

ghost approved these changes Nov 11, 2021

View reviewed changes

behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 11, 2021

behlendorf approved these changes Nov 12, 2021

View reviewed changes

allanjude approved these changes Nov 14, 2021

View reviewed changes

tonynguien approved these changes Nov 15, 2021

View reviewed changes

tonynguien added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 15, 2021

tonynguien merged commit 269b5da into openzfs:master Nov 16, 2021

nabijaczleweli mentioned this pull request Dec 5, 2021

zfeature_common#zfs_mod_supported_feature(): FreeBSD supports edonr #12823

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable edonr in FreeBSD #12735

Enable edonr in FreeBSD #12735

rincebrain commented Nov 9, 2021

rincebrain commented Nov 9, 2021 •

edited

Loading

rincebrain commented Nov 9, 2021 •

edited

Loading

ghost commented Nov 9, 2021

rincebrain commented Nov 9, 2021

ghost commented Nov 9, 2021 •

edited by ghost

Loading

rincebrain commented Nov 9, 2021

ghost commented Nov 9, 2021

bghira commented Nov 9, 2021

rincebrain commented Nov 9, 2021 •

edited

Loading

bghira commented Nov 9, 2021

ghost left a comment

ghost commented Nov 11, 2021

ghost commented Nov 11, 2021

ghost commented Nov 11, 2021

behlendorf left a comment

allanjude left a comment

Enable edonr in FreeBSD #12735

Enable edonr in FreeBSD #12735

Conversation

rincebrain commented Nov 9, 2021

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

rincebrain commented Nov 9, 2021 • edited Loading

rincebrain commented Nov 9, 2021 • edited Loading

ghost commented Nov 9, 2021

rincebrain commented Nov 9, 2021

ghost commented Nov 9, 2021 • edited by ghost Loading

rincebrain commented Nov 9, 2021

ghost commented Nov 9, 2021

bghira commented Nov 9, 2021

rincebrain commented Nov 9, 2021 • edited Loading

bghira commented Nov 9, 2021

ghost left a comment

Choose a reason for hiding this comment

ghost commented Nov 11, 2021

ghost commented Nov 11, 2021

ghost commented Nov 11, 2021

behlendorf left a comment

Choose a reason for hiding this comment

allanjude left a comment

Choose a reason for hiding this comment

rincebrain commented Nov 9, 2021 •

edited

Loading

rincebrain commented Nov 9, 2021 •

edited

Loading

ghost commented Nov 9, 2021 •

edited by ghost

Loading

rincebrain commented Nov 9, 2021 •

edited

Loading