- Download Diff

Summary:

7446 zpool create should support efi system partition

Review Request #219 — Created Sept. 24, 2016 and submitted May 17, 2017, 3:29 p.m.

Information
Owner:	tsoome
Repository:	illumos-gate
Branch:
Bugs:	7446
Depends On:
Commit:	9e9ea61...
Reviewers
Groups:	general
People:

Description

Testing Done

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:~#

Issues

0
0
5
6
All Issues: 11

Description	From	Last Updated
I'm concerned about the potential vagueness of the optional [size] argument. Consider: zpool create -B 3G poolname zpool create -B ...	mahrens	Oct. 2, 2016, 4:29 p.m.
Why? gcc isn't smart enough? :-)	yuripv	Dec. 13, 2016, 8:55 p.m.
can we rephrase this a bit? eg, "creating boot partition is only supported on whole disk vdevs".	yuripv	Dec. 13, 2016, 8:52 p.m.
When is boot_size something other than 0?	gwilson	Jan. 3, 2017, 3:52 p.m.
Can you simply do this if 'B' is specified?	gwilson	Jan. 3, 2017, 3:57 p.m.
This check is done in the kernel (look at vdev_is_bootable). It would be more appropriate to add code there.	gwilson	Jan. 7, 2017, 12:53 p.m.
boot_size is 0?	gwilson	Jan. 7, 2017, 12:53 p.m.
Does UEFI require the use of s0? If not, why not leave s0 alone since it's used by such tools ...	gwilson	Jan. 7, 2017, 12:54 p.m.
Out of curiosity, why would anybody want to specify the boot_size rather than have the system determine this for them? ...	gwilson	Jan. 4, 2017, 12:27 p.m.
This check seems misplaced. Haven't we already validated properties?	gwilson	Jan. 4, 2017, 12:23 p.m.
Same thing -- if we know some limitations then we should report them when we validated the property.	gwilson	Jan. 4, 2017, 12:25 p.m.

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

tsoome Sept. 29, 2016, 9: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

mahrens Sept. 29, 2016, 10:48 p.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.)

tsoome Oct. 2, 2016, 4: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.

Change Summary:

implement bootsize set-once pool property.

Diff:

Revision 2 (+334 -58)

Show changes

	usr/src/cmd/syseventd/modules/zfs_mod/zfs_mod.c
	usr/src/cmd/zpool/zpool_main.c
	usr/src/cmd/zpool/zpool_util.h
	usr/src/cmd/zpool/zpool_vdev.c
	usr/src/common/zfs/zpool_prop.c
	usr/src/lib/libzfs/common/libzfs.h
	usr/src/lib/libzfs/common/libzfs_pool.c
	usr/src/man/man1m/zpool.1m
	1 more

Change Summary:

missed few bits from update to use property.

Diff:

Revision 3 (+335 -58)

Show changes

	usr/src/cmd/syseventd/modules/zfs_mod/zfs_mod.c
	usr/src/cmd/zpool/zpool_main.c
	usr/src/cmd/zpool/zpool_util.h
	usr/src/cmd/zpool/zpool_vdev.c
	usr/src/common/zfs/zpool_prop.c
	usr/src/lib/libzfs/common/libzfs.h
	usr/src/lib/libzfs/common/libzfs_pool.c
	usr/src/man/man1m/zpool.1m
	1 more

Change Summary:

added testing.

Testing Done:

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

Change Summary:

Added the issue number.

Summary:

-	zpool: zpool create should support efi system partition
+	7446 zpool create should support efi system partition

Description:

~		zpool: zpool create should support efi system partition
	~	7446 zpool create should support efi system partition

Bugs:

+	7446

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:

+	976357861ad4abd472d48951ad7122142265643b

Diff:

Revision 4 (+341 -60)

Show changes

	usr/src/cmd/syseventd/modules/zfs_mod/zfs_mod.c
	usr/src/cmd/zpool/zpool_main.c
	usr/src/cmd/zpool/zpool_util.h
	usr/src/cmd/zpool/zpool_vdev.c
	usr/src/common/zfs/zpool_prop.c
	usr/src/lib/libzfs/common/libzfs.h
	usr/src/lib/libzfs/common/libzfs_pool.c
	usr/src/man/man1m/zpool.1m
	5 more

Change Summary:

Fix for bootsize use in calculation of expandsz. Tested with mirror and raidz.

Commit:

-	976357861ad4abd472d48951ad7122142265643b
+	db9bc7c74152b041aa60852311d09ae876c63e3e

Diff:

Revision 5 (+346 -62)

Show changes

	usr/src/cmd/syseventd/modules/zfs_mod/zfs_mod.c
	usr/src/cmd/zpool/zpool_main.c
	usr/src/cmd/zpool/zpool_util.h
	usr/src/cmd/zpool/zpool_vdev.c
	usr/src/common/zfs/zpool_prop.c
	usr/src/lib/libzfs/common/libzfs.h
	usr/src/lib/libzfs/common/libzfs_pool.c
	usr/src/man/man1m/zpool.1m
	5 more

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

tsoome Dec. 13, 2016, 9:38 p.m.

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

Testing Done:

		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:~#

Change Summary:

suggestions from Yuri + comment rewrite in libzfs - we are beyond of proof of concept now.

Commit:

-	db9bc7c74152b041aa60852311d09ae876c63e3e
+	9e9ea615f7c0aad3c54865496d42fb671ce520ca

Diff:

Revision 6 (+346 -62)

Show changes

	usr/src/cmd/syseventd/modules/zfs_mod/zfs_mod.c
	usr/src/cmd/zpool/zpool_main.c
	usr/src/cmd/zpool/zpool_util.h
	usr/src/cmd/zpool/zpool_vdev.c
	usr/src/common/zfs/zpool_prop.c
	usr/src/lib/libzfs/common/libzfs.h
	usr/src/lib/libzfs/common/libzfs_pool.c
	usr/src/man/man1m/zpool.1m
	5 more

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

Show all issues

When is boot_size something other than 0?

tsoome Jan. 3, 2017, 4: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?

tsoome Jan. 3, 2017, 4: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.

tsoome Jan. 3, 2017, 4: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?

tsoome Jan. 3, 2017, 4: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.

tsoome Jan. 3, 2017, 4: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.

tsoome Jan. 4, 2017, 12: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.

tsoome Jan. 3, 2017, 4: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?

tsoome Jan. 3, 2017, 4:24 p.m.

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

tsoome Jan. 4, 2017, 12: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.

tsoome Jan. 4, 2017, 12: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.

Ship it!

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

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

7446 zpool create should support efi system partition

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Testing Done:

Change Summary:

Summary:

Description:

Bugs:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Testing Done:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)