usr/src/cmd/zpool/zpool_main.c (Diff revision 1)

Show all issues

I'm concerned about the potential vagueness of the optional [size] argument.  Consider:
zpool create -B 3G poolname

zpool create -B 3_is_the_name_of_my_pool
I would also ask if we could use a "-o property=value" to specify this, rather than a top-level option.  Even if the new "property" is not stored/used after creating the pool, using this as an interface for setting it seems preferable to me.
It might also be nice to make the bootsize show up as a read-only property, which integrates nicely with setting it with "-o".

1. 

   Toomas Soome Sept. 29, 2016, 11:36 p.m.

   hm. The property by itself does sound pretty good. Im not entirely sure about read only status, but then again, this is for whole disk setup, anything other is about manual disk management anyhow. I need to study those interfaces about properties - both for passing with zpool and how to create read only property in pool:D

2. 

   Matthew Ahrens Sept. 30, 2016, 12:48 a.m.

   To clarify - I meant to say "set-once property", i.e. which can be set at pool creation time, and is readonly after that.  (Since my understanding is that the boot size must be specified at pool creation time and can't be changed after that.)

3. 

   Toomas Soome Oct. 2, 2016, 6:29 p.m.

   ok, here we go. I quite like the result:) There is one side defect (not related to property itself) - the current expandsize calculation is assuming  the whole disk, but should be adjusted by boot partition size. I doubt we can access bootsize pool property in vdev_disk_open(), probably should use  the partition table instead... However, I think the expandsize is best to be dealt as separate issue.

usr/src/cmd/zpool/zpool_vdev.c (Diff revision 5)

Show all issues

can we rephrase this a bit? eg, "creating boot partition is only supported on whole disk vdevs".

usr/src/cmd/zpool/zpool_main.c (Diff revision 5)

Show all issues

Why? gcc isn't smart enough? :-)

1. 

   Toomas Soome Dec. 13, 2016, 10:38 p.m.

   I think it is leftover from pre-bootsize size argument implementation.

usr/src/cmd/zpool/zpool_main.c (Diff revision 6)

Show all issues

When is boot_size something other than 0?

1. 

   Toomas Soome Jan. 3, 2017, 5:24 p.m.

   user can specify custom size with setting property as -o bootsize=value

usr/src/cmd/zpool/zpool_main.c (Diff revision 6)

Show all issues

Can you simply do this if 'B' is specified?

1. 

   Toomas Soome Jan. 3, 2017, 5:24 p.m.

   you mean in while loop, processing the options? We do not have strict order for -B and -o bootsize=value. So its is easier to set things right after loop is done.

usr/src/cmd/zpool/zpool_vdev.c (Diff revision 6)

Show all issues

This check is done in the kernel (look at vdev_is_bootable). It would be more appropriate to add code there.

1. 

   Toomas Soome Jan. 3, 2017, 5:24 p.m.

   The kernel check is there anyhow, but we can do simple test already in zpool and avoid call to kernel in first place... also the error checking and messaging is more simple while done early, don't you agree?

usr/src/cmd/zpool/zpool_vdev.c (Diff revision 6)

Show all issues

boot_size is 0?

1. 

   Toomas Soome Jan. 3, 2017, 5:24 p.m.

   on split we copy the existing label; so the boot_size is only meaningful when we get to call zpool_label_disk() while we are creating new pool.

usr/src/lib/libzfs/common/libzfs_pool.c (Diff revision 6)

Show all issues

Does UEFI require the use of s0? If not, why not leave s0 alone since it's used by such tools as zdb to inspect the pool labels. It would make more sense to use s6, for example.

1. 

   Toomas Soome Jan. 3, 2017, 5:24 p.m.

   And would mess up the partition name order versus allocated space order. IMO this is not good. if zdb is really just blindly assuming s0, it has to be fixed, because s0 is really not special in any way for zfs, it just happen to be used in case of whole disk pool, with partition setup, you can not assume s0 anyhow.

2. 

   Toomas Soome Jan. 4, 2017, 1:51 p.m.

   btw, I havent inspected the zdb code from this angle, but from basic zdb usage, I havent seen issues (so far anyhow).

usr/src/lib/libzfs/common/libzfs_pool.c (Diff revision 6)

Show all issues

Out of curiosity, why would anybody want to specify the boot_size rather than have the system determine this for them? For example, why have a 10GB boot partition vs a 256MB one? It seems like we should just pick a default and simplify this process significantly.

1. 

   Toomas Soome Jan. 3, 2017, 5:24 p.m.

   well, I would not expect that much about 10G partition, but much smaller one than 256MB:) the actual boot blocks are:
   -r-xr-xr-x   1 root     sys       169984 jaan  1 00:23 /boot/bootia32.efi
   
   -r-xr-xr-x   1 root     sys       147968 jaan  1 00:23 /boot/bootx64.efi
   However, in some cases you want to have extra space to deploy firmware updates - vendors do provide firmware updates in form of efi application binaries. + the compatibility issues with firmware and different FAT types.

usr/src/lib/libzfs/common/libzfs_pool.c (Diff revision 6)

Show all issues

This check seems misplaced. Haven't we already validated properties?

1. 

   Toomas Soome Jan. 3, 2017, 5:24 p.m.

   hm, thats actually good point, have to look on it, thanks!

2. 

   Toomas Soome Jan. 4, 2017, 1:27 p.m.

   After thinking a bit, I did reach to conclusion that there is no way to give up the checks. And the reason is simple - we can not trust the values are already tested, we can not even assume they are tested, and so we need to validate our input data. The property validation in this context is totally separated and there is no evidence the validation actually has taken place at all - this function does not see any properties. So I'll just drop this and next issue:)

usr/src/lib/libzfs/common/libzfs_pool.c (Diff revision 6)

Show all issues

Same thing -- if we know some limitations then we should report them when we validated the property.

1. 

   Toomas Soome Jan. 4, 2017, 1:27 p.m.

   Well, this would be valid note if this function would receive the list of properties and we would call the property validation procedure. Right now the arguments for this function may or may not have anything to do about properties, there is no way to know.

I know how much of a pain setting up an EFI parition layout is.  LGTM.

Ship It!