Split "shared" userspace & kernel code into separate files #16492

robn · 2024-08-31T09:02:21Z

Motivation and Context

My continuing mission to make libzpool into a first-class citizen.

Why I've chosen that as a mission is a whole separate thing, that I'm planning on a talk on. For now, at least, lets say similar reasons as #16253: I would like to experiment with different implementations for some of these things in both the kernel and in userspace, and them sharing code makes it tough to change one without worrying about breaking the other.

Description

lib/libzpool/Makefile.am lists a few files that are shared between userspace and the Linux kernel. This commit gives userspace its own source for all but two of these:

arc_os.c: mostly no shared code, just lift the userspace code out
trace.c: remove from userspace builds, its already an effective no-op
vdev_label_os.c: already a no-op for Linux, just copy it
zfs_debug.c: just a very straightforward list-based implementation for userspace, and the printing functions only exist there
zfs_racct.c: already a no-op for Linux, just copy it
zfs_znode.c: much of the Linux and FreeBSD code was identical, so lifted all that into a common zfs_znode.c. Userspace has no ZPL (yet), so no need for an implementation of this for it.

The remaining files (vdev_file.c, zio_crypt.c) require more work, so I'll get them in a future PR.

How Has This Been Tested?

Build checked on Linux 6.1.99 and FreeBSD 14.1. ZTS run within error bars 💀

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.

amotin

Looks fine on a quick look. Just one minor thought:

amotin · 2024-09-05T14:18:36Z

lib/libzpool/vdev_label_os.c

