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

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 5054
Build 5133: arc lint + arc unit

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

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.

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) ?

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!

rename to zfsbootcfg

allow empty argument to wipe out the next boot options

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
39 ↗	(On Diff #20087)	This should be either The .Nm utility is used to set OR .Nm is used to set
45 ↗	(On Diff #20087)	the next time the machine is booted.
76 ↗	(On Diff #20087)	without changing the
80 ↗	(On Diff #20087)	s/znextboot/zfsbootcfg/ (or use .Nm? I'm not sure if that works in this context...?)
84 ↗	(On Diff #20087)	Same as above.
91 ↗	(On Diff #20087)	.Fx 12.0 .
98 ↗	(On Diff #20087)	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 `Makefile`s.

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
91 ↗	(On Diff #20087)	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 ↗	(On Diff #20138)	Needs a comma after "moment"
111 ↗	(On Diff #20138)	"be able to" is not needed.

allanjude added a reviewer: tsoome.Sep 30 2016, 10:18 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...

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.

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.

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

@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!

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
32 ↗	(On Diff #21017)	This should probably just be "next boot".
50 ↗	(On Diff #21017)	`If booting fails,`
51 ↗	(On Diff #21017)	Passive -> active s/will automatically revert/automatically reverts/ No need for the possessive "its" here, just "the".
52 ↗	(On Diff #21017)	`boot configuration for the next boot.`
57 ↗	(On Diff #21017)	`read the boot option information from the first disk found in the first`
58 ↗	(On Diff #21017)	`ZFS pool found.`

man pages fixes suggested by wblock

Herald edited edge metadata. · View Herald TranscriptOct 12 2016, 7:21 AM

@wblock thank you!

In D7612#170785, @avg wrote:

@wblock thank you!

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

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?

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.

@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.

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
32 ↗	(On Diff #21295)	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"
40 ↗	(On Diff #21295)	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
50 ↗	(On Diff #21295)	Missing a comma for the pause after "fails": If booting fails, the machine automatically reverts to the previous
51 ↗	(On Diff #21295)	Probably don't need "boot" again here: configuration for the next boot.
56 ↗	(On Diff #21295)	read the boot option information from the first disk found in the first

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
32 ↗	(On Diff #21295)	Sorry for my misunderstanding, will fix.

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.

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 20086

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

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

sbin/Makefile

sbin/zfsbootcfg/

sbin/zfsbootcfg/Makefile

sbin/zfsbootcfg/zfsbootcfg.c

sys/boot/i386/common/drv.h

sys/boot/i386/common/drv.c

sys/boot/i386/gptzfsboot/Makefile

sys/boot/i386/zfsboot/Makefile

sys/boot/i386/zfsboot/zfsboot.c

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

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

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

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

zfsbootcfg: a simple tool to set next boot (one time) options for zfsbootClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 20086

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

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

sbin/Makefile

sbin/zfsbootcfg/

sbin/zfsbootcfg/Makefile

sbin/zfsbootcfg/zfsbootcfg.c

sys/boot/i386/common/drv.h

sys/boot/i386/common/drv.c

sys/boot/i386/gptzfsboot/Makefile

sys/boot/i386/zfsboot/Makefile

sys/boot/i386/zfsboot/zfsboot.c

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

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

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

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

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

Revision Contents
Changeset List