Add org.openzfs:raidz_expansion to read-whitelist in ZFS bootloader

[cracauer]

Summary

Here is the situation. ZFS in 15-PRERELEASE has feature org.openzfs:raidz_expansion. By default a new ZFS pool is created with all supported features on.

However, the ZFS bootloader doesn't know about it. So if somebody does a new install or a new root filesystem booting from ZFS they are unable to boot.

Now, adding this is the easy part. What I need ZFS specialists to say whether org.openzfs:raidz_expansion is indeed transparent for file read purposes. I imagine it could get complicated when e.g. an expand is in progress and interrupted by a reboot.

Test Plan

Will go through the motions of testing this in practice from a new install. I don't have multi-disk arrays that I could use for rootand expand for this test, though.

[imp]

Linux has a policy of creating root partitions with a tiny subset of the capabilities in it. We should look into updating the installer to do that, but I'm pretty sure it wouldn't have big uptake.

But this change is needed, go ahead.

[cracauer]

In D52174#1191934, @imp wrote:

Linux has a policy of creating root partitions with a tiny subset of the capabilities in it. We should look into updating the installer to do that, but I'm pretty sure it wouldn't have big uptake.

But this change is needed, go ahead.

We remind ZFS users all the time to upgrade their pools (which I don't like). Users would re-introduce excluded features easily.

I am waiting for some ZFS specialist to voice an opinion on whether raidz_expansion is indeed read-neutral.

[kevans]

In D52174#1191937, @cracauer wrote:

In D52174#1191934, @imp wrote:

Linux has a policy of creating root partitions with a tiny subset of the capabilities in it. We should look into updating the installer to do that, but I'm pretty sure it wouldn't have big uptake.

But this change is needed, go ahead.

We remind ZFS users all the time to upgrade their pools (which I don't like). Users would re-introduce excluded features easily.

I am waiting for some ZFS specialist to voice an opinion on whether raidz_expansion is indeed read-neutral.

I've piped the review to some folks that could comment more on the kind of changes involved to be sure we're not accidentally setting a landmine. The end result is probably that we go ahead with it, but we may want to drop a comment first if actually doing any expansion would break loader.

[imp]

In D52174#1191939, @kevans wrote:

In D52174#1191937, @cracauer wrote:

In D52174#1191934, @imp wrote:

Linux has a policy of creating root partitions with a tiny subset of the capabilities in it. We should look into updating the installer to do that, but I'm pretty sure it wouldn't have big uptake.

But this change is needed, go ahead.

We remind ZFS users all the time to upgrade their pools (which I don't like). Users would re-introduce excluded features easily.

I am waiting for some ZFS specialist to voice an opinion on whether raidz_expansion is indeed read-neutral.

I've piped the review to some folks that could comment more on the kind of changes involved to be sure we're not accidentally setting a landmine. The end result is probably that we go ahead with it, but we may want to drop a comment first if actually doing any expansion would break loader.

Yea, we need to ask @mm to include entries in UPDATING when new properties are added. They always hit us out of the blue, it seems. I know not all new properties are a problem, but this one affects how the pools are created, so it's on the 'incompatible' list...

[imp]

And thanks! I'm glad we caught this early in the release cycle.

[kevans]

We chatted a bit and I was pointed to the theory statement in module/zfs/vdev_raidz.c (line ~148). My inclination is that we should actually disable this in the installer-created root pools and warn about it in documentation somewhere. Our read support would Just Work(TM) pre-expansion, but as @cracauer had already suspected there's a period of time in the middle of an expansion where you need to re-math for blocks that haven't been reflowed yet.

The "Time Dependent Geometry" segment of the theory would also lead one to believe that we're going to have a bad time in a post-expansion world, as we now have a variable stripe width afterwards rather than fixed based on the # vdevs.

I think this feature's a major footgun for the root pool at the moment, and I wouldn't really want to gamble here.

[cracauer]

zfeature_common.c:759 says that this feature does not fall into the category of readonly-compatible features. That's the list that determines whether a lack of this feature in the running kernel would still allow a readonly mount.

We could also forbid actually expanding a raidz when it is a root filesystem.

[cracauer]

Another can of worms is what I often do, which is prepare a new FreeBSD installation by connecting a new disk as a second disk to an existing install and prepare a root filesystem on the commandline. It will be very hard to prevent me from footshooting in that situation.

[kevans]

In D52174#1191950, @cracauer wrote:

zfeature_common.c:759 says that this feature does not fall into the category of readonly-compatible features. That's the list that determines whether a lack of this feature in the running kernel would still allow a readonly mount.

We could also forbid actually expanding a raidz when it is a root filesystem

I don't think I'm really confident that we can actually accomplish that, and my concern is that even a slight slip here means that we let them create a situation that they effectively cannot recover from without rebuilding everything. I can't say that I'm comfortable in any way with leaving this feature enabled.

[kevans]

In D52174#1191951, @cracauer wrote:

Another can of worms is what I often do, which is prepare a new FreeBSD installation by connecting a new disk as a second disk to an existing install and prepare a root filesystem on the commandline. It will be very hard to prevent me from footshooting in that situation.

In that scenario, you're much better served by immediately finding out that loader won't accept it rather than after you've been using the system for a while and forget that it won't accept it after you decide to expand.

[cracauer]

Why is this feature set in zpools that are not raidz, anyway? That seems easy to avoid.

[cracauer]

I think the least bad thing as a first step is to allow boot with the raidz_expansion flag set if the zpool is not zraid in the first place. That covers many users.

[kevans]

In D52174#1191958, @cracauer wrote:

I think the least bad thing as a first step is to allow boot with the raidz_expansion flag set if the zpool is not zraid in the first place. That covers many users.

That adds extra complexity for a feature that I really don't agree with enabling at all on root pools in the first place, and I don't intend to participate in any review in that direction.

[cracauer]

Longer term ZFS might be changed to never set the raidz_expansion flag when the zpool is not raidz.

There is no reason to deny import from older code without the feature in that situation.

[imp]

So this is an installer bug? We should fail here?

[kevans]

In D52174#1191961, @imp wrote:

So this is an installer bug? We should fail here?

IMO yes, we should at the very least continue to fail because it sounds like we need new stripe width math for post-expansion pools. We'd need more smarts to handle a situation where an expansion is interrupted and we're booting into that state, as well, but one could argue that's maybe not as critical if you can import the pool offline and finish the expansion, I think.