Page MenuHomeFreeBSD
Differential D7612

zfsbootcfg: a simple tool to set next boot (one time) options for zfsboot
ClosedPublic
Actions

Authored by avg on Aug 23 2016, 1:12 PM.
Tags
None
Referenced Files
F103497391: D7612.vs21017.id21295.whitespaceignore-most.diff
Mon, Nov 25, 6:42 PM
Unknown Object (File)
Fri, Nov 22, 10:51 PM
Unknown Object (File)
Fri, Nov 22, 5:09 PM
Unknown Object (File)
Sat, Nov 16, 1:18 PM
Unknown Object (File)
Sun, Oct 27, 5:21 AM
Unknown Object (File)
Oct 19 2024, 8:17 AM
Unknown Object (File)
Oct 17 2024, 12:20 AM
Unknown Object (File)
Oct 16 2024, 3:50 PM
View All Files
Subscribers
allanjude
emaste
imp
rpokala
wblock
ZFS

Details

Reviewers
kib
jhb
delphij
wblock
mahrens
mav
smh
tsoome
Group Reviewers
manpages
Commits
rS308089: zfsbootcfg: a simple tool to set next boot (one time) options for zfsboot
Summary

zfsboot reads one time boot directives from ZFS pool Pad2 area.
The area was previously described as "Boot Block Header", but currently
it is know as Pad2, marked as reserved and is zeroed out on pool creation.
The new code interprets data in this area, if any, using the same format
as boot.config. The area is immediately wiped out.
Failure to parse the directives results in a reboot right after
the wipe out. Otherwise the boot sequence proceeds as usual.

zfsbootcfg writes zfsboot arguments specified on the command line to the Pad2
area of a disk identified by vfs.zfs.boot.primary_pool and
vfs.zfs.boot.primary_vdev kenv variables that are set by loader during boot.

Diff Detail

Event Timeline

avg updated this revision to Diff 19582.Aug 23 2016, 1:12 PM
avg retitled this revision from to znextboot: a simple tool to set next boot (one time) options for zfsboot.
avg updated this object.
avg edited the test plan for this revision. (Show Details)
avg added reviewers: delphij, mav, kib, jhb, smh.
avg added a subscriber: ZFS.
Herald added a subscriber: imp. · View Herald TranscriptAug 23 2016, 1:12 PM
avg added a reviewer: mahrens.Aug 23 2016, 1:31 PM
jhb edited edge metadata.Aug 23 2016, 10:23 PM

The only thing that stuck out to me is that 'nextboot' on FreeBSD doesn't select alternate partitions/filesystems (boot0cfg had a way to do that for boot0, and gpart can set 'bootme', etc. flags for GPT that do something like this in conjunction with gptboot(8)). 'nextboot' allows you to choose an alternate kernel from the current filesystem for the next boot (but falling back to the 'stock' kernel if it has to reboot due to crash on boot, etc.). To that end, I wish there was a different name that avoided conflating the different semantics of nextboot vs znextboot. I can't think of one off the top of my head.

Code wise the limited changes I saw in sys/boot looked ok. Matt will have to speak to the ZFS-related changes I imagine.

avg added a comment.Aug 23 2016, 10:42 PM
In D7612#158342, @jhb wrote:

To that end, I wish there was a different name that avoided conflating the different semantics of nextboot vs znextboot. I can't think of one off the top of my head.

Perhaps something like zfsbootcfg(following the same style as boot0cfg) ?

jhb added a comment.Aug 25 2016, 7:56 PM
In D7612#158345, @avg wrote:
In D7612#158342, @jhb wrote:

To that end, I wish there was a different name that avoided conflating the different semantics of nextboot vs znextboot. I can't think of one off the top of my head.

Perhaps something like zfsbootcfg(following the same style as boot0cfg) ?

That would work for me. Thanks!

avg updated this revision to Diff 20085.Sep 6 2016, 12:46 PM
avg edited edge metadata.

rename to zfsbootcfg

avg updated this revision to Diff 20086.Sep 6 2016, 12:48 PM

allow empty argument to wipe out the next boot options

