Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add compatibility file for GRUB versions up to v2.06 #15909

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

usaleem-ix
Copy link
Contributor

@usaleem-ix usaleem-ix commented Feb 19, 2024

Motivation and Context

GRUB is not able to detect ZFS pool if snaphsot of top level boot
pool dataset is created. This issue is observed with GRUB versions
up to v2.06 if extensible_dataset feature is enabled on ZFS boot pool.

#15261
#13873

Description

A new compatibility file is added grub2-2.06 for GRUB versions
upto 2.06. compatibility=grub2-2.06 would enable all read-only
compatible zpool features except extensible_dataset and other
features that depend on it.

The existing grub2 compatibility file lists all read-only features
that can be enabled on boot pool for grub with version 2.12
onwards. The issue is fixed on GRUB v2.12.

Documented both grub2 and grub2-2.06 files in zpool-features.7.

livelist feature also depends on extensible_dataset, but it was
documented. zpool-features.7 is updated for that as well.

How Has This Been Tested?

Tried creating boot pool with compatibility=grub2-2.06, after
creating the snapshot of top level dataset of ZFS boot pool.
Verified grub-probe still detects the ZFS pool.

Confirmed after re-enabling extensible_dataset feature and issue
occurs.

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:

@amotin
Copy link
Member

amotin commented Feb 20, 2024

I am not sure this approach is really usable. Without knowing which GRUB versions are affected, it is impossible to safely choose the compatibility profile. And the lack of understanding for the issue makes its further maintenance problematic. Read-only-compatible features should not cause problems to GRUB, so I wonder if it is just a GRUB bug triggered by some of the features, or some feature is not so much compatible as we'd like.

@rincebrain
Copy link
Contributor

It was a bug they've fixed, yes, but I don't think we're going to win the argument of making every distro cherrypick grub2 fixes in a timely fashion, compared to people running our newest versions or people packaging newer things than the distro does, and getting burned.

I said in another one, I think #15459, that I think we should do the opposite, and make grub2 the minimum set of features we know don't break grub2 anywhere, since that's historically what we used this for. I realize this makes people dirty retroactively, which is unfortunate, but that's a tradeoff of historical pools causing "why is it saying this" versus potentially unbounded versions of new pools causing surprises if they follow what we used the compatibility=grub2 option to mean historically.

Admittedly, this is biased by me not looking forward to having to explain to a lot of people who will come by asking why -o compatibility=grub2 broke boot for them until the unfixed versions of grub2 are entirely gone from the list of things people commonly install, but I think I'd make the same argument even if I wasn't going to be the one often explaining this to people later.

@usaleem-ix usaleem-ix changed the title Add grub2-legacy for minimum list of features for grub2 compatibility Add compatibility file for GRUB versions up to v2.06 Feb 28, 2024
@usaleem-ix
Copy link
Contributor Author

I have updated the PR and added a new compatibility file, grub2-2.06 for GRUB versions up to 2.06. compatibility=grub2-2.06 would enable all read-only compatible zpool features except extensible_dataset and other features that depend on it.

The existing grub2 compatibility file lists all read-only features that can be enabled on boot pool for grub with version 2.12
onwards.

GRUB is not able to detect ZFS pool if snaphsot of top level boot
pool is created. This issue is observed with GRUB versions up to
v2.06 if extensible_dataset feature is enabled on ZFS boot pool.

compatibility=grub2-2.06 would enable all read-only compatible
zpool features except extensible_dataset and other features that
depend on it.

The existing grub2 compatibility file lists all read-only features
that can be enabled on boot pool for grub with version 2.12
onwards.

Signed-off-by: Umer Saleem <usaleem@ixsystems.com>
@usaleem-ix
Copy link
Contributor Author

I have been working on testing all the zpool features present in grub2-2.06 on earlier versions of grub, as early as 2.02~beta2-36ubuntu3.27 on Ubuntu 16.04. I have tested after activating the features and grub still works, able to detect zfs boot pool and boots from it.

While the naming convention of newly added file and whether we should rename this file to grub2 is up for discussion, since replacing would mean there will be zpool features enabled on pool that are not listed in compatibility list, and would dirty the zpool status for existing pools.

The issue is fixed in grub and working around it by dirty-ing the pool status for nearly all zpools using grub2 compatibility does not seem like a good solution to me.

@amotin amotin added Status: Code Review Needed Ready for review and testing Status: Design Review Needed Architecture or design is under discussion labels Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing Status: Design Review Needed Architecture or design is under discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants