7446 zpool create should support efi system partition
Review Request #219 - Created Sept. 24, 2016 and submitted
Information | |
---|---|
Toomas Soome | |
illumos-gate | |
7446 | |
9e9ea61... | |
Reviewers | |
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 4194270format>
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 0errors: 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 0errors: No known data errors
root@beastie:~#
-
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_poolI 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".
Change Summary:
added testing.
Testing Done: |
|
---|
Change Summary:
Added the issue number.
Summary: |
|
||||||
---|---|---|---|---|---|---|---|
Description: |
|
||||||
Bugs: |
|
Change Summary:
Adjust vdev statistics vs->vs_esize and metaslab_class_expandable_space() by bootsize, so the expandable size is reported correct. As at least stats is read quite a lot, cache bootsize in spa.
Commit: |
|
||
---|---|---|---|
Diff: |
Revision 4 (+341 -60) |
Change Summary:
Fix for bootsize use in calculation of expandsz. Tested with mirror and raidz.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+346 -62) |
-
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".
-
usr/src/cmd/zpool/zpool_main.c (Diff revision 5) -
Why? gcc isn't smart enough? :-)
Testing Done: |
|
---|
Change Summary:
suggestions from Yuri + comment rewrite in libzfs - we are beyond of proof of concept now.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+346 -62) |
-
usr/src/cmd/zpool/zpool_main.c (Diff revision 6) -
When is boot_size something other than 0?
-
usr/src/cmd/zpool/zpool_main.c (Diff revision 6) -
Can you simply do this if 'B' is specified?
-
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.
-
usr/src/cmd/zpool/zpool_vdev.c (Diff revision 6) -
boot_size is 0?
-
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. -
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.
-
usr/src/lib/libzfs/common/libzfs_pool.c (Diff revision 6) -
This check seems misplaced. Haven't we already validated properties?
-
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.
I know how much of a pain setting up an EFI parition layout is. LGTM.
Ship It!