7446 zpool create should support efi system partition

Review Request #219 — Created Sept. 24, 2016 and submitted

tsoome
illumos-gate
7446
9e9ea61...
general

7446 zpool create should support efi system partition

root@test:~# zpool create -B tank c0t1d0s0
create boot partition can only be used with wholedisk: c0t1d0s0
root@test:~# zpool create -B -o bootsize= tank c0t1d0s0
bad boot partition size '': bad numeric value ''
root@test:~# zpool create -B -o bootsize=asd tank c0t1d0s0
bad boot partition size 'asd': bad numeric value 'asd'
root@test:~# zpool create -B -o bootsize=a1 tank c0t1d0s0
bad boot partition size 'a1': bad numeric value 'a1'
root@test:~# zpool create -B -o bootsize=1a tank c0t1d0s0
bad boot partition size '1a': invalid numeric suffix 'a'
root@test:~# zpool create -B -o bootsize=1m tank c0t1d0s0
create boot partition can only be used with wholedisk: c0t1d0s0
root@test:~# zpool create -B -o bootsize=1m tank c0t1d0
Warning: EFI System partition size 1M is not allowing to create FAT32 file
system, which may result in unbootable system.
root@test:~#

root@test:~# zpool get bootsize tank
NAME PROPERTY VALUE SOURCE
tank bootsize 1M local
root@test:~#

root@test:~# zpool set bootsize=2M tank
cannot set property for 'tank': property 'bootsize' can only be set during pool creation
root@test:~#

root@test:~# zpool destroy tank
root@test:~# zpool create tank c0t1d0
root@test:~# zpool get bootsize tank
NAME PROPERTY VALUE SOURCE
tank bootsize - default
root@test:~#

root@test:~# zpool destroy tank
root@test:~# zpool create -B tank c0t1d0
root@test:~# zpool get bootsize tank
NAME PROPERTY VALUE SOURCE
tank bootsize 256M local
root@test:~#

format> ver

Volume name = < >
ascii name = <lofi-test-1.0-2.00GB>
bytes/sector = 512
sectors = 4194303
accessible sectors = 4194270
Part Tag Flag First Sector Size Last Sector
0 system wm 256 256.00MB 524543
1 usr wm 524544 1.74GB 4177886
2 unassigned wm 0 0 0
3 unassigned wm 0 0 0
4 unassigned wm 0 0 0
5 unassigned wm 0 0 0
6 unassigned wm 0 0 0
8 reserved wm 4177887 8.00MB 4194270

format>

sample setup with mirror:
tsoome@uefi-oi:~$ zpool get bootsize
NAME PROPERTY VALUE SOURCE
rpool bootsize 256M local
tsoome@uefi-oi:~$ zpool list
NAME SIZE ALLOC FREE EXPANDSZ FRAG CAP DEDUP HEALTH ALTROOT
rpool 15,6G 5,93G 9,70G - 26% 37% 1.00x ONLINE -
tsoome@uefi-oi:~$ zpool status
pool: rpool
state: ONLINE
scan: resilvered 4,51G in 0h1m with 0 errors on Tue Nov 22 23:19:50 2016
config:

    NAME        STATE     READ WRITE CKSUM
    rpool       ONLINE       0     0     0
      mirror-0  ONLINE       0     0     0
        c3t0d0  ONLINE       0     0     0
        c3t1d0  ONLINE       0     0     0

errors: No known data errors
tsoome@uefi-oi:~$

and with raidz1:

root@beastie:~# zpool get bootsize rpool
NAME PROPERTY VALUE SOURCE
rpool bootsize 256M local
root@beastie:~# zpool list
NAME SIZE ALLOC FREE EXPANDSZ FRAG CAP DEDUP HEALTH ALTROOT
rpool 14,5T 60,6G 14,4T - 0% 0% 1.00x ONLINE -
root@beastie:~# zpool status
pool: rpool
state: ONLINE
scan: none requested
config:

    NAME        STATE     READ WRITE CKSUM
    rpool       ONLINE       0     0     0
      raidz1-0  ONLINE       0     0     0
        c3t0d0  ONLINE       0     0     0
        c3t1d0  ONLINE       0     0     0
        c3t3d0  ONLINE       0     0     0
        c3t4d0  ONLINE       0     0     0

errors: No known data errors
root@beastie:~#

  • 0
  • 0
  • 5
  • 6
  • 11
Description From Last Updated
mahrens
  1. 
      
  2. usr/src/cmd/zpool/zpool_main.c (Diff revision 1)
     
     
     

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

  3. 
      
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
yuripv
  1. 
      
  2. usr/src/cmd/zpool/zpool_vdev.c (Diff revision 5)
     
     

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

  3. 
      
yuripv
  1. 
      
  2. usr/src/cmd/zpool/zpool_main.c (Diff revision 5)
     
     

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

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

  3. 
      
tsoome
tsoome
gwilson
  1. 
      
  2. usr/src/cmd/zpool/zpool_main.c (Diff revision 6)
     
     

    When is boot_size something other than 0?

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

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

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

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

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

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

    1. 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?

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

    boot_size is 0?

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

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

    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. 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. btw, I havent inspected the zdb code from this angle, but from basic zdb usage, I havent seen issues (so far anyhow).

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

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

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

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

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

    2. 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:)

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

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

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

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

  2. 
      
yuripv
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...