Skip to content

Conversation

@robn
Copy link
Member

@robn robn commented Oct 22, 2025

Motivation and Context

As part of my ongoing project to make userspace into a "first class" platform, and to make OpenZFS easier to port to other platforms, I am working to make headers consistent across all platforms. I would like to get to a point where zfs_context.h is the same for all platforms and compile modes, and so provides a complete list of "necessary to implement" for any platform.

The first part of that is to lift all the userspace-specific stuff out of zfs_context.h into separated headers in lib/libspl/include. That's this PR.

This PR deliberately stops at emptying zfs_context.h of definitions. The next PR I open on this project will collapse it into a single list of headers that works across all platforms, but that will require movement across all platform headers, not just userspace, and I don't want to complicate review on this further.

Description

There's a lot of commits here, but they are basically just picking a specific concept or object from the userspace part zfs_context.h and moving it out to a specific header under lib/libspl/include. For most, that header does not already exist. For some, it does exist but wasn't used, or was in conflict, and has been updated appropriately.

After that, there's a long tail of misc stuff, which has been either moved somewhere more appropriate or deleted.

I expect that much of this could be collapsed into a smaller set of commits, but for the moment I'm keeping them separate for review.

How Has This Been Tested?

This should all be entirely mechanical, so anything that compiles should work.

The final series compiles on Linux and FreeBSD, and ZTS passes.

It should compile clean at each individual commit, but I have moved things around a little while assembling the final series, so something may be stacked wrong. I'll go and check that now, but its also pretty low-key.

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)
  • Quality assurance (non-breaking change which makes the code more robust against bugs)
  • 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:

@robn robn force-pushed the userspace-context-lift branch 2 times, most recently from e75afb5 to 5d26232 Compare October 22, 2025 03:11
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Aside from a a couple small nits this is a welcome bit of long overdue refactoring.

For next steps I think it would make the most sense to move the remaining user space implementations out of libzpool and into libspl. It looks like lib/libzpool/kernel.c and lib/libzpool/taskq.c should cover almost everything.

Do you want to tackle that in this PR or another? I'm fine with pulling in this whole stack of commits, having it broken up made it much easier to review.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Oct 22, 2025
@robn
Copy link
Member Author

robn commented Oct 23, 2025

Actually, responding to both your comments, I realised what happened here: zfs_context_os.h should not be in libspl - its for the "zfs module", not the SPL! That's why it doesn't make sense!

Both those functions are very libzpool-specific, so they can either say in zfs_context_os.h when I move it away from libspl, or they can go to a libzpool.h, along with kernel_init() etc - things the program needs to drive libzpool, not things that the module internals need to operate.

I'll give it a try. I think there's at least enough overlap with this PR that it could go here, but I'll see how it shakes out - if its ick enough, maybe the next PR is better. More soon!

robn added 25 commits October 29, 2025 16:42
Keeping the spl/zfs module split, libzpool is the zfs module for
userspace. Headers and functions specific to it belong there.
robn added 23 commits October 29, 2025 16:42
The extra inclusion via xvattr.h appears to upset the linter in CI. I'm
not entirely sure what its complaint is, but removing sys/string.h
entirely is not quite possible yet, and include guards are rarely a bad
idea, so this will do.

Sponsored-by: https://despairlabs.com/sponsor/
Signed-off-by: Rob Norris <[email protected]>
Sponsored-by: https://despairlabs.com/sponsor/
Signed-off-by: Rob Norris <[email protected]>
sys/debug.h is not really the right place for them, but we already have
some there for libspl, so it is at least convenient.

Sponsored-by: https://despairlabs.com/sponsor/
Signed-off-by: Rob Norris <[email protected]>
These are kind-of compiler attribute placeholders, so go here with the
others for now.

Sponsored-by: https://despairlabs.com/sponsor/
Signed-off-by: Rob Norris <[email protected]>
@robn robn force-pushed the userspace-context-lift branch from 5d26232 to c7b256c Compare October 29, 2025 05:47
@robn
Copy link
Member Author

robn commented Oct 29, 2025

@behlendorf ok, here we go. This actually doubles the size of the PR, but I'm not sure it cleanly splits into two. Let me know what you think. If I don't split, I should rewrite the OP at least.

The whole thing here is ensuring the the implementations of things live in the same place as their headers. So there's a bunch of stuff moved over to libspl from libzpool, libspl gains a dedicated libspl_init() and libspl_fini() to initialise physmem, the rng, etc, while libzpool gains a libzpool.h with kernel_init(), handle_tunable_option(), etc.

It's a lot, but I think it lands in a good place.

@behlendorf
Copy link
Contributor

This is looking good, and it definitely feels like we're moving in the right direction.

Where I'd really like to end up is only including the sys/zfs_context.h headers when building the zfs kmod, libzpool, or low-level commands like zdb, zhack, ztest which can muck around with a pools internals. We're getting closer, but it seems like libzfs ends up including zfs_ioctl.hfor some shared user/kernel structures. That's fine, but it unfortunately drags in a bunch of other zfs headers we don't need. So there's some refactoring needed there stiil.

I took the liberty of pushing this PR a bit further with the above goal in mind. What do think of these proposed additions from https://github.com/behlendorf/zfs/tree/userspace-context-lift-part-2.

  1. Unify the new lib/libzpool/include headers with the existing include/sys/ / include/os/<OS>/zfs/sys headers. This is already where all of the other core zfs headers live, several of which are already user/kernel aware, so we might as well keep them in one place.

  2. Pulled all of the SET_ERROR / DTRACE_PROBE usage and bogus headers out of the spl layer to fix this layering violation. If we're going to wire those up to the internal zfs debug infrastructure they should live in zfs_debug.h. Luckily they were lightly used in the spl layer so I don't think we're losing much.

  3. Stopped adding -I$(top_srcdir)/include/os/@ac_system_l@/zfs to the default user include paths. I've added LIBZPOOL_CPPFLAGS to make it easy to add this path were we still need it today. My goal is the gradually clean up the headers so we can drop it from most of the Makefiles. Adding a similar LIBSPL_CPPFLAGS and giving it the same treatment seems like a reasonable thing to do. Although I haven't tried it yet.

  4. Stop adding -I$(top_srcdir)/module/icp/include to the default user include paths. Again, only set it when it's needed.

It might be a good idea to go even farther and move the headers in to include/spl/ and include/zfs/ directories. Something to mull over a bit more. Anyway, that's enough for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Code Review Needed Ready for review and testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants