Add all read-only compatible features to GRUB2 compatibility feature set

[Luflosi]

Motivation and Context

Since GRUB2 opens pools in read-only mode, all read-only compatible features can be enabled.

Description

Several read-only compatible features have been introduced since the GRUB2 compatibility file was created but the file wasn't updated.

I added all features from zpool_feature_init() in module/zcommon/zfeature_common.c where the
ZFEATURE_FLAG_READONLY_COMPAT flag is set.

This is my first PR here, so please excuse any mistakes I may have made.

How Has This Been Tested?

I created a ZFS pool with -o compatibility=grub2 used it as /boot and successfully booted from the pool using GRUB2. To make this easy and reproducible, I wrote a NixOS VM test, which I plan to upstream to Nixpkgs. I'll happily provide more details about this if requested.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[rincebrain]

I don't actually think you can do that.

I haven't personally tested this, but I know there were reports at least at one point that some of the read-only compatible features caused grub to burp and error out unexpectedly anyway, so I'd test that that's no longer true on pools with them all active before adding them.

[Luflosi]

Are you sure? I just enabled all these features except block_cloning, zilsaxattr and zpool_checkpoint on the pool storing /boot on one of my servers and it rebooted just fine. I can't enable the other three features because they are not yet supported by a released version of ZFS and I don't want to run ZFS from master in production.
I also wrote the VM test as written above, so unless I did something very wrong there, I expect this to work.

Or do the features actually need to be active instead of just enabled to notice a failure?
This is the relevant output of zpool get all bpool on the server I mentioned above:

bpool  compatibility                  grub2                          local
bpool  feature@async_destroy          enabled                        local
bpool  feature@empty_bpobj            enabled                        local
bpool  feature@lz4_compress           active                         local
bpool  feature@multi_vdev_crash_dump  disabled                       local
bpool  feature@spacemap_histogram     active                         local
bpool  feature@enabled_txg            active                         local
bpool  feature@hole_birth             active                         local
bpool  feature@extensible_dataset     active                         local
bpool  feature@embedded_data          active                         local
bpool  feature@bookmarks              enabled                        local
bpool  feature@filesystem_limits      enabled                        local
bpool  feature@large_blocks           enabled                        local
bpool  feature@large_dnode            disabled                       local
bpool  feature@sha512                 disabled                       local
bpool  feature@skein                  disabled                       local
bpool  feature@edonr                  disabled                       local
bpool  feature@userobj_accounting     enabled                        local
bpool  feature@encryption             disabled                       local
bpool  feature@project_quota          active                         local
bpool  feature@device_removal         enabled                        local
bpool  feature@obsolete_counts        enabled                        local
bpool  feature@zpool_checkpoint       disabled                       local
bpool  feature@spacemap_v2            active                         local
bpool  feature@allocation_classes     enabled                        local
bpool  feature@resilver_defer         enabled                        local
bpool  feature@bookmark_v2            disabled                       local
bpool  feature@redaction_bookmarks    disabled                       local
bpool  feature@redacted_datasets      disabled                       local
bpool  feature@bookmark_written       disabled                       local
bpool  feature@log_spacemap           active                         local
bpool  feature@livelist               enabled                        local
bpool  feature@device_rebuild         enabled                        local
bpool  feature@zstd_compress          disabled                       local
bpool  feature@draid                  disabled                       local

[rincebrain]

enabled just means "we're allowed to be using it, but we're not, yet", while active means "we actually wrote things with this incompatibility on the pool", and I think the latter was what made grub grumpy.

I'd be perfectly happy if I'm wrong and this is not an issue, but I seem to recall reports of GRUB complaining about one or two read-only features it shouldn't mind showing up, so I'd like to confirm that's not a thing that comes up somewhat before saying I think this is a fine plan.

(I'll go try to dig out more concrete information later, so this isn't just "some guy I met once said it broke".)

[gmelikov]

You should test grub-probe /boot too, there are some cases when grub will indeed boot from such pools, but it won't probe it, and some features with enabled support may break boot when they become active.

Overall, I'd be happy if they are truly work, but we should test full cycle.

And interesting point that some grubs (at least in opensuse) may have more problems with new features, unfortunately I didn't have time to bisect their source for a reason.

[Luflosi]

I accidentally added the read-only compatible feature obsolete_counts, which has a dependency on the device_removal feature, which is not read-only compatible and not supported by GRUB2. I removed it now.

I'm trying to get all the features to be "active" instead of "enabled" in the VM test. I don't know how to make the features zilsaxattr and block_cloning active. Could you try to give me some hints please?

[rincebrain]

block_cloning would require freebsd or unmerged code to get to flip to active on Linux, as the Linux interfaces for it don't exist yet in tree. Once they do, you could do something like cp --reflink foo bar to do the thing.

It looks like, to get zilsaxattr active, you'd need to commit xattr changes to a ZIL and have them pending - e.g. I think you'd need to do something like snapshot the dataset while they're pending, unmount it, then roll it back so it has to play them back next mount.

