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.

Sign up for GitHub

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

Jump to bottom

Add note for upcoming ZoL 2.1 compatibility property #433

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add note for upcoming ZoL 2.1 compatibility property #433

wants to merge 1 commit into from

Conversation

jacobgreenleaf
Copy link

@jacobgreenleaf jacobgreenleaf commented Apr 25, 2023 •
edited
Loading

Makes a small note available both for existing users of bullseye-backports and the soon to be released bookworm release that the compatibility option is available and we can use grub2.

A future version of this guide referencing bookworm may
want to continue enabling some of the features that are read-compatible
but not on that list. That is:

zpool create \ 
    -o ashift=12 \ 
    -o autotrim=on -d \ 
    -o cachefile=/etc/zfs/zpool.cache \ 
-   -o feature@async_destroy=enabled \ 
-   -o feature@bookmarks=enabled \ 
-   -o feature@embedded_data=enabled \ 
-   -o feature@empty_bpobj=enabled \ 
-   -o feature@enabled_txg=enabled \ 
-   -o feature@extensible_dataset=enabled \ 
-   -o feature@filesystem_limits=enabled \ 
-   -o feature@hole_birth=enabled \ 
-   -o feature@large_blocks=enabled \ 
    -o feature@livelist=enabled \ 
-   -o feature@lz4_compress=enabled \ 
-   -o feature@spacemap_histogram=enabled \ 
    -o feature@zpool_checkpoint=enabled \ 
+   -o compatibility=grub2 \ 
    -O devices=off \ 
    -O acltype=posixacl -O xattr=sa \ 
    -O compression=lz4 \ 
    -O normalization=formD \ 
    -O relatime=on \ 
    -O canmount=off -O mountpoint=/boot -R /mnt \ 
    bpool

And possibly device_rebuild, resilver_defer to be added.

@rlaager rlaager self-assigned this Apr 28, 2023
@colmbuckley
Copy link
Contributor

In theory, all "read-only compatible" properties should be added to the compatibility.d/grub2 file. I have an open PR to add livelist and zpool_checkpoint. Is there any reason why the other two shouldn't also be added? Why do you say "possibly" here?

@colmbuckley
Copy link
Contributor

In theory, all "read-only compatible" properties should be added to the compatibility.d/grub2 file. I have an open PR to add livelist and zpool_checkpoint. Is there any reason why the other two shouldn't also be added? Why do you say "possibly" here?

... ah, this is explained in the existing doc. @rlaager let me know if you think we should just add these to compatibility.d/grub2 upstream (or add to my PR yourself); it's probably a good idea to minimize the differences in features enabled on grub2 pools and no-compatibility pools?

@ghost
Copy link

ghost commented May 25, 2023 via email

@gmelikov
Copy link
Member

Even more, grub from Suse doesn't work well with most of features, didn't have time to investigate further, unfortunately.

But I'm all to use compats with actual tests on selected distros.

@rlaager
Copy link
Member

rlaager commented May 25, 2023 •
edited
Loading

I commented on the PR:
This seems fine to me. I've been listing those features in the Debian Root-on-ZFS HOWTO. Note that I explicitly warn against userobj_accounting, since I have seen errors with that, even though it should be compatible.

@ghost
Copy link

ghost commented May 25, 2023 via email

@colmbuckley
Copy link
Contributor

I'm all for providing more features on the GRUB pool. But since it is meant to be compatible with GRUB, compatibility with GRUB should be the main focus.

I absolutely agree; and to be honest I don't think I'm the best person to decide on which features should be present in which compatibility files - all I did was implement it.

Did you test the features with GRUB? Note that GRUB support for ZFS is indeed not robust, and there are instances where read-only compatible features which are not compatible with GRUB, such as the userobj_accounting feature. See this page.

I did test that GRUB worked ok with livelist and zpool_checkpoint, both of which provide features which are useful enough that I can envisage their being used on a boot pool.

The balance I think we should draw is "works with GRUB, has no reported problems, and is conceivably useful on a small /boot pool". userobj_accounting doesn't make the cut. @rlaager's "root on ZFS" page (this page, also the one you refer to) seems to draw a good balance - but I would be inclined to add known-good features to the compatibility.d/grub2 file.

@colmbuckley
Copy link
Contributor

Even more, grub from Suse doesn't work well with most of features, didn't have time to investigate further, unfortunately.

But I'm all to use compats with actual tests on selected distros.

This concerns me. What version of GRUB2 does SUSE ship with? Is there any conversation about these problems we could dig into?

@colmbuckley
Copy link
Contributor

colmbuckley commented May 25, 2023 •
edited
Loading

Let me pose the question a different way:

Assuming the main use of -o compatibility=grub2 is going to be "installing /boot on a ZFS partition, per the instructions in this document", which features should we consider including in the GRUB2 compatibility file?

I see several main categories of features:

  1. Known compatible with GRUB, low-risk, useful for boot pools:
    • async_destroy
    • bookmarks
    • embedded_data
    • empty_bpobj
    • enabled_txg
    • extensible_dataset
    • filesystem_limits
    • hole_birth
    • large_blocks
    • livelist
    • lz4_compress
    • spacemap_histogram
    • zpool_checkpoint
  2. Known, or presumed incompatible with GRUB:
    • bookmark_v2
    • bookmark_written
    • device_removal
    • draid
    • edonr
    • encryption
    • large_dnode
    • multi_vdev_crash_dump
    • obsolete_counts (technically compatible, but useless without device_removal)
    • redaction_bookmarks
    • redacted_datasets
    • sha512
    • skein
    • spacemap_v2
    • userobj_accounting
    • zstd_compress
  3. Probably compatible with GRUB, not currently recommended for boot pool:
    • allocation_classes
    • device_rebuild
    • log_spacemap
    • project_quota
    • resilver_defer

My question is: would any of the features in the final group here be actively harmful to include in compatibility.d/grub2 (and therefore probably being enabled by default in any installations which follow this guide)?

Approaches:

  1. Maximal conservatism: these features are very unlikely to ever be useful in a boot pool so we should steer clear of them.
  2. Maximal liberalism: they won't do any harm, and there's benefit to having the boot and root pools as close as possible to each other in configuation, so we should include them.
  3. Case by case.

My personal opinion favors maximal liberalism; I think there's a benefit to enabling as many features as possible on the boot pool and only excluding features which are known to break GRUB. Features being enabled is almost definitely the most common case (that's why they're called features, and why zpool status flags them as being available).

But I feel that @rlaager has put the most effort into figuring out what works well with GRUB and what doesn't, so I'm inclined to defer to his judgement.

WDYT?

@rlaager
Copy link
Member

rlaager commented May 26, 2023

  1. Known compatible with GRUB, low-risk, useful for boot pools:

One could potentially split a subset of this list into:
0. GRUB actually claims to support the feature by name.

  • lz4_compress
  • hole_birth
  • embedded_data
  • extensible_dataset
  • large_blocks

Then these remain in the next category:

  • async_destroy
  • bookmarks
  • empty_bpobj
  • enabled_txg
  • filesystem_limits
  • livelist
  • spacemap_histogram
  • zpool_checkpoint

I'm not sure all of these are "useful for boot pools". I'm sure we could bikeshed that to death. But, for example, I think "hole_birth" is effectively moot these days, after the bug was found in it.

I say this not to pick on you, but because this has been something I've actually thought about. Should the recommendations in the Root on ZFS HOWTOs be limited to just those that are demonstrably useful for a boot pool? That's a bit of a different question of whether a given feature should be part of compatibility=grub2. (Though your assumption, which I think is reasonable, that compatibilty=grub2 is for /boot on ZFS makes them essentially the same.)

  1. Probably compatible with GRUB, not currently recommended for boot pool:

    • allocation_classes
    • device_rebuild
    • log_spacemap
    • project_quota
    • resilver_defer

My question is: would any of the features in the final group here be actively harmful to include in compatibility.d/grub2 (and therefore probably being enabled by default in any installations which follow this guide)?

Those should be safe. But I can't remember what all I've tested and what the results were. That's why I try to document these things, like I did with userobj_accounting.

Also, when testing features, it's easy to test with them "enabled", but we really need to test with them "active". So you'd have to make a boot pool and put a "special" device in it to test allocation_classes (which I think I might have done and would likely be fine), setup a deferred resilver situation (however that works) for "resilver_defer", etc.

My personal opinion favors maximal liberalism; I think there's a benefit to enabling as many features as possible on the boot pool and only excluding features which are known to break GRUB. Features being enabled is almost definitely the most common case (that's why they're called features, and why zpool status flags them as being available).

I've been taking the "maximal liberalism" approach. Like you, my thinking is that we only exclude them if they're known to not work. On the other hand, this leads to enabling a long list of features of questionable utility. The work of hand-enabling them goes away with the "compatibility" option though. We are still taking some risk by enabling features. This is what is making me start leaning towards more conservative.

@colmbuckley
Copy link
Contributor

I am absolutely inclined to defer to your expertise and judgement on this; so I suggest leaving the contents of the compatibility file alone for now, post- adding livelist and zpool-checkpoint. There's nothing stopping a given system administrator from using a different compatibility file if they feel the need for an additional feature (or opening a PR against openzfs and fighting the good fight there...)

Hopefully we can go with just -o compatibility=grub2 for the bookworm version of this document. Not sure whether my recent PR will make the cut for the release (probably not), but it'll be included soon enough I suspect. Either way, the impact will be negligible.

@darkbasic darkbasic mentioned this pull request Jun 21, 2023
Closed
@darkbasic
Copy link

darkbasic commented Jul 5, 2023 •
edited
Loading

On Bookworm:

root@debian:~# zpool create \
    -o ashift=12 \
    -o autotrim=on -d \
    -o cachefile=/etc/zfs/zpool.cache \
    -o feature@livelist=enabled \
    -o feature@zpool_checkpoint=enabled \
    -o compatibility=grub2 \
    -O devices=off \
    -O acltype=posixacl -O xattr=sa \
    -O compression=lz4 \
    -O normalization=formD \
    -O relatime=on \
    -O canmount=off -O mountpoint=/boot -R /mnt \
    bpool ${DISK}3
Warning: feature "zpool_checkpoint" enabled but is not in specified 'compatibility' feature set.
Warning: feature "livelist" enabled but is not in specified 'compatibility' feature set.
cannot create 'bpool': operation not supported on this type of pool

@colmbuckley
Copy link
Contributor

colmbuckley commented Jul 5, 2023 via email •
edited
Loading

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

@rlaager rlaager

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jacobgreenleaf @colmbuckley @gmelikov @rlaager @darkbasic