module: weld all but spl.ko into zfs.ko

[nabijaczleweli]

Motivation and Context

I guess this supersedes #13206?

We can't weld spl.ko in because it launders hand-washes GPL-only symbols: #13206 (comment)

How Has This Been Tested?

git grep 🙈; builds in- and out-of-tree, zstd symbols get regenerated idempotently, I tried the explicit targets in the Makefile and they all work fine.

Haven't tried DKMS builds, built-in (but CI will), FreeBSD, or any of the Fedora-specific stuff (but I did fix them so long as they mentioned the module names).

zfs is aliased to all the previous module names so this is transparent (except for /sys/module/XXX/ paths, but there was exactly one in use).

I just inlined the init/fini externs for zcommon in both callsites. I don't think it matters much, it's also what we do for some parameters on FreeBSD.

Cc: @rincebrain

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. – fixed them, the rest should be fine probably
• I have run the ZFS Test Suite with this change applied. – CI take my hand, also see sexion 2
• All commit messages are properly formatted and contain Signed-off-by.

[nabijaczleweli]

I also did a pass over licences and authors and now the modinfo looks like this:

root@kasan-test:~/zfs# modinfo module/zfs.ko  | head -30
filename:       /root/zfs/module/zfs.ko
version:        2.1.99-991_g5e98d1db2
license:        CDDL
license:        Dual BSD/GPL
license:        zstd: Dual BSD/GPL
license:        Lua: MIT
author:         OpenZFS
author:         zstd: Facebook, Inc.
author:         Lua: Lua.org, PUC-Rio
description:    ZFS
alias:          zzstd
alias:          zcommon
alias:          zunicode
alias:          znvpair
alias:          zlua
alias:          icp
alias:          zavl
alias:          devname:zfs
alias:          char-major-10-249
srcversion:     5C193FC283BE6EE531D4959
depends:        spl
retpoline:      Y
name:           zfs
vermagic:       5.16.0-5-amd64 SMP preempt mod_unload modversions

(Fletcher implementations are BSD/GPL.)

Only the first stanza matters, so this still taints the kernel.

[rincebrain]

I would suggest that #13206 might want to still land before this does, if only because #13206 is much less disruptive to, say, backport into 2.1. (Not saying that you can't do that for this, just that the act of flattening the modules might be a bit much to do in a point release, IMO.)

I figured I'd do a more thorough read once the CI has less ❌, but my only immediate thought was "we probably want zfs.sh to still try to unload the old module names for at least a few versions, otherwise zfs.sh -u won't DTRT if you cross this boundary". (I also thought the same about make uninstall.)

[nabijaczleweli]

Neither of these are backportable because both of them break a committed userspace ABI (except this does {icp => zfs}.icp_{{aes,gcm}_impl,gcm_avx_chunk_size} and {zcommon => zfs}.zfs_{fletcher_4_impl,max_dataset_missing} and #13206 does {zfs => zcommon}.zfs_dbgmsg_{enable,maxsize}, which would (a) get reverted when this lands and (b) this accomplishes the same goal).

The upgrade path is a good point. I fixed zfs.sh to just unload with dependencies via modprobe -r, which works everywhere, on both sides of the upgrade. And the (un)installation by removing the former directories, in a separate, trivially reversible (once we hit 3.0, or whenever we'll be able to safely) commit.

[rincebrain]

I don't know that we promise anything about the tunables not changing in point releases, but it's simple enough to also still expose them from zfs in addition to zcommon, like #13206 currently does with zfs_flags.

[nabijaczleweli]

Does it? Looks to me like it got moved wholesale, including the tunable procfs path.

[rincebrain]

I believe I moved it and copied the tunable code, but didn't delete the old one.

[nabijaczleweli]

Well, yeah, but the other two zfs_dbgmsg_{enable,maxsize} tunables did get moved, so that's, I'd say, worse?

[rincebrain]

Sure, my remark was that that's trivial to add back again, if that's the primary concern.

[nabijaczleweli]

I mean, if you foresee new code using zfs_dbgmsg outside zfs.ko proper in 2.x backports, then so long as you move those tunables back to zfs.ko as well, then this is transparent to the user, and I've no objections

[rincebrain]

I believe RH will give you free RHEL licenses if you register as a developer with them. And by "I believe" I mean "I have such an account and licenses."

…

On Wed, Apr 6, 2022 at 8:33 PM наб ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In module/Kbuild.in <#13274 (comment)>: > @@ -1,31 +1,23 @@ # When integrated in to a monolithic kernel the spl module must appear # first. This ensures its module initialization function is run before # any of the other module initialization functions which depend on it. -ZFS_MODULES += spl/ -ZFS_MODULES += avl/ -ZFS_MODULES += icp/ -ZFS_MODULES += lua/ -ZFS_MODULES += nvpair/ -ZFS_MODULES += unicode/ -ZFS_MODULES += zcommon/ -ZFS_MODULES += zfs/ -ZFS_MODULES += zstd/ - -# The rest is only relevant when run by kbuild -ifneq ($(KERNELRELEASE),) - -obj-$(CONFIG_ZFS) := $(ZFS_MODULES) Didn't try CentOS at all, but I would've if I hadn't managed to configure for a kernel build without coming up with a repository licence. This is a great tip if I'm ever unfortunate enough to need it, though, thanks! — Reply to this email directly, view it on GitHub <#13274 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABUI7PHHK2RMQUKXWU6DYDVDYUORANCNFSM5SF3Y7VQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[nabijaczleweli]

CI's all green (save for built-in, but that's because of default_attrs) – is there anything else needed for this?

[behlendorf]

This looks good to me, just one trivial cosmetic nit.