+/*
+ * Check if the reserved boot area is in-use.
+ *
+ * This function always returns 0, as there are no known external uses
+ * of the reserved area on Linux.


FreeBSD might use reserved area for booting in case of MBR partitioning. If it is relevant here and now.

I just lifted the existing code. I don't know what happens on FreeBSD really. If you can give me a brief description I can put a TODO comment in here?

It is not something that is really happenING, more like happenED. Before GPT spread there was a weird way of booting from ZFS on MBR, which IIRC included storing some boot code in the reserved areas of ZFS, which is incompatible with RAIDZ expansion. I've never done ZFS on MBR myself, and these days I am not sure many people use MBR at all, so it may be a moot point. Also I am not sure whether this function is really useful for user-space code, so was merely bringing an attention to think of possible functionality loss.

I've updated the comment. I think probably the kernel FreeBSD version could actually just be the generic version for all platforms, because I don't really see why an apparently-in-use reserved area should be ignored in Linux; what if its a dual-boot system or something? But I think that's for a future change.

lundman · 2024-09-07T07:20:55Z

So I understand, we will then have a separate zfs_znode.c for just useland, and manually keep the sources synchronised? Is that better?

robn · 2024-09-07T07:29:57Z

So I understand, we will then have a separate zfs_znode.c for just useland, and manually keep the sources synchronised? Is that better?

There is a common zfs_znode.c for all variants, then an OS-specific zfs_znode_os.c that provides the glue to the platform filesystem layer (eg kernel VFS). Userspace has never had such glue; it was all behind #ifdef _KERNEL before, so nothing is lost. If it ever does have such glue (not sure what that looks like), then it'd be in lib/libzpool/zfs_znode.os.c or something.

The intent here is to not have to keep things in sync; before this PR, module/os/linux/zfs/zfs_znode.c and module/os/freebsd/zfs/zfs_znode.c had a lot of duplicate code; now its shared.

lundman · 2024-09-07T07:57:40Z

Nice, then I follow, carry on

The Linux arc_os.c carries userspace and kernel code, with very little overlap between the two. This lifts the userspace parts out into a separate arc_os.c for libzpool and removes it from the Linux side. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/

The no-op is fine for both. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/

It does nothing in userspace anyway. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/

The no-op is fine for both. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/

For now, userspace has no znode implementation. Some of the property and path handling code is used there though and is the same on all platforms, so we only need a single copy of it. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/

Just nice and simple, with room to grow. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/

The no-op is fine for both. Reviewed by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/ Closes #16492

It does nothing in userspace anyway. Reviewed by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/ Closes #16492

The no-op is fine for both. Reviewed by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/ Closes #16492

For now, userspace has no znode implementation. Some of the property and path handling code is used there though and is the same on all platforms, so we only need a single copy of it. Reviewed by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/ Closes #16492

Just nice and simple, with room to grow. Reviewed by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/ Closes #16492

The Linux arc_os.c carries userspace and kernel code, with very little overlap between the two. This lifts the userspace parts out into a separate arc_os.c for libzpool and removes it from the Linux side. Reviewed by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/ Closes openzfs#16492

The no-op is fine for both. Reviewed by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/ Closes openzfs#16492

It does nothing in userspace anyway. Reviewed by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/ Closes openzfs#16492

The no-op is fine for both. Reviewed by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/ Closes openzfs#16492

For now, userspace has no znode implementation. Some of the property and path handling code is used there though and is the same on all platforms, so we only need a single copy of it. Reviewed by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/ Closes openzfs#16492

Just nice and simple, with room to grow. Reviewed by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/ Closes openzfs#16492

For some reason it was dropped when split from kernel, that makes raidz_test to accumulate in RAM up to 100GB of logs we don't need. Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc. Fixes openzfs#16492 Fixes openzfs#16566

For some reason it was dropped when split from kernel, that makes raidz_test to accumulate in RAM up to 100GB of logs we don't need. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Igor Kozhukhov <[email protected]> Reviewed-by: Rob Norris <[email protected]> Reviewed-by: Tino Reichardt <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc. Closes #16492 Closes #16566 Closes #16664

For some reason it was dropped when split from kernel, that makes raidz_test to accumulate in RAM up to 100GB of logs we don't need. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Igor Kozhukhov <[email protected]> Reviewed-by: Rob Norris <[email protected]> Reviewed-by: Tino Reichardt <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc. Closes openzfs#16492 Closes openzfs#16566 Closes openzfs#16664

robn force-pushed the userspace-split branch from 57ab1f2 to a24ea51 Compare September 4, 2024 22:42

amotin approved these changes Sep 5, 2024

View reviewed changes

robn force-pushed the userspace-split branch from a24ea51 to 4e2f8ba Compare September 7, 2024 06:34

behlendorf approved these changes Sep 13, 2024

View reviewed changes

behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Sep 13, 2024

robn force-pushed the userspace-split branch from 4e2f8ba to 6dd440b Compare September 16, 2024 10:09

robn added 6 commits September 19, 2024 12:06

vdev_label_os: copy Linux implementation for userspace

7e51116

The no-op is fine for both. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/

libzpool: don't include trace.c

d3ac91b

It does nothing in userspace anyway. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/

zfs_racct: copy Linux implementation for userspace

51fb1c5

The no-op is fine for both. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/

zfs_debug: specific variant for userspace

a15d19c

Just nice and simple, with room to grow. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: https://despairlabs.com/sponsor/

robn force-pushed the userspace-split branch from 6dd440b to a15d19c Compare September 19, 2024 02:11

behlendorf closed this in 8fc0beb Sep 19, 2024

amotin mentioned this pull request Oct 18, 2024

zfs_debug: Restore log size limit for userspace #16664

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split "shared" userspace & kernel code into separate files #16492

Split "shared" userspace & kernel code into separate files #16492

robn commented Aug 31, 2024 •

edited

Loading

amotin left a comment

amotin Sep 5, 2024

robn Sep 6, 2024

amotin Sep 6, 2024

robn Sep 7, 2024

lundman commented Sep 7, 2024

robn commented Sep 7, 2024

lundman commented Sep 7, 2024

Split "shared" userspace & kernel code into separate files #16492

Split "shared" userspace & kernel code into separate files #16492

Conversation

robn commented Aug 31, 2024 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

amotin left a comment

Choose a reason for hiding this comment

amotin Sep 5, 2024

Choose a reason for hiding this comment

robn Sep 6, 2024

Choose a reason for hiding this comment

amotin Sep 6, 2024

Choose a reason for hiding this comment

robn Sep 7, 2024

Choose a reason for hiding this comment

lundman commented Sep 7, 2024

robn commented Sep 7, 2024

lundman commented Sep 7, 2024

robn commented Aug 31, 2024 •

edited

Loading