avg updated this revision to Diff 20087.Sep 6 2016, 12:48 PM

add a basic manual page

Herald added a reviewer: manpages. · View Herald TranscriptSep 6 2016, 12:48 PM
avg retitled this revision from znextboot: a simple tool to set next boot (one time) options for zfsboot to zfsbootcfg: a simple tool to set next boot (one time) options for zfsboot.Sep 6 2016, 12:53 PM
avg updated this object.
avg edited edge metadata.
rpokala added a subscriber: rpokala.Sep 6 2016, 2:21 PM
rpokala added inline comments.
sbin/zfsbootcfg/Makefile
15

ISTR seeing something on the lists that ${.CURDIR}/../../ should be avoided, and to use ${SRCTOP} instead?

sbin/zfsbootcfg/zfsbootcfg.8
40

This should be either

The
.Nm
utility is used to set

OR

.Nm
is used to set
46

the next time the machine is booted.

77

without changing the

81

s/znextboot/zfsbootcfg/

(or use .Nm? I'm not sure if that works in this context...?)

85

Same as above.

92

.Fx 12.0 .

99

uses the

emaste added a subscriber: emaste.Sep 6 2016, 6:17 PM
emaste added inline comments.
sbin/zfsbootcfg/Makefile
15

I'm not sure there's been an explicit mention, but ${SRCTOP} is indeed preferred in new Makefiles.

avg updated this revision to Diff 20138.Sep 7 2016, 1:17 PM
  • use SRCTOP
  • manual page fixes suggested by rpokala
Herald edited edge metadata. · View Herald TranscriptSep 7 2016, 1:17 PM
avg marked 9 inline comments as done.Sep 7 2016, 1:24 PM
avg added inline comments.
sbin/zfsbootcfg/zfsbootcfg.8
92

I was not sure what to put here.
Will I have to change the version when I MFC the change to stable/XX ?

avg marked an inline comment as done.Sep 7 2016, 1:25 PM
avg mentioned this in D7797: rc.d/zfsbe: a new script designed for boot environment support.Sep 7 2016, 7:48 PM
allanjude added a subscriber: allanjude.Sep 30 2016, 6:18 PM
wblock added a subscriber: wblock.Sep 30 2016, 9:01 PM
wblock added inline comments.
sbin/zfsbootcfg/zfsbootcfg.8
95

Needs a comma after "moment"

111

"be able to" is not needed.

allanjude added a reviewer: tsoome.Sep 30 2016, 10:18 PM
tsoome edited edge metadata.Sep 30 2016, 11:40 PM

There is an issue:) The idea is nice and will complement the freebsd logic, but You are missing EFI version. I think UEFI and BIOS versions should be kept as functionally close as possible, especially as one can have both variants in the same system, and select based on boot device name...

tsoome added a comment.Oct 1 2016, 6:38 AM

Note about tooling, for illumos port I'll definitely go for "beadm activate -t bename" as it will keep the user experience logic, but since current beadm in freebsd is script, you need something to actually set the data. Also as there is an work to port beadm/libbe bits, its still something to think about.

tsoome added inline comments.Oct 1 2016, 7:55 AM
cddl/contrib/opensolaris/lib/libzfs/common/libzfs.h
216

It would be nice to have get/set nextboot interface, so we could have an way to verify the nextboot setup.

tsoome added a comment.Oct 1 2016, 10:55 AM

I hope Matt can give more/better advice, but I have this nagging feeling about label updates in kernel...

First of all, we have nice generic label update mechanism, I think, it should be possible to put it into the use there, and avoid custom interface for specific field.

What about something like this:

When nextboot is set (SET ioctl), we update "pad2" data field and sync pool/vdev labels on disk, note, that we have 4 labels per disk anyhow, so we can get *all* the label instances in place.

Now from loader, when pool is probed, we can fetch the pad2 (need better name there;), while probing, and as soon as we have determined the best UB, we can either nuke the pad2 or pass the location back to boot2 for nuking. Nuking in place would even be nice as it will happen inside zfs module and therefore we do not need to duplicate the implementation for bios/uefi (or other platforms). So the nextboot is reset, and on reboot, the normal bootfs property logic will take over.

When kernel is starting and opening the boot pool, it can apply additional label sync with clean pad2, to make sure the nextboot is reset for all label copies.

I think this might help to reduce down the corner cases, also perhaps could simplify the possible get/set interface; I would still like to see the get interface because whenever I perform the changes, I do like to verify if things are indeed properly set (guess I'm not alone there:) - for example even after beadm activate, I usually do verify the result from beadm list;)

Anyhow, those are just the ideas, perhaps flawed ones, perhaps useful.

avg updated this revision to Diff 21017.Oct 4 2016, 9:22 AM
avg edited edge metadata.

improve the man page based on the wblock's comments

Herald edited edge metadata. · View Herald TranscriptOct 4 2016, 9:22 AM
avg marked 2 inline comments as done.Oct 4 2016, 9:23 AM
avg added a comment.Oct 4 2016, 9:35 AM

@tsoome I hope it's okay if I take your comments as suggestions for the future enhancements rather than blockers.

EFI is a relatively new thing, I am not very familiar with the code, I do not have any systems with it, and I hope that something better (more flexible and convenient) could be done for EFI using its capabilities.

'Get' interface would be, of course, very nice to have, but for now, if you are unsure what's configured for the next boot, you can simply overwrite or clear it.

Regarding the higher level tools. Yep, they could use zfsbootcfg or the new libzfs function or the new ioctl directly depending on their language and what's more convenient for them. I do not see any blockers here.

With regard to using all labels, I need to think about that. I wanted to keep the code very simple for now. Your suggestion sounds good to me and it does not seem hard to implement. Just need to think about it more.

Thank you!

tsoome added a comment.Oct 4 2016, 10:45 AM
In D7612#168708, @avg wrote:

@tsoome I hope it's okay if I take your comments as suggestions for the future enhancements rather than blockers.

EFI is a relatively new thing, I am not very familiar with the code, I do not have any systems with it, and I hope that something better (more flexible and convenient) could be done for EFI using its capabilities.

'Get' interface would be, of course, very nice to have, but for now, if you are unsure what's configured for the next boot, you can simply overwrite or clear it.

Regarding the higher level tools. Yep, they could use zfsbootcfg or the new libzfs function or the new ioctl directly depending on their language and what's more convenient for them. I do not see any blockers here.

With regard to using all labels, I need to think about that. I wanted to keep the code very simple for now. Your suggestion sounds good to me and it does not seem hard to implement. Just need to think about it more.

Thank you!

Absolutely, I was trying to iterate about ideas I got, and perhaps not all are possible at freebsd context, at least not immediately . And once you have first iteration, it can be improved/redone etc.

wblock added inline comments.Oct 7 2016, 2:50 PM
sbin/zfsbootcfg/zfsbootcfg.8
33

This should probably just be "next boot".

51

If booting fails,

52

Passive -> active
s/will automatically revert/automatically reverts/

No need for the possessive "its" here, just "the".

53

boot configuration for the next boot.

58

read the boot option information from the first disk found in the first

59

ZFS pool found.

avg updated this revision to Diff 21295.Oct 12 2016, 7:21 AM
avg marked 6 inline comments as done.
avg edited edge metadata.

man pages fixes suggested by wblock

Herald edited edge metadata. · View Herald TranscriptOct 12 2016, 7:21 AM
avg added a comment.Oct 12 2016, 7:22 AM

@wblock thank you!

wblock added a comment.Oct 14 2016, 4:06 PM
In D7612#170785, @avg wrote:

@wblock thank you!

You're welcome, but none of them seem to have changed the source.

avg added a comment.Oct 14 2016, 4:13 PM
In D7612#171482, @wblock wrote:
In D7612#170785, @avg wrote:

@wblock thank you!

You're welcome, but none of them seem to have changed the source.

Apologies, I failed to understand this comment... could you please explain?

wblock added a comment.Oct 14 2016, 4:37 PM
In D7612#171485, @avg wrote:
In D7612#171482, @wblock wrote:
In D7612#170785, @avg wrote:

@wblock thank you!

You're welcome, but none of them seem to have changed the source.

Apologies, I failed to understand this comment... could you please explain?

They were all marked done, but without any changes to the man page source. This might be Phabricator "helping" out, or a not-updated diff, or just that they were considered but rejected.

avg added a comment.Oct 14 2016, 4:49 PM

@wblock I remember making them and updating the diff.
I look at this https://reviews.freebsd.org/D7612?vs=21017&id=21295&whitespace=ignore-most#toc and I see the changes.

wblock added a comment.Oct 14 2016, 5:34 PM
In D7612#171511, @avg wrote:

@wblock I remember making them and updating the diff.
I look at this https://reviews.freebsd.org/D7612?vs=21017&id=21295&whitespace=ignore-most#toc and I see the changes.

That did show some of them. I went back through and checked them. Thanks!

sbin/zfsbootcfg/zfsbootcfg.8
33

The suggestion was to s/reboot/boot/, not remote "the". So for this line, it should be:

.Nd "specify zfsboot options for the next boot"
41

It's hard to say how to mark this up. The sentence probably should read (rendered) as

zfsbootcfg is used to set boot.config(5)-style options to be used by

This markup renders okay, but might not be the right way to do it:

.Nm
is used to set
.Xr boot.config 5 Ns -style
options to be used by
51

Missing a comma for the pause after "fails":

If booting fails, the machine automatically reverts to the previous
52

Probably don't need "boot" again here:

configuration for the next boot.
57
read the boot option information from the first disk found in the first
avg updated this revision to Diff 21416.Oct 15 2016, 9:33 AM
avg edited edge metadata.

more man page fixes for things that I misunderstood or missed.
suggested by wblock

Herald edited edge metadata. · View Herald TranscriptOct 15 2016, 9:33 AM
avg marked 4 inline comments as done.Oct 15 2016, 9:34 AM
avg added inline comments.
sbin/zfsbootcfg/zfsbootcfg.8
33

Sorry for my misunderstanding, will fix.

wblock accepted this revision.Oct 16 2016, 5:48 PM
wblock added a reviewer: wblock.

Thank you!

This revision is now accepted and ready to land.Oct 16 2016, 5:48 PM
Closed by commit rS308089: zfsbootcfg: a simple tool to set next boot (one time) options for zfsboot (authored by avg). · Explain WhyOct 29 2016, 2:09 PM
This revision was automatically updated to reflect the committed changes.
avg marked an inline comment as done.

Revision Contents
Changeset List

PathSize
cddl/
contrib/
opensolaris/
lib/
libzfs/
common/
1 line
22 lines
sbin/
1 line
27 lines
113 lines
71 lines
sys/
boot/
i386/
common/
4 lines
4 lines
gptzfsboot/
2 lines
zfsboot/
2 lines
127 lines
cddl/
contrib/
opensolaris/
uts/
common/
fs/
zfs/
sys/
2 lines
38 lines
50 lines
sys/
fs/
1 line

Diff 20138

View Options

cddl/contrib/opensolaris/lib/libzfs/common/libzfs.h

Loading...
View Options

cddl/contrib/opensolaris/lib/libzfs/common/libzfs_pool.c

Loading...
View Options

sbin/Makefile

Loading...
View Options

sbin/zfsbootcfg/

Loading...
View Options

sbin/zfsbootcfg/Makefile

Loading...
View Options

sbin/zfsbootcfg/zfsbootcfg.8

Loading...
View Options

sbin/zfsbootcfg/zfsbootcfg.c

Loading...
View Options

sys/boot/i386/common/drv.h

Loading...
View Options

sys/boot/i386/common/drv.c

Loading...
View Options

sys/boot/i386/gptzfsboot/Makefile

Loading...
View Options

sys/boot/i386/zfsboot/Makefile

Loading...
View Options

sys/boot/i386/zfsboot/zfsboot.c

Loading...
View Options

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/vdev.h

Loading...
View Options

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_label.c

Loading...
View Options

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c

Loading...
View Options

sys/cddl/contrib/opensolaris/uts/common/sys/fs/zfs.h

Loading...