From eaa25f1a8e5f6ed72c2095a06df14621ed04ee6d Mon Sep 17 00:00:00 2001 From: Richard Laager Date: Tue, 18 Aug 2020 01:12:39 -0500 Subject: [PATCH] Remove GRUB restrictions The GRUB restrictions are based around the pool's bootfs property. Given the current situation where GRUB is not staying current with OpenZFS pool features, having either a non-ZFS /boot or a separate pool with limited features are pretty much the only long-term answers for GRUB support. Only the second case matters in this context. For the restrictions to be useful, the bootfs property would have to be set on the boot pool, because that is where we need the restrictions, as that is the pool that GRUB reads from. The documentation for bootfs describes it as pointing to the root pool. That's also how it's used in the initramfs. ZFS does not allow setting bootfs to point to a dataset in another pool. (If it did, it'd be difficult-to-impossible to enforce these restrictions cross-pool). Accordingly, bootfs is pretty much useless for GRUB scenarios moving forward. Even for users who have only one pool, the existing restrictions for GRUB are incomplete. They don't prevent you from enabling the unsupported checksums, for example. For that reason, I have ripped out all the GRUB restrictions. A little longer-term, I think extending the proposed features=portable system to define a features=grub is a much more useful approach. The user could set that on the boot pool at creation, and things would Just Work. Reviewed-by: Paul Dagnelie Reviewed-by: Brian Behlendorf Signed-off-by: Richard Laager Closes #8627 --- lib/libzfs/libzfs_pool.c | 20 +------------------- man/man5/zpool-features.5 | 9 --------- man/man8/zfsprops.8 | 2 -- module/zfs/spa.c | 21 +-------------------- module/zfs/zfs_ioctl.c | 39 --------------------------------------- 5 files changed, 2 insertions(+), 89 deletions(-) diff --git a/lib/libzfs/libzfs_pool.c b/lib/libzfs/libzfs_pool.c index f848cb3cfc..b75ab3ec15 100644 --- a/lib/libzfs/libzfs_pool.c +++ b/lib/libzfs/libzfs_pool.c @@ -3143,7 +3143,6 @@ zpool_vdev_attach(zpool_handle_t *zhp, const char *old_disk, uint_t children; nvlist_t *config_root; libzfs_handle_t *hdl = zhp->zpool_hdl; - boolean_t rootpool = zpool_is_bootable(zhp); if (replacing) (void) snprintf(msg, sizeof (msg), dgettext(TEXT_DOMAIN, @@ -3211,18 +3210,8 @@ zpool_vdev_attach(zpool_handle_t *zhp, const char *old_disk, zcmd_free_nvlists(&zc); - if (ret == 0) { - if (rootpool) { - /* - * XXX need a better way to prevent user from - * booting up a half-baked vdev. - */ - (void) fprintf(stderr, dgettext(TEXT_DOMAIN, "Make " - "sure to wait until resilver is done " - "before rebooting.\n")); - } + if (ret == 0) return (0); - } switch (errno) { case ENOTSUP: @@ -3652,13 +3641,6 @@ zpool_vdev_remove(zpool_handle_t *zhp, const char *path) return (zfs_error(hdl, EZFS_BADVERSION, msg)); } - if (!islog && !avail_spare && !l2cache && zpool_is_bootable(zhp)) { - zfs_error_aux(hdl, dgettext(TEXT_DOMAIN, - "root pool can not have removed devices, " - "because GRUB does not understand them")); - return (zfs_error(hdl, EINVAL, msg)); - } - zc.zc_guid = fnvlist_lookup_uint64(tgt, ZPOOL_CONFIG_GUID); if (zfs_ioctl(hdl, ZFS_IOC_VDEV_REMOVE, &zc) == 0) diff --git a/man/man5/zpool-features.5 b/man/man5/zpool-features.5 index 3f690c3340..0416afaa3f 100644 --- a/man/man5/zpool-features.5 +++ b/man/man5/zpool-features.5 @@ -340,9 +340,6 @@ can turn on the \fBedonr\fR checksum on any dataset using the and will return to being \fBenabled\fR once all filesystems that have ever had their checksum set to \fBedonr\fR are destroyed. -The \fBedonr\fR feature is not supported by GRUB and must not be used on -the pool if GRUB needs to access the pool (e.g. for /boot). - FreeBSD does not support the \fBedonr\fR feature. .RE @@ -827,9 +824,6 @@ can turn on the \fBsha512\fR checksum on any dataset using \fBactive\fR once a \fBchecksum\fR property has been set to \fBsha512\fR, and will return to being \fBenabled\fR once all filesystems that have ever had their checksum set to \fBsha512\fR are destroyed. - -The \fBsha512\fR feature is not supported by GRUB and must not be used on -the pool if GRUB needs to access the pool (e.g. for /boot). .RE .sp @@ -861,9 +855,6 @@ can turn on the \fBskein\fR checksum on any dataset using \fBactive\fR once a \fBchecksum\fR property has been set to \fBskein\fR, and will return to being \fBenabled\fR once all filesystems that have ever had their checksum set to \fBskein\fR are destroyed. - -The \fBskein\fR feature is not supported by GRUB and must not be used on -the pool if GRUB needs to access the pool (e.g. for /boot). .RE .sp diff --git a/man/man8/zfsprops.8 b/man/man8/zfsprops.8 index 5a2b45e64f..1fcb07c6ff 100644 --- a/man/man8/zfsprops.8 +++ b/man/man8/zfsprops.8 @@ -762,8 +762,6 @@ The and .Sy edonr checksum algorithms require enabling the appropriate features on the pool. -These pool features are not supported by GRUB and must not be used on the -pool if GRUB needs to access the pool (e.g. for /boot). FreeBSD does not support the .Sy edonr algorithm. diff --git a/module/zfs/spa.c b/module/zfs/spa.c index 79b88e9111..e358404db9 100644 --- a/module/zfs/spa.c +++ b/module/zfs/spa.c @@ -621,7 +621,6 @@ spa_prop_validate(spa_t *spa, nvlist_t *props) if (!error) { objset_t *os; - uint64_t propval; if (strval == NULL || strval[0] == '\0') { objnum = zpool_prop_default_numeric( @@ -633,27 +632,9 @@ spa_prop_validate(spa_t *spa, nvlist_t *props) if (error != 0) break; - /* - * Must be ZPL, and its property settings - * must be supported by GRUB (compression - * is not gzip, and large dnodes are not - * used). - */ - + /* Must be ZPL. */ if (dmu_objset_type(os) != DMU_OST_ZFS) { error = SET_ERROR(ENOTSUP); - } else if ((error = - dsl_prop_get_int_ds(dmu_objset_ds(os), - zfs_prop_to_name(ZFS_PROP_COMPRESSION), - &propval)) == 0 && - !BOOTFS_COMPRESS_VALID(propval)) { - error = SET_ERROR(ENOTSUP); - } else if ((error = - dsl_prop_get_int_ds(dmu_objset_ds(os), - zfs_prop_to_name(ZFS_PROP_DNODESIZE), - &propval)) == 0 && - propval != ZFS_DNSIZE_LEGACY) { - error = SET_ERROR(ENOTSUP); } else { objnum = dmu_objset_id(os); } diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c index 10b37a4ada..a58fd99ad5 100644 --- a/module/zfs/zfs_ioctl.c +++ b/module/zfs/zfs_ioctl.c @@ -300,23 +300,6 @@ history_str_get(zfs_cmd_t *zc) return (buf); } -/* - * Check to see if the named dataset is currently defined as bootable - */ -static boolean_t -zfs_is_bootfs(const char *name) -{ - objset_t *os; - - if (dmu_objset_hold(name, FTAG, &os) == 0) { - boolean_t ret; - ret = (dmu_objset_id(os) == spa_bootfs(dmu_objset_spa(os))); - dmu_objset_rele(os, FTAG); - return (ret); - } - return (B_FALSE); -} - /* * Return non-zero if the spa version is less than requested version. */ @@ -4478,18 +4461,6 @@ zfs_check_settable(const char *dsname, nvpair_t *pair, cred_t *cr) } spa_close(spa, FTAG); } - - /* - * If this is a bootable dataset then - * verify that the compression algorithm - * is supported for booting. We must return - * something other than ENOTSUP since it - * implies a downrev pool version. - */ - if (zfs_is_bootfs(dsname) && - !BOOTFS_COMPRESS_VALID(intval)) { - return (SET_ERROR(ERANGE)); - } } break; @@ -4531,16 +4502,6 @@ zfs_check_settable(const char *dsname, nvpair_t *pair, cred_t *cr) intval != ZFS_DNSIZE_LEGACY) { spa_t *spa; - /* - * If this is a bootable dataset then - * we don't allow large (>512B) dnodes, - * because GRUB doesn't support them. - */ - if (zfs_is_bootfs(dsname) && - intval != ZFS_DNSIZE_LEGACY) { - return (SET_ERROR(EDOM)); - } - if ((err = spa_open(dsname, &spa, FTAG)) != 0) return (err);