Remove grub special-casing #8627
Conversation
rlaager
force-pushed
the
remove-grub-references
branch
from
7f35d64
to
9ad0415
Compare
rlaager
force-pushed
the
remove-grub-references
branch
from
9ad0415
to
4c2aa21
Compare
|
This has been rebased on master after my changes in #8626 were merged. This eliminates the shared commit (which was already merged) and strips out the GRUB mentions from |
Codecov Report
@@ Coverage Diff @@
## master #8627 +/- ##
==========================================
+ Coverage 79.24% 79.75% +0.51%
==========================================
Files 393 393
Lines 123859 123833 -26
==========================================
+ Hits 98149 98766 +617
+ Misses 25710 25067 -643
Continue to review full report at Codecov.
|
rlaager
force-pushed
the
remove-grub-references
branch
from
4c2aa21
to
b4f0abc
Compare
|
I rebased this on master and removed the mention about lz4. |
|
I've been giving this some thought and I agree we're going to need to reevaluate the grub restrictions. They made good sense when both grub and ZFS could be updated together (in OpenSolaris) but that's never been true for Linux. In fact, these days there's a decent chance the system is booting with UEFI and doesn't use grub at all. In which case, we have no boot restrictions. @rlaager is there a compelling need to relax these restrictions right away? If not, I think we should consider tackling this with the proposed portable pools feature. |
|
Since 0.8.0 is feature frozen, I think this should wait until after then, at least. I think we are in agreement on that. I might ask for an exception if this would otherwise have to wait for 0.9.0 rather than 0.8.1, but I’m not even sure how I feel about either part of that. I think your concern might be that we are removing the working-but-incomplete GRUB restrictions (and maybe more importantly, the warnings in the documentation) without having a better replacement. If that is how you feel, we can wait until a features=grub is available. Alternatively, we could rip out the code, since it is incomplete, but leave the restrictions in the docs. I think those are complete, from my previous PR. |
|
Yes, that's right. I'm not necessarily against removing these restrictions, but before doing so I'd like to have a clearer path forward. If we have a specific use case, relaying them is definitely still an option for 0.8.1. One additional consideration is that after 0.8 we'll be bringing in the code to make it possible for FreeBSD to use this repository as its upstream. They have more control over their grub implementation and they may want to keep, or even extend, these restrictions, |
FreeBSD has its own bootloader that isn't GRUB. Someone poked me about reviewing this. I can't do it right now, but I imagine that this waiting a few more days (or possibly a week) won't hurt. |
|
Just to add to the poking: Yesterday I hit #8538 when setting up a new system without GRUB. I'll have to re-create the root pool with |
Be aware that zstd on a zfs bootpool requires some more work on grub before it would work (even after merging #10278 ) @BrainSlayer already prepared the required grub modifications in a grubfork. |
|
@Ornias1993: Like I said, I don't plan on using GRUB. I directly boot a |
|
@Ornias1993 i dont think that my mods can handle the restrictions mentioned here. my mods are just a extension for the compression support. the rest remains. however the restrictions here mentioned state that grub doesnt support compression at all. thats not true. grub did support already all compression variants (except for my zstd mod which i made) |
I wasn't talking about this PR @BrainSlayer It was a general statement about ZSTD and Grub compatiblity. I was NOT refering to this PR. |
|
@Ornias1993 okay |
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. Signed-off-by: Richard Laager <rlaager@wiktel.com>
rlaager
force-pushed
the
remove-grub-references
branch
from
b4f0abc
to
2545268
Compare
|
I've rebased this on current master. It still looks correct to me, but I haven't actually tested this change in any way (even building it). |
There was a problem hiding this comment.
I agree with the general approach, but I think it would be good to replace the documentation notes we're removing with a general explanation that GRUB is falling increasingly behind in feature support, so GRUB boot pools need to be very careful about what features they're enabling. While this may be reasonably well known among the ZFS community, there are plenty of potential new users who don't know about these issues.
|
@pcd1193182 Do you have any specific language or ideas of where to document this? |
There was a problem hiding this comment.
@rlaager unless you object I'd like to go ahead and merge this as is. We can follow up with improved documentation once we have a better idea what that should look like.
behlendorf
added
Status: Accepted
|
@behlendorf I remain in favor of this. |
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 <pcd@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Laager <rlaager@wiktel.com> Closes openzfs#8627
|
@rlaager or anyone, could you please clarify the current state of You mentioned:
This sounds like GRUB requires setting I assumed your comment meant that the plan was to lift that limitation. I am using zfs-2.0.0-rc5, which seems to include the related commit eaa25f1a8e. In this version, I cannot set I am having issues getting GRUB working with two pools - boot pool So I am trying to figure out if there is any way to force GRUB2 ignoring pools without |
It does not. Really, just pretend the
GRUB will boot the kernel and initrd from bpool. That will mount what is specified by root=ZFS=..., per step 5.3. Make sure you set that manually (because GRUB doesn't support all the features on
That compression error can almost certainly be ignored. |
|
I might be hijacking this thread to discuss my GRUB issues. I would appreciate any help. This may also be a pretty common scenario; perhaps other people facing the same issue may find this useful. If you would prefer me to move this comment to a new thread or issue, please let me know. It is really hard to ignore compression errors, because then GRUB spits out more than 24 lines of errors in the console, prints I tried multiple ways to suppress errors or configure serial terminal with an artificially large height, but it looks like GRUB just uses 24 lines by default, so I reached a dead end. I guess my question is how GRUB makes a distinction between a boot pool and a non-boot pool. GRUB probes boot device on mkconfig:
This code processes the boot partition correctly, and generates Output: All this code is only used for the menu generation, and the I also saw GRUB using And the other comment implies that I was hoping that I could use |
|
instead of fixing bootfs and making it work it's been bandied about like a hot potato no one wants to touch because it'll be misunderstood/misused. it has been proposed to remove the property entirely. |
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 <pcd@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Laager <rlaager@wiktel.com> Closes openzfs#8627
Motivation and Context
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.
Description
This removes the bootfs-based restrictions that are intended to address GRUB's feature limitation.
How Has This Been Tested?
I have done zero testing of this particular change.
Types of changes
Checklist:
Signed-off-by.