-
Notifications
You must be signed in to change notification settings - Fork 1.9k
bootenv: reorganise and document #17985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
I'll just note here (since the header message is the original source of regression) that we would absolutely love to get rid of the ritual for trying to write anything in userland that's even tangentially using libzfs / friends. We can of course consolidate this to a Makefile.inc or Makefile.zfs somewhere, but it's still a tangled mess of include paths with some strict ordering requirements. bectl(8) (previously linked) notably doesn't even use libzfs.h, it just wanted <libnvpair.h> via <be.h> but even that required some non-standard types that invoke the ritual. |
|
I think it is good to try to minimize the dependency from zfs code, but I'm not really sure it actually is possible; especially if we are talking about changing the label layout and having "independent" code updating label area may lead to interesting compatibility issues. |
In 99d7453 I split out the "OS-specific" zfs_bootenv_os.h for userspace, added a placeholder "posix" vendor and set BOOTENV_OS to use it by default. This turned out to be the wrong thing to do. BOOTENV_OS is not actually OS-specific but rather, part of a protocol coordinating behaviour between libzfsbootenv and the bootloader. So, all bootenv keys now get prefixed with posix:, and their respective loaders can no longer find them[1]. Reverting would have been straightforward, however, I am more interested in trying to reorganise libzfsbootenv and surrounds to provide all the pieces a loader, its support tools and the kernel module need in order to have agreement on how to communicate with each other. This commit is attempting that rework. The two goals here are: - it should be possible to set and get the loader prefix in libzfsbootenv at run time, allowing the possibility for the library to work on loader prefixes other than the one it was compiled with. - a loader is a constrained environment, and so should be able to get all the config it needs from a single header that libzfsbootenv and the kernel at least default to using. FreeBSD (and illumos, which borrows heavily from it) is the only system out there that currently has this tight integration between loader, kernel and tools, so it is the obvious target to focus on here, but I'm also trying to clearly draw the lines for future systems. First, we remove sys/zfs_bootenv.h and the various sys/zfs_bootenv_os.h. This is not really a public API, and as discussed, is not the proper way to target a loader anyway. The important macros for a consumer have been moved to fs/zfs.h as public and supported names. These are the minimum necessary to use libzfsbootenv (actually lzc_get_bootenv/lzc_set_bootenv) effectively anyway, and since they live in headers, a standalone loader can make use of them. Here we have macros nvlist keys for "known" loaders, and for common/known bootenv vars. Worth noting that only "bootonce" is currently used in libzfsbootenv proper, but listing them out at least makes sure they're documented and we can encourage future systems to do it the same way. libzfsbootenv gains an internal notion of the current loader, just the string prefix. Two new functions are added, lzbe_set_loader() and lzbe_get_loader(), allowing runtime changes. By compiler define, we set the default on FreeBSD and illumos to the matching macro from fs/zfs.h, and "unknown" otherwise. This should keep existing consumers working without change. The rest is simple cleanup. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <robn@despairlabs.com> 1. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=291211
14f7f62 to
ee23f22
Compare
|
yea, OpenZFS header includes is black manage. In FreeBSD, I'd love to get it into a macro, and maybe it would be nice if that responsibility resided in the upstream somehow. I agree with @kevans91 : the current situation is super ugly. A good test would be to create a filesystem image (I have some scripts that do this for pkgbase that may suffice), boot it in qemu, create a new be, activate it -t, and either use As for the bootenv cleanup, I'm generally in favor. It might be better to document different protocols around than than to try to standardize at this late date (since any bootloader already with its own protocol likely will have years of transition needed here, even though the boot once feature implemented as one of these protocols is infrequently used, it does have to always work). So at the very least, zfsbootcfg has to write a protocol the boot loader will understand. Today, that's relatively highly coupled in FreeBSD, though the protocol is simple. I do rather like a future where any can be written. |
Motivation and Context
In 99d7453 I split out the "OS-specific"
zfs_bootenv_os.hfor userspace, added a placeholder "posix" vendor and setBOOTENV_OSto use it by default. This turned out to be the wrong thing to do.BOOTENV_OSis not actually OS-specific but rather, part of a protocol coordinating behaviour between libzfsbootenv and the bootloader. So, all bootenv keys now get prefixed withposix:, and their respective loaders can no longer find them. See FreeBSD bug 291211,Reverting would have been straightforward, however, I am more interested in trying to reorganise libzfsbootenv and surrounds to provide all the pieces a loader, its support tools and the kernel module need in order to have agreement on how to communicate with each other. This commit is attempting that rework.
/cc @bsdimp @kevans91 @tsoome
Description
The two goals here are:
it should be possible to set and get the loader prefix in libzfsbootenv at run time, allowing the possibility for the library to work on loader prefixes other than the one it was compiled with.
a loader is a constrained environment, and so should be able to get all the config it needs from a single header that libzfsbootenv and the kernel at least default to using.
FreeBSD (and illumos, which borrows heavily from it) is the only system out there that currently has this tight integration between loader, kernel and tools, so it is the obvious target to focus on here, but I'm also trying to clearly draw the lines for future systems.
First, we remove
sys/zfs_bootenv.hand the varioussys/zfs_bootenv_os.h. This is not really a public API, and as discussed, is not the proper way to target a loader anyway.The important macros for a consumer have been moved to
fs/zfs.has public and supported names. These are the minimum necessary to use libzfsbootenv (actuallylzc_get_bootenv()/lzc_set_bootenv()) effectively anyway, and since they live in headers, a standalone loader can make use of them.Here we have macros nvlist keys for "known" loaders, and for common/known bootenv vars. Worth noting that only "bootonce" is currently used in libzfsbootenv proper, but listing them out at least makes sure they're documented and we can encourage future systems to do it the same way.
libzfsbootenv gains an internal notion of the current loader, just the string prefix. Two new functions are added,
lzbe_set_loader()andlzbe_get_loader(), allowing runtime changes. By compiler define, we set the default on FreeBSD and illumos to the matching macro fromfs/zfs.h, and "unknown" otherwise. This should keep existing consumers working without change.The rest is simple cleanup.
How Has This Been Tested?
Compiled checked on Linux and FreeBSD 15.0-RC4. ZTS run on Linux completed, but we don't really exercise the bootenv code there much so I didn't expect it to show anything.
(Incidentally, it might be worth bundling
zfsbootcfginto either the test suite or even as a binary we ship).I've done what might pass at the support patch for FreeBSD in robn/freebsd-src@06e509f; still working with the FreeBSD folks on that.
Types of changes
Checklist:
Signed-off-by.