[Luflosi]

My VM test only works with Linux. Is the unmerged code to enable cp --reflink on Linux available as a PR or otherwise? I couldn't find anything with a quick GitHub search.

I think you'd need to do something like snapshot the dataset while they're pending, unmount it, then roll it back so it has to play them back next mount.

That sounds prone to race conditions. Is there a way that I can test this in a reliable way?

On a similar note, is there a reliable way I can test the device_rebuild and resilver_defer features?

I think I have activated all other newly added features except allocation_classes, where I'm currently debugging why Linux doesn't like the pool after a reboot.

[rincebrain]

I mean, the feature is literally only active when there's pending ZIL data on a dataset, so I think the best you'd get would be plumbing a knob somewhere to force it to not flush in a timely fashion to make the window deterministic, unless someone already has tooling for it I'm not aware of.

device_rebuild is active if zpool replace -s or zpool attach -s is running, resilver_defer is active if you do something that would require a resilver to trigger while one is already running, e.g. another zpool attach/replace.

e: I have a branch I was playing with it in that should suffice for just getting the feature activated, but it's just an experiment, there are known explosions if you touch complicated things, etc. So just using it to generate a test pool is probably best.

[Luflosi]

I ran the latest ZFS master version aebd94c with rincebrain@fd21c46 applied as a patch.
This is what the main part of my VM testing code currently looks like:

"flock /dev/vda parted --script /dev/vda -- mklabel msdos"
+ " mkpart primary ext2 1M 256MB"          # /boot
+ " mkpart primary ext2 256MB 512MB"       # /boot special metadata device
+ " mkpart primary linux-swap 512MB 1536M"
+ " mkpart primary ext2 1536M -1s",        # /
"udevadm settle",

"mkswap /dev/vda3 -L swap",
"swapon -L swap",

"mkfs.ext3 -L nixos /dev/vda4",
"mount LABEL=nixos /mnt",

# Use as many ZFS features as possible to verify that GRUB can handle them
"zpool create -o compatibility=grub2 -O utf8only=on -O normalization=formD bpool /dev/vda1",
"zpool add bpool -o ashift=12 special /dev/vda2",
"zfs create -o recordsize=1M -o mountpoint=legacy -o compression=lz4 -o xattr=sa -o acltype=posixacl -o relatime=on bpool/boot",

# Snapshotting the top-level dataset would trigger a bug in GRUB2: https://github.com/openzfs/zfs/issues/13873
"zfs snapshot bpool/boot@snap-1",
"zfs clone bpool/boot@snap-1 bpool/test",
"zpool checkpoint bpool",
"mkdir -p /mnt/boot",
"mount -t zfs bpool/boot /mnt/boot",
"touch /mnt/boot/test",
"setfattr -n user.comment -v 'this is a comment' /mnt/boot/test",
"cp --reflink=always /mnt/boot/test /mnt/boot/test2",
"sleep 10", # TODO: remove
"zpool status >&2",

# Print out all enabled and active ZFS features (and some other stuff)
"zpool get all bpool >&2",

# Abort early if GRUB2 doesn't like the disks
"grub-probe --target=device /mnt/boot >&2",

This is python code with strings containing bash code.

cp --reflink=always /mnt/boot/test /mnt/boot/test2 above doesn't seem to change the block_cloning feature from "enabled" to "active". Did I do anything incorrectly?
When the copy is happening, the kernel prints out some debug messages:

machine # [    9.612312] file_in ffff9713859c6000 pos_in 0 file_out ffff9713859c7100 pos_out 0 len 0 remap_flags 0
machine # [    9.612318]
machine # [    9.612318] BEFORE file_in ffff9713871739b0 pos_in 0 file_out ffff971393842c60 pos_out 0 len 0
machine # [    9.613934]
machine # [    9.613934] AFTER file_in ffff9713871739b0 pos_in 0 file_out ffff971393842c60 pos_out 0 len 0 err 0

[rincebrain]

I wouldn't be astonished if I screwed up activating the feature, though I would have expected the clone calls to do it. Curious.

Yeah, there's debug prints, it's very much a debug toy build.

You could probably use zhack feature commands to set it to active instead.

[Luflosi]

I have never used the zhack command before. From reading the man page, it seems like it can only "enable" features, not activate them. I tried executing zhack feature enable bpool com.fudosecurity:block_cloning but got the error message zhack: 'com.fudosecurity:block_cloning' is a real feature, will not enable.
Can zhack not be used in the way you suggested?

[xnox]

In ubuntu we have to patch tooling to prevent accidental upgrade of the "bpool" which are trying to keep grub compatible.

It would be nice if we could have a pool featureflag for grub compat, and thus prevent pool upgrades and/or enabling features outside of the grub-compatible one.

Similarly testing should be done in against grub's supported available and active features.

[behlendorf]

Closing this was addressed by #15459 and #15499.