Skip to content

OpenZFS 7446 - zpool create should support efi system partition - #7664

Closed
behlendorf wants to merge 1 commit into
openzfs:masterfrom
behlendorf:openzfs-7446
Closed

behlendorf wants to merge 1 commit into
openzfs:masterfrom
behlendorf:openzfs-7446

Conversation

@behlendorf

Copy link
Copy Markdown
Contributor

Description

Since we support whole-disk configuration for boot pool, we also will need whole disk support with UEFI boot and for this, zpool create should create efi-system partition.

Motivation and Context

OpenZFS-issue: https://illumos.org/issues/7446
OpenZFS-commit: openzfs/openzfs@7855d95b30

How Has This Been Tested?

Locally built with some basic manual functional testing. This patch conflicted significantly with many of the changes made to handle the Linux block device naming conventions. Those issues should all be resolved and the Linux behavior preserved. This PR should not change the default behavior in any way unless the bootfs property is set. When reviewing special attention should be paid to the updated zpool_label_disk function.

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

@codecov

codecov Bot commented Jun 29, 2018 •

Copy link
Copy Markdown

Codecov Report

Merging #7664 into master will decrease coverage by 0.04%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7664      +/-   ##
==========================================
- Coverage   77.96%   77.91%   -0.05%     
==========================================
  Files         368      368              
  Lines      111657   111757     +100     
==========================================
+ Hits        87054    87078      +24     
- Misses      24603    24679      +76
Flag Coverage Δ
#kernel 78.57% <100%> (-0.13%) ⬇️
#user 66.51% <50%> (-0.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e03a41a...595f88d. Read the comment docs.

Authored by: Toomas Soome <tsoome@me.com>
Reviewed by: Andrew Stormont <andyjstormont@gmail.com>
Reviewed by: Yuri Pankov <yuri.pankov@gmail.com>
Approved by: Dan McDonald <danmcd@kebe.com>
Ported-by: Brian Behlendorf <behlendorf1@llnl.gov>

OpenZFS-issue: https://illumos.org/issues/7446
OpenZFS-commit: openzfs/openzfs@7855d95
@claudiusraphaelpaeth

claudiusraphaelpaeth commented Jul 7, 2018 •

Copy link
Copy Markdown

In the linked feature request https://illumos.org/issues/7446 is stated:

For time being, I have set default size 256MB (thats minimum size for FAT32 with 4k blocks).

This is wrong, as the minimum size for creation is 259 MiB, because of the Allocation Table. So to provide 256 MiB as a default setting, won't work.

UEFI is not using FAT32, per se. Some BIOS, which do not follow the UEFI-specification, use FAT32 exclusively, which is the reason why in the first years of UEFI becoming an option for the common market (2008 to 2012) many recommendations issued that, and in the end it was reduced to use FAT32. UEFI does not require FAT32, but FAT-based in general. You have the option to use FAT16 for the initial format also, which gives you on 4KiB-Cluster a range from 123/128MiB to 255MiB. This is the easiest way to guarantee access on all kind of storage-types (UFD/HDD/SSHD/SSD/ODD/MODD/etc.) in all standard and advanced formats (512B/1K/2K/4K/8K/1M). UEFI allows for 256/1024 chars in utf-8 per folder-/file-name, in this case, too; Also a maximum Partition size of 4GiB as in FAT16B can be used for initial creation. Using 128MiB, is also the common advice for free space before any primary partition on GPT. Due to some partitioning/formatting tools having no support for empty-partitions/partition set to unformatted (Windows for example), the ESP-type (EFI-System-Partition) can be used to fill the gap. One reason, why ESP conditionals do not imply that only one ESP may be present. Whereas, there are other reasons, e.g. Backup of rootfs with guaranteed offsets. However
i'll cut it here, as this is the condensed info you need to 'respect' if you wish to support a (semi)automatic ESP-creation.

It is not meant as a lecture, but as a reasoning to overthink twice supporting 'common sense'-defaults.
Personally (and that is the reason why i wrote this comment) I am having a hard time accepting the rules you (the developers/coders) are seeing as 'default'. Out of interest for the reasoning of your (in-)sane 'defaults', e.g. 'enabling all features by default' or the (in my opinion absolute hazardous) (mis-)information 'Not all features are available … you should upgrade your pool', has been hitting me like a thunderstruck; especially when i read the discussions growing from the mentioning, that the notice is misleading.

Don't get me wrong, i am happy that you do this work, as it allows me to use this ('enterprise')-file-system on a massive amount of operating-systems. But, really, on the one hand saying it's an enterprise-fs for servers in operation (only) and not meant to be used as a backup- or data-pool-for-processing, and on the other hand defaulting to features/ways, in a way that even let Fedora look like an LTS, is, to sum it simple and nicely: 'Irritating'.

And the moment i have read 'Merging is blocked --- Merging can be performed automatically with 1 approving review.', which is itself a clear statement, regarding the conflict i tried to describe before, i wanted to take the chance and hopefully can motivate you to at least overthink the 'common' way you, the makers of this wonderful piece of software distribute.

I am trying hard to get myself back into the necessary-coding-skills, so i can at least make a customized 'sane' defaulting build, but i am not seeing, that i will get to that point, before you reach 1.0 RC. And neither did i understood completely in which way and under what conditions you do handle, feature requests, to create at least a proposal, with appended reasoning why.

So, thanks for your interest and patience on my input; please let this comment be an inspiration. It is in no way meant as offensive. Thank you.

@behlendorf

Copy link
Copy Markdown
Contributor Author

Closing for now. I'm not going to be able to make the time to finalize these changes and perform the needed testing. This can be revisited at a future date.

@behlendorf behlendorf closed this Aug 2, 2018
@gcs-github

Copy link
Copy Markdown

I'm interested in this feature. What would be the best way to help?

@rlaager

rlaager commented Apr 12, 2019

Copy link
Copy Markdown
Member

@gcs-github The best way to help would be to port OpenZFS 7446 to ZFS-on-Linux and open a pull request.

@codecov

codecov Bot commented Apr 12, 2019 •

Copy link
Copy Markdown

Codecov Report

Merging #7664 into master will increase coverage by 0.06%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7664      +/-   ##
==========================================
+ Coverage   77.96%   78.02%   +0.06%     
==========================================
  Files         368      368              
  Lines      111657   111757     +100     
==========================================
+ Hits        87054    87202     +148     
+ Misses      24603    24555      -48
Flag Coverage Δ
#kernel 78.57% <100%> (-0.14%) ⬇️
#user 67% <50%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e03a41a...595f88d. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants