bsdinstall: use gpt label instead of swap partition name when updating fstab.

[wanpengqian_gmail.com]

Summary

When partition scheme is GPT*, bsdinstall still use partition name instead of gpt label for swap partition when updating fstab.
this patch will use gpt label instead partition name.

A useful scenario is, after the system is installed, if users add more disks or change the disk sequence, this behavior will make zroot disk name change. if fstab uses the fixed partition name, the user should update fstab too. while using the gpt label. it is always correct.

Test Plan

Install and check fstab.
also check if system use gpt label using swapinfo command.

[wanpengqian_gmail.com]

Any chance to get some notice? everytime I install a new machine, I have to modify fstab manually to avoid this issue.

[wanpengqian_gmail.com]

remove comments

[chuck]

The code changes do not match the title or summary. Did you update the wrong review?

[wanpengqian_gmail.com]

Rollback to previous diff.

[wanpengqian_gmail.com]

In D32936#826849, @chuck wrote:

The code changes do not match the title or summary. Did you update the wrong review?

I am so sorry, I used the wrong number while updating the patch.
It has been corrected.

[wanpengqian_gmail.com]

@kevans I am sorry I just picked up the latest commiter from bsdinstall history,
maybe you can take a look for this patch. I have been suffering from this issue for many years.

[kevans]

I think this generally looks good, though I have a minor suggestion for you to consider...

usr.sbin/bsdinstall/scripts/zfsboot

1078–1089

It looks like we can use the gpt label for encrypted swap, too; how would you feel about absorbing the $ZFSBOOT_SWAP_ENCRYPTION branch just above with something like:

suffix=""
if [ "$ZFSBOOT_SWAP_ENCRYPTION" ]; then
    suffix=".eli"
fi

# Use $suffix in both of the below cases

[kevans]

usr.sbin/bsdinstall/scripts/zfsboot

1078–1089

(well, local suffix="")

[wanpengqian_gmail.com]

Update as reviewer suggested.

[wanpengqian_gmail.com]

usr.sbin/bsdinstall/scripts/zfsboot

1078–1089

Yeah, I follow your suggestion, it seems we can simplify the logic here.
Here is the new patch.

I still need to build a test iso and for testing. Maybe this is what you mean?

[allanjude]

The reason we stopped doing this originally, is that if ZFS imports the pool using disk names like /dev/ada0p3, then the GPT label version like /dev/gpt/swap went away, and then you ended up with no swap mounted.
I don't know if things have changed somewhat since when I ran into these problems around 2015. I know there was some work to try to mitigate the issue.

[kevans]

In D32936#841641, @allanjude wrote:

The reason we stopped doing this originally, is that if ZFS imports the pool using disk names like /dev/ada0p3, then the GPT label version like /dev/gpt/swap went away, and then you ended up with no swap mounted.
I don't know if things have changed somewhat since when I ran into these problems around 2015. I know there was some work to try to mitigate the issue.

On a recent-ish -CURRENT box, ZFS importing the pool only wipes out the /dev/gpt node for that partition but not the label for other partitions on the disk. Same for, e.g., swapping on /dev/ada0p3 (wipes out /dev/gpt/swap0 while in use as a swap device).

usr.sbin/bsdinstall/scripts/zfsboot

1078–1089

Yeah, this looks reasonable (though, is GELI + MIRROR a valid and functional configuration?), thanks!

[wanpengqian_gmail.com]

I have built a test ISO and it works as expected within the BHYVE virtual machine with following conditions

GPT* schema	1 Disk	2 Disks	3 Disks
normal	OK	OK	OK
mirror	OK	OK	OK
encrypt	OK	OK	OK
mirror + encrypt	OK	OK	OK

I still need to check MBR condition combinations. and more tests on real machines.

usr.sbin/bsdinstall/scripts/zfsboot

1078–1089

Yes, GELI + MIRROR is a valid and functional configuration. I remove following code

	elif [ "$ZFSBOOT_SWAP_ENCRYPTION" -a "$ZFSBOOT_SWAP_MIRROR" ]; then
		f_eval_catch $funcname printf "$PRINTF_FSTAB" \
		             /dev/mirror/swap.eli none swap sw 0 0 \
		             $BSDINSTALL_TMPETC/fstab || return $FAILURE
		isswapmirror=1

because this condition will process here

	elif [ "$ZFSBOOT_SWAP_MIRROR" ]; then
		f_eval_catch $funcname printf "$PRINTF_FSTAB" \
		             /dev/mirror/swap$suffix none swap sw 0 0 \
		             $BSDINSTALL_TMPETC/fstab || return $FAILURE
		isswapmirror=1
	else

since we have set the suffix before

[kevans]

Awesome, thanks! IMO you've done sufficient testing to rule out obvious harm; MBR gets caught in the wildcard and uses the exact same /etc/fstab contents as it did before, so I think we can safely say this is good. I'll give it a ~day or two for someone to object or bring up some scenario we hadn't considered before landing it.

usr.sbin/bsdinstall/scripts/zfsboot

1078–1089

Oops, indeed, I had missed that removed branch. Excellent!

[kevans]

In D32936#841641, @allanjude wrote:

The reason we stopped doing this originally, is that if ZFS imports the pool using disk names like /dev/ada0p3, then the GPT label version like /dev/gpt/swap went away, and then you ended up with no swap mounted.
I don't know if things have changed somewhat since when I ran into these problems around 2015. I know there was some work to try to mitigate the issue.

I think @dim might have, in parallel, stumbled upon this problem and provided some pointers that suggest it may be a non-issue on more recent installs. A redacted quote:

05:56 <@dim> [...] ah when I do a 14-CURRENT snapshot auto-ZFS install on a fresh VM I see that 
             it adds kern.geom.label.disk_ident.enable=0 and kern.geom.label.gptid.enable=0 to 
             /boot/loader.conf
05:56 <@dim> [...] i.e. that might force root-on-zfs to use the traditional /dev/diskXpY 
             devices instead of /dev/diskid
05:57 <@dim> [....] and in this case, when it's booted, it shows geom device da0 just fine, plus 
             /dev/gpt labels

I wonder if your memory is from mid-2014 or so (or installs dating to that time), and perhaps fixed along with a patch for adding encrypted/mirrored swap: rG2875e59f52f95d59ab7bf94ea67a98292f4f7775

The commit message doesn't really expand on why kern.geom.label.gptid.enable=0 was set, unfortunately, but it seems like a reasonable conclusion.