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

Enable /boot on btrfs subvolume with GRUB2 #2255

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

Conan-Kudo
Copy link
Contributor

@Conan-Kudo Conan-Kudo commented Dec 18, 2019

This depends on a version of grubby with btrfs subvolume support. For example, grubby >= 8.40-10.

This essentially reverses the block imposed in response to rhbz#864198 and correctly fixes rhbz#1038885.

Resolves: rhbz#1418336

This pull request obsoletes #1375.

@Conan-Kudo Conan-Kudo force-pushed the bootable-btrfs-for-all branch from eaaa184 to 9287ed7 Compare December 18, 2019 01:03
This depends on a version of grubby with btrfs subvolume support. For
example, grubby >= 8.40-10.

This essentially reverses the block imposed in response to rhbz#864198
and correctly fixes rhbz#1038885.

Resolves: rhbz#1418336
@Conan-Kudo Conan-Kudo force-pushed the bootable-btrfs-for-all branch from 9287ed7 to c113e6b Compare December 18, 2019 01:06
@@ -114,7 +114,7 @@ class GRUB2(BootLoader):
stage2_must_be_primary = False

# requirements for boot devices
stage2_device_types = ["partition", "mdarray"]
stage2_device_types = ["partition", "mdarray", "btrfs subvolume"]
stage2_raid_levels = [raid.RAID0, raid.RAID1, raid.RAID4,
raid.RAID5, raid.RAID6, raid.RAID10]
stage2_raid_member_types = ["partition"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This statement confused me. This didn't exist before, and I'm not entirely sure what it does. Should I be concerned about it for Btrfs enablement?

Copy link
Contributor

@cmurf cmurf Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stage2 device is a reference to the portion of BIOS GRUB found in /boot/grub (/boot/grub2 on Fedora). There are three consequences with /boot on Btrfs:
a. If the user opts for zstd compression, boot menu will fail to populate as GRUB+blscfg.mod will find but fail to read the compressed config files in /boot/loader/entries/*.conf. Fix for that is to get zstd baked into the grubx64.efi on UEFI and stage 1.5 for BIOS GRUB. I have a todo for that. (lzo and zlib compression support is built into btrfs.mod, so that's always worked). The short term work around is for the user to use chattr +C on /boot or just don't use compression. This is the only "fatal" consequence of /boot on Btrfs, but the user has to opt to get there, because the installer doesn't give them the option to use compression.
b. On BIOS, the grubenv is located on /boot/grub2/grubenv and GRUB disallows writes to grubenv when on btrfs (and raid, lvm, luks, zfs, maybe some other things) because GRUB's file writes are sector overwrites, there's no file system metadata updates, including checksums on Btrfs. Mismatching checksums means kernel code will EIO on such files. Basically Btrfs considers such writes as corrupting file data. And it's something other fs upstreams take issue with, including XFS and ext4. A better solution for getting state out of GRUB is needed. The hidden GRUB menu feature includes GRUB changing boot_success=1 to 0, and if boot succeeds, grubenv modified to change boot_success=1. That way GRUB knows whether to display the GRUB menu following a failed boot. On Btrfs, the menu won't ever be shown unless the user intervenes by pressing a key, because GRUB won't write to gruvenv on Btrfs.
c. There's a small complication with the BootloaderSpec which ostensibly is about having a shared /boot across multiple OS installations; and I'm not sure if this is something Fedora wants to support. GPT partition type GUID provides lightweight discovery of a partitions purpose, where we lack such a facility with a /boot on Btrfs. It could be done with an xattr but that's a bit "secret decoder ring" in that it requires parsing Btrfs on-disk format, rather than looking for a GUID in a fixed location in GPT. See bug
https://bugzilla.redhat.com/show_bug.cgi?id=825236#c38

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix for that is to get zstd baked into the grubx64.efi on UEFI

This one should be easy to fix then.

@Conan-Kudo
Copy link
Contributor Author

Also, as somewhat of a sanity check, do I need to do anything special here for handling with BLS mode? At a glance, it didn't seem like it...

(cc: @martinezjavier)

@poncovka poncovka added manual testing required This issue can't be merged without manual testing master Please, use the `f39` label instead. labels Dec 18, 2019
@poncovka
Copy link
Contributor

Jenkins, test this please.

@martinezjavier
Copy link
Contributor

Also, as somewhat of a sanity check, do I need to do anything special here for handling with BLS mode?

The kernel-install plugins should already figure out the relative path to the kernel and initrd images by using grub2-mkrelpath if /bootis in a btrfs subvolume.

I wasn't aware of the issues that @cmurf pointed out but I'm also not that familiar with btrfs.

@jkonecny12 jkonecny12 added the notable change Important changes like API change, behavior change... label Dec 18, 2019
@jkonecny12
Copy link
Member

I've assigned @Conan-Kudo to the bug and set the bug to POST.

@jkonecny12
Copy link
Member

jkonecny12 commented Dec 18, 2019

Also @Conan-Kudo please write here explicitly when this feature is ready for merging. We then make the sanity checking ;)

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me.

@jkonecny12 jkonecny12 self-assigned this Jan 2, 2020
@jkonecny12
Copy link
Member

jkonecny12 commented Jan 3, 2020

I've done sanity check and everything seems to be working fine.

Everything is ready for merge. Just waiting for @Conan-Kudo confirmation.

@jkonecny12 jkonecny12 removed the manual testing required This issue can't be merged without manual testing label Jan 3, 2020
@Conan-Kudo
Copy link
Contributor Author

@vathpela wanted to take a look first. Once I get the 👍 from him, we can merge this.

@vathpela
Copy link
Contributor

vathpela commented Jan 6, 2020

I don't see any obvious issues here. FYI I've done https://koji.fedoraproject.org/koji/taskinfo?taskID=40191468 to address the zstd grub efi issue.

@Conan-Kudo
Copy link
Contributor Author

@jkonecny12 This is good to merge then!

@jkonecny12 jkonecny12 merged commit ff173eb into rhinstaller:master Jan 7, 2020
@Conan-Kudo Conan-Kudo deleted the bootable-btrfs-for-all branch January 7, 2020 14:04
@Conan-Kudo
Copy link
Contributor Author

🎉 🎆 🥇 💯

@jkonecny12
Copy link
Member

Welcome to the community maintainers team @Conan-Kudo ;). Enjoy your feature!

@mhussaincov
Copy link

mhussaincov commented Feb 11, 2020 via email

egservice pushed a commit to vulturm/macbook-grub2 that referenced this pull request Feb 21, 2020
cmurf and javierm noticed[0] that we don't have zstd enabled, and that could
cause issues in some cases for /boot on btrfs subvolumes.  This adds it to our
module list.

[0] rhinstaller/anaconda#2255 (comment)

Related: rhbz#1418336

Signed-off-by: Peter Jones <[email protected]>
@jkonecny12
Copy link
Member

hey there, thank god you enabled booting on btrfs! I am a end user of fedora, don't have any programmeing skills of any kind, just wanted to thank you and the team for this! when do you believe that this feature will end up in fedora? note, i'm running fedora 31 congrats on this by the way! Majid

On 07/01/2020, Jiri Konecny @.***> wrote: Welcome to the community maintainers team @Conan-Kudo ;). Enjoy your feature! -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #2255 (comment)
-- kind regards, Majid Hussain

Hi, it will be part of Fedora 32.
We're happy that we were able to find a way to get this done! Thanks a lot to @Conan-Kudo for stepping out and making the changes.
We can't update already released Fedora so you have to wait for next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master Please, use the `f39` label instead. notable change Important changes like API change, behavior change...
Development

Successfully merging this pull request may close these issues.

7 participants