7537 want nextboot (one time boot) support

Review Request #249 - Created Nov. 6, 2016 and updated

Information
Toomas Soome
illumos-gate
7537
c2c46b6...
Reviewers
general
7537 want nextboot (one time boot) support

bootadm activate -t/-T and boot time/post reboot verification.

Issues

  • 9
  • 8
  • 0
  • 17
Description From Last Updated
Why do we return the best error? If only one disk managed to successfully write the nextboot information, and the ... Paul Dagnelie Paul Dagnelie
Could we verify the checksum here? Paul Dagnelie Paul Dagnelie
It's a little confusing that we are defining the "nextboot" to mean the "dataset" string (for zpool_get_nextboot) and the "zfs:dataset:" ... Matthew Ahrens Matthew Ahrens
The semantic here is that we return a random pad2, unless one of them is a zero-length string, in which ... Matthew Ahrens Matthew Ahrens
This seems very specific to treating pad2 as a string. I.e. we aren't really checking if it's "zeroed", only that ... Matthew Ahrens Matthew Ahrens
Why is this needed? It looks like we are modifying the pad2 area of the labels, and not modifying anything ... Matthew Ahrens Matthew Ahrens
Are we intentionally allowing this on suspended / readonly pools? That seems dangerous. We may also want to pass allow_log=B_TRUE. ... Matthew Ahrens Matthew Ahrens
What would be the downside of using zfs_secpolicy_read here? Matthew Ahrens Matthew Ahrens
What would be the downside of using zfs_secpolicy_read here? Matthew Ahrens Matthew Ahrens
Paul Dagnelie
Paul Dagnelie

   
usr/src/boot/sys/boot/zfs/zfs.c (Diff revision 2)
 
 

Why are we using zap_scratch here? I guess it doesn't really matter, but it seems wrong to use the zap_scratch space for non-zap stuff.

usr/src/boot/sys/boot/zfs/zfs.c (Diff revision 2)
 
 

Why do we use a precalculated checksum here? It looks like the sha256 source code is present in usr/src/boot/sys/cddl/boot/zfs/sha256.c. Could we use that instead?

usr/src/boot/sys/boot/zfs/zfs.c (Diff revision 2)
 
 

Could we verify the checksum here?

Matthew Ahrens

   
usr/src/lib/libzfs/common/libzfs_impl.h (Diff revision 2)
 
 

use the zpool_ prefix

usr/src/lib/libzfs/common/libzfs_pool.c (Diff revision 2)
 
 
It's a little confusing that we are defining the "nextboot" to mean the "dataset" string (for zpool_get_nextboot) and the "zfs:dataset:" string (for zfs_ioc_get_nextboot).  Would it be possible to use the same string all the way, or to use different names for these different strings?
usr/src/uts/common/fs/zfs/vdev_label.c (Diff revision 2)
 
 
 

The semantic here is that we return a random pad2, unless one of them is a zero-length string, in which case we return the zero-length string.

There's no checksum of the pad2, right? So if one of them is damaged such that it becomes a zero-length string, the overall property is lost.

What's the motivation behind this behavior?

usr/src/uts/common/fs/zfs/vdev_label.c (Diff revision 2)
 
 
 
 
 

This seems very specific to treating pad2 as a string. I.e. we aren't really checking if it's "zeroed", only that it's a zero-length string (subsequent bytes may be nonzero). Can we document that somewhere?

Also, I think this is equivalent to:

if (pad2[0] == '\0')
  *cbp[0] = '\0';
usr/src/uts/common/fs/zfs/vdev_label.c (Diff revision 2)
 
 

do we also want to check vdev_writeable()?

usr/src/uts/common/fs/zfs/vdev_label.c (Diff revision 2)
 
 

I guess there's no performance concern with writing each vdev one at a time? If there are a lot of disks, this could be slow, but presumably this is typically used only on the root pool which has a small number of disks?

usr/src/uts/common/fs/zfs/zfs_ioctl.c (Diff revision 2)
 
 

Why is this needed? It looks like we are modifying the pad2 area of the labels, and not modifying anything referenced by the uberblock.

usr/src/uts/common/fs/zfs/zfs_ioctl.c (Diff revision 2)
 
 

Are we intentionally allowing this on suspended / readonly pools? That seems dangerous.

We may also want to pass allow_log=B_TRUE. Even if not used from the CLI, this will cause the ioctl to log itself.

usr/src/uts/common/fs/zfs/zfs_ioctl.c (Diff revision 2)
 
 

What would be the downside of using zfs_secpolicy_read here?

usr/src/uts/common/fs/zfs/zfs_ioctl.c (Diff revision 2)
 
 

What would be the downside of using zfs_secpolicy_read here?

Loading...