Increase the minimum EFI System Partition (ESP) size to 500MiB #4711

hughsie · 2023-04-24T10:57:32Z

Currently the default allocated space for the EFI System Partition (ESP) is a minimum of 200 MiB and a maximum of 600MiB. This smaller size is more than enough for storing the EFI binaries just needed to boot the machine.

The ESP is also used to deploy firmware updates which need 2x SPI flash size (typically 64MB, soon 128MB) as free space. We're also soon going to modernize how the bootloader is deployed, and this also is going to need more space again.

Microsoft mandates OEMs to allocate a minimum of 500MiB and so I think we should just do the same.

VladimirSlavik · 2023-04-24T11:28:45Z

/kickstart-test --testtype smoke

VladimirSlavik · 2023-04-24T11:36:32Z

=========================== short test summary info ============================
FAILED unit_tests/pyanaconda_tests/modules/storage/test_platform.py::PlatformTestCase::test_aarch64_efi
FAILED unit_tests/pyanaconda_tests/modules/storage/test_platform.py::PlatformTestCase::test_arm_efi
FAILED unit_tests/pyanaconda_tests/modules/storage/test_platform.py::PlatformTestCase::test_efi

These unit tests all expect the size to be 200, like size=Size("200MiB") - you'll want to increase that to 500.

Kickstart tests succeeded, so this should work in reality.

hughsie · 2023-04-24T12:07:52Z

These unit tests all expect the size to be 200

Aha, I wasn't sure if those were defining the size or testing. Fixed, thanks.

VladimirSlavik · 2023-04-24T12:21:19Z

I just let the tests run again, but... I'll note that you bumped the default for normal platforms and the 3 tests for that, and also the one mac_efi test but not the actual value for that platform... ;-)

But we're getting there! Thank you!

VladimirSlavik · 2023-04-24T12:23:59Z

I can write a release note later, if you want to skip that part... If not, see docs/release-notes/template.rst

hughsie · 2023-04-24T12:32:55Z

I just let the tests run again, but... I'll note that you bumped the default for normal platforms and the 3 tests for that, and also the one mac_efi test but not the actual value for that platform... ;-)

Do we also want the macefi size to be 500MB? There is not going to be firmware updates for that hardware, but I think it makes sense to be consistent.

VladimirSlavik · 2023-04-24T13:30:56Z

I can't answer this without guessing. But if I were to go for that guess - there's no updates, so there's no use for the space, and it's sufficient as of now, so let's keep it as-is.

hughsie · 2023-04-24T14:05:45Z

Ack, okay. Fixed.

VladimirSlavik · 2023-04-24T15:52:22Z

/kickstart-test --testtype smoke

VladimirSlavik

Code LGTM, thank you!

The release note text could use some more work, I'd suggest something like this:

    The size of the EFI System Partition (ESP) created by Anaconda has changed from 200 MiB to 500 MiB.

    The reasons for this change include:
    - This partition is used to deploy firmware updates. These updates need free space of twice the SPI flash size, which will grow from 64 to 128 MiB in near future and make the current partition size too small.
    - The larger space enables future changes to how the bootloader is deployed on Fedora.
    - The new minimum is identical with what Microsoft mandates OEMs allocate for the partition.

I'd also suggest "Bootloader" for the Type at top. This applies to all installations, not just kickstart.

Feel free to leave cleaning that for us though...

berrange · 2023-04-24T17:08:56Z

pyanaconda/modules/storage/platform.py

@@ -224,7 +224,7 @@ def _bootloader_partition(self):
        return PartSpec(
            mountpoint="/boot/efi",
            fstype="efi",
-            size=Size("200MiB"),
+            size=Size("500MiB"),
            max_size=Size("600MiB"),


With the default bumped all the way to 500, keeping 600 for the maximum is not really facilitating much enlargement. IOW should max_size also be increased, perhaps to 1GiB ?

@berrange I was struggling to think what we would do with more than 600GB.

IIUC, /boot is already 1 GB and that's primary to cope with multiple big kernels. If we expect those kernels to all turn into UKIs, then that would seem to justify having 1 GB for ESP too, or at the very least raising the ceiling to all people to opt-in to much larger ESP.

Yes, agreed, I've modified the patch to allow scaling up to 2GiB now.

poettering · 2023-04-25T08:01:54Z

I would prefer if this would grow even larger. We already know that gfx driver people want to include their drivers in initrds in a future no so far away, and that means 100mb+ in an initrd. if you stick to 500M this is going to require a bumping very soon again I am sure. And it's really hard retroactviely growing the ESP.

(see here for a discussion about that: https://mastodon.social/@karolherbst@chaos.social/110066943704825190)

I'd really be more aggressive here. i.e. on a large disk go for 2g at least, and on the tiniest of systems I'd still go for 800m or so. (if tech savvy people really want to go smaller, allow them via some form of override, sure, but by default i would never go below 800M, so you have some room for the coming years)

poettering · 2023-04-25T08:03:47Z

(and it's not just that some drivers will grow to 100M+ but that we want to go for a future with pre-build kernels, and that likely means shipping multiple of them in the ESP at the same time, even on systems that don't need them.)

poettering · 2023-04-25T08:10:27Z

This well-respected page from 2015 suggests 550M as a minimum btw: https://www.rodsbooks.com/linux-uefi/

ArchLinux recommends 512M minimum https://wiki.archlinux.org/title/EFI_system_partition (and that's a pretty old recommendation too)

debian also defaults to 500M since a longer time.

I think in 2023 we shouldn't really default to a default that 8 years ago seemed appropriate, in particular with the clear perspective pre-built UKIs and graphics drivers in the initrd will boost our disk space needs in future.

hughsie · 2023-04-25T08:46:24Z

I would prefer if this would grow even larger.

I think a lot of people working on UKIs would agree, and that can certainly be a future change -- but this hopefully non-controversial change is about aligning the value as used by Microsoft and to fix future firmware updates. Your suggested 2GB is going to be a significant percentage of space on IoT devices and needs much more careful consideration -- for instance deleting the /boot and expanding /boot/efi when we do the switch to UKIs, or maybe defaulting to the larger "maximum" value when the target disk is much larger. That's deliberately not covered in this proposal, although I have changed the maximum size to 2GiB to be helpful.

hughsie · 2023-04-25T08:46:44Z

The release note text could use some more work, I'd suggest something like this:

I've modified it to your text, thanks.

poettering · 2023-04-25T08:52:15Z

I would prefer if this would grow even larger.

I think a lot of people working on UKIs would agree, and that can certainly be a future change -- but this hopefully non-controversial change is about aligning the value as used by Microsoft and to fix future firmware updates. Your suggested 2GB is going to be a significant percentage of space on IoT devices and needs much more careful consideration -- for instance deleting the /boot and expanding /boot/efi when we do the switch to UKIs, or maybe defaulting to the larger "maximum" value when the target disk is much larger. That's deliberately not covered in this proposal, although I have changed the maximum size to 2GiB to be helpful.

Well so far the only controversy your proposal triggered was that people (such as me) saying you are not bold enough, and go higher. I think the noise in the other direction was rather muted, no?

hughsie · 2023-04-25T08:56:40Z

saying you are not bold enough

You're completely free to submit another PR and Fedora Change Proposal which supersedes this one. I'm changing the default by 300MB for two easy to justify reasons, you're proposing changing it by 1800 for a different reason entirely.

VladimirSlavik · 2023-04-25T17:56:18Z

If there's a Fedora Change associated with this, I'd love to see it mentioned in the commit message as well as release note links :-)

VladimirSlavik · 2023-04-25T17:56:24Z

/kickstart-test --testtype smoke

hughsie · 2023-04-25T20:53:50Z

It's here: https://fedoraproject.org/wiki/Changes/BiggerESP

Maybe don't link to it just yet as we're figuring out if it's a self contained change or a systemwide change.

nl6720 · 2023-04-26T06:56:18Z

ArchLinux recommends 512M minimum https://wiki.archlinux.org/title/EFI_system_partition (and that's a pretty old recommendation too)

Arch recommends 300 MiB which matches what, AFAIK, Windows Setup creates on 4Kn drives. 512 MiB is only recommended for "early and/or buggy UEFI implementations". And for systems with that will have multiple kernel packages installed, the Arch recommendation is at least 1 GiB.

jkonecny12 · 2023-04-26T10:06:00Z

Hi thanks for this contribution. I see two things we should resolve before merging this. Give it visibility for a few people (I'll ping them) and have the change accepted by FESCO (it is not right now).

Also @hughsie if you can, please add the change link to the Release notes.

Hi @jstodola @frozencemetery, are you fine with this change?

jkonecny12 · 2023-04-26T10:06:22Z

Blocked until we have ACKs from all the parties.

jkonecny12

Looks good to me.

Currently the default allocated space for the EFI System Partition (ESP) is a minimum of 200 MiB and a maximum of 600MiB. This smaller size is more than enough for storing the EFI binaries just needed to boot the machine. The ESP is also used to deploy firmware updates which need 2x SPI flash size (typically 64MB, soon 128MB) as free space. We're also soon going to modernize how the bootloader is deployed, and this also is going to need more space again. Microsoft mandates OEMs to allocate a minimum of 500MiB and so I think we should just do the same. Also increase the maximum size to 2GiB to accommodate future UKIs. Signed-off-by: Richard Hughes <richard@hughsie.com>

hughsie · 2023-04-26T11:07:55Z

Also @hughsie if you can, please add the change link to the Release notes.

Done.

frozencemetery · 2023-04-26T11:53:58Z

Hi @jstodola @frozencemetery, are you fine with this change?

Yes, bootloader folks are fine with it. Thanks for checking

jstodola

It looks good to me.

VladimirSlavik · 2023-05-04T12:46:21Z

/kickstart-test --testtype smoke

VladimirSlavik · 2023-05-25T16:48:29Z

Hello @hughsie, do you have any idea when this will be revisited, roughly? I don't want to miss that just because the final commitment happens elsewhere than this PR...

hughsie · 2023-05-25T16:49:22Z

This was approved as a fedora feature. I think it's good to go now, actually.

AdamWill · 2023-06-07T18:10:58Z

Now we're looking into this for https://bugzilla.redhat.com/show_bug.cgi?id=2212121 , one thing that worries me: did we consider the impact of the large increase in max size on cases like live images? i.e. deliverables which are built through anaconda? Presumably we don't want those to have 2G ESPs, but maybe now they will? I haven't checked, but it seems like a concern.

AdamWill · 2023-06-11T00:49:55Z

So, the first compose we got through with this change shows two failures in openQA tests that are probably related to it (and a third, but that one probably just needs adjustments to the test to handle the larger ESP). I didn't dig into them yet as it's the weekend, but if anyone else wants to, feel free. anaconda logs and journal are on the assets tab:

https://openqa.fedoraproject.org/tests/1971210 (UEFI install of Workstation package set to a 13GiB disk)
https://openqa.fedoraproject.org/tests/1971142 (test which takes a 10GiB disk with two 5GiB partitions, deletes one of them, and does a minimal install to the freed-up space)

AdamWill · 2023-06-12T17:14:35Z

I've filed the various issues openQA ran into after this change and marked them as blocking the Change bug tracker:

For now we have worked around the aarch64 image size issue by simply increasing the image sizes, on the assumption that we really do want the ESP on those images to be 2G in size; if we don't, we'll have to make more adjustments somewhere.

I will probably fiddle with the openQA stuff to work around the other two bugs so those tests don't fail every day, but the underlying issues do still need to be looked at.

VladimirSlavik added the f39 label Apr 24, 2023

VladimirSlavik self-assigned this Apr 24, 2023

hughsie force-pushed the esp-size branch from cee7fc6 to 0d069c3 Compare April 24, 2023 12:07

VladimirSlavik added the release note required Write a release note for this change. label Apr 24, 2023

hughsie force-pushed the esp-size branch from 0d069c3 to 39351d8 Compare April 24, 2023 14:05

VladimirSlavik approved these changes Apr 24, 2023

View reviewed changes

berrange reviewed Apr 24, 2023

View reviewed changes

hughsie force-pushed the esp-size branch from 39351d8 to 6e45ce7 Compare April 25, 2023 08:49

VladimirSlavik added notable change Important changes like API change, behavior change... and removed release note required Write a release note for this change. labels Apr 26, 2023

jkonecny12 added the blocked Don't merge this pull request! label Apr 26, 2023

jkonecny12 approved these changes Apr 26, 2023

View reviewed changes

hughsie force-pushed the esp-size branch from 6e45ce7 to c253390 Compare April 26, 2023 11:07

jstodola approved these changes Apr 26, 2023

View reviewed changes

VladimirSlavik removed the blocked Don't merge this pull request! label May 26, 2023

VladimirSlavik merged commit 3b1fde0 into rhinstaller:master May 26, 2023

VladimirSlavik mentioned this pull request Aug 28, 2023

Revert the ESP maximum size back to 600MiB #5081

Merged

5 tasks

Increase the minimum EFI System Partition (ESP) size to 500MiB #4711

Increase the minimum EFI System Partition (ESP) size to 500MiB #4711

Conversation

hughsie commented Apr 24, 2023

VladimirSlavik commented Apr 24, 2023

VladimirSlavik commented Apr 24, 2023 • edited Loading

hughsie commented Apr 24, 2023

VladimirSlavik commented Apr 24, 2023

VladimirSlavik commented Apr 24, 2023

hughsie commented Apr 24, 2023

VladimirSlavik commented Apr 24, 2023

hughsie commented Apr 24, 2023

VladimirSlavik commented Apr 24, 2023

VladimirSlavik left a comment

Choose a reason for hiding this comment

berrange Apr 24, 2023

Choose a reason for hiding this comment

hughsie Apr 24, 2023

Choose a reason for hiding this comment

berrange Apr 25, 2023

Choose a reason for hiding this comment

hughsie Apr 25, 2023

Choose a reason for hiding this comment

poettering commented Apr 25, 2023 • edited Loading

poettering commented Apr 25, 2023

poettering commented Apr 25, 2023

hughsie commented Apr 25, 2023

hughsie commented Apr 25, 2023

poettering commented Apr 25, 2023

hughsie commented Apr 25, 2023

VladimirSlavik commented Apr 25, 2023

VladimirSlavik commented Apr 25, 2023

hughsie commented Apr 25, 2023

nl6720 commented Apr 26, 2023

jkonecny12 commented Apr 26, 2023

jkonecny12 commented Apr 26, 2023 • edited Loading

jkonecny12 left a comment

Choose a reason for hiding this comment

hughsie commented Apr 26, 2023

frozencemetery commented Apr 26, 2023

jstodola left a comment

Choose a reason for hiding this comment

VladimirSlavik commented May 4, 2023

VladimirSlavik commented May 25, 2023

hughsie commented May 25, 2023

AdamWill commented Jun 7, 2023

AdamWill commented Jun 11, 2023 • edited Loading

AdamWill commented Jun 12, 2023

VladimirSlavik commented Apr 24, 2023 •

edited

Loading

poettering commented Apr 25, 2023 •

edited

Loading

jkonecny12 commented Apr 26, 2023 •

edited

Loading

AdamWill commented Jun 11, 2023 •

edited

Loading