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 all read-only compatible zpool features to grub2 compatibility #15459

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

usaleem-ix
Copy link
Contributor

Motivation and Context

GRUB opens the boot-pool in read-only mode, but not all read-only compatible zpool
features are listed in grub2 compatibility file.

Description

GRUB opens the boot pool in read-only mode. All read-only compatible features for zpool
can be enabled and added to grub2 compatibility, as GRUB does not open the boot-pool
for write.

How Has This Been Tested?

Tested by creating a boot-pool with all read-only compatible zpool features enabled and
booting from the boot-pool.

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:

GRUB opens the boot pool in read-only mode. All read-only
compatible features for zpool can be enabled and added to
grub2 compatibility, as GRUB does not open the boot-pool
for write.

Signed-off-by: Umer Saleem <[email protected]>
@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Oct 31, 2023
@behlendorf behlendorf merged commit cba99a0 into openzfs:master Oct 31, 2023
19 checks passed
@usaleem-ix usaleem-ix deleted the NAS-124771 branch November 1, 2023 05:21
usaleem-ix added a commit to truenas/zfs that referenced this pull request Nov 1, 2023
GRUB opens the boot pool in read-only mode. All read-only
compatible features for zpool can be enabled and added to
grub2 compatibility, as GRUB does not open the boot-pool
for write.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes openzfs#15459
usaleem-ix added a commit to truenas/zfs that referenced this pull request Nov 1, 2023
GRUB opens the boot pool in read-only mode. All read-only
compatible features for zpool can be enabled and added to
grub2 compatibility, as GRUB does not open the boot-pool
for write.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes openzfs#15459
ixhamza pushed a commit to truenas/zfs that referenced this pull request Nov 20, 2023
GRUB opens the boot pool in read-only mode. All read-only
compatible features for zpool can be enabled and added to
grub2 compatibility, as GRUB does not open the boot-pool
for write.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes openzfs#15459
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
GRUB opens the boot pool in read-only mode. All read-only
compatible features for zpool can be enabled and added to
grub2 compatibility, as GRUB does not open the boot-pool
for write.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Umer Saleem <[email protected]>
Closes openzfs#15459
@rincebrain
Copy link
Contributor

rincebrain commented Feb 11, 2024

#15261 notes that any feature that enables extensible_dataset appears to break at least some versions of grub if there's literally any snapshot ever on the root dataset, and it does not unbreak if you destroy the snapshot.

So this should probably be reverted.

(I don't, personally, boot ZFS with GRUB, so this is just my summary from trying to digest those bug reports, but it seems like we should probably do something about breaking things like this...)

@usaleem-ix
Copy link
Contributor Author

@rincebrain Since this PR was opened after #15261 was reported, it seems less likely this would be the root cause. I quickly checked and this commit is not present in 2.1.x and 2.2.0, which the issue is describing. It's present from 2.2.1 onwards.

extensible_dataset feature was added in grub2 compatibility list when it was first introduced 3 years ago. We may proceed to remove extensible_dataset and all features that depend on extensible_dataset, but reverting this one would not cover all features that can enable extensible_dataset.

@rincebrain
Copy link
Contributor

rincebrain commented Feb 12, 2024 via email

@usaleem-ix
Copy link
Contributor Author

Adding a second compatibility file with minimum set of features required, seems more reasonable to me.

@behlendorf
Copy link
Contributor

Then perhaps a new cmd/zpool/compatibility.d/grub2-minimal compatibility file with just the bare essentials.

@rincebrain
Copy link
Contributor

I almost feel like the reverse might be better, where "grub2" means "every version of grub2 works with this" and "grub2-features" means "this is the most features we know of any grub2 version supporting", since people have, historically, used grub2 to try to mean "as many features as we can reliably enable without breaking boot, and no more", so moving that concept to a new name would require changing lots of expectations, whereas a new featureful one would be adding a new option while avoiding changing older expectations for this.

@usaleem-ix
Copy link
Contributor Author

#15909

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants