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

WIP: Reorder and regroup options #308

Closed
wants to merge 3 commits into from

Conversation

anarcat
Copy link
Contributor

@anarcat anarcat commented May 20, 2022

So it seems most of my ideas of #307 were refused, which is fine. But there's one final idea that I think might reach consensus here, and it's that options should be regrouped together consistently and logically. For example, the posixacl, xattr and dnodesize options go together, relatime and normalization do not.

This PR attempts to clean that up in one example instructions, the Bullseye one that I am more familiar with. Ideally this would be done elsewhere, but I'm not going to waste any time on that until this is somewhat agreed on here. :)

Thanks!

@rlaager rlaager self-assigned this May 21, 2022
Copy link
Member

@rlaager rlaager left a comment

Choose a reason for hiding this comment

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

This seems fine to me in principle.

  • Please make similar changes to the Ubuntu 22.04 HOWTOs. They're close enough that often this can be git diff Debian/*Bullseye* | patch "Ubuntu/Ubuntu 22.04 Root on ZFS.rst" If the patch applies cleanly (or you want to do it by hand), you could do Debian Buster and Ubuntu 20.04 too, but that's less important. My main goal is to keep the current versions up-to-date.
  • Please add Signed-off-by to the commits.
  • dnodesize=auto is intentionally not used on the bpool. Note that large_dnode is not enabled there. (This is slightly harder to tell now that we're using compatibility=grub2.)
  • I see you reordered in the Notes section to keep them in the order they are listed in the command. Do you think we should keep using the order listed, or alphabetize them in the notes? Either way will require moving some options.

This regroups the options passed to zpool create in a more logical
way.

 1. the ACL/xattr/dnodesize settings are all on one line, since they
    are all related (latter being optimizations because we enable the
    former)

 2. ashift, autotrim, compression, normalization, relatime are all
    independent options that merit their own line for better
    visibility, they are also optional and therefore it makes sense to
    have them separate

 3. canmount, mountpoint and -R are all related as well

This is only a "whitespace" fix: there should be no functional change
to this patch.

I also noted that dnodesize=auto is *not* passed to the bpool cache
creation, even though it's passed to the rpool creation. I suspect
that might be an omission. This also goes to show that ordering those
options consistently makes it easier to spot those errors.

This is a followup to openzfs#307.

Signed-off-by: Antoine Beaupré <anarcat@debian.org>
We stick to two rules here:

 * -o options come before -O

 * pool-specific options are wedged in between -o and
   -O (e.g. compatibility=grub2)

I don't know if that was a deliberate policy, I'm just trying to guess
what the pattern was

Signed-off-by: Antoine Beaupré <anarcat@debian.org>
Signed-off-by: Antoine Beaupré <anarcat@debian.org>
@anarcat
Copy link
Contributor Author

anarcat commented May 24, 2022

Please make similar changes to the Ubuntu 22.04 HOWTOs. They're close enough that often this can be git diff Debian/*Bullseye* | patch "Ubuntu/Ubuntu 22.04 Root on ZFS.rst" If the patch applies cleanly (or you want to do it by hand), you could do Debian Buster and Ubuntu 20.04 too, but that's less important. My main goal is to keep the current versions up-to-date.

Done with 22.04.

Please add Signed-off-by to the commits.

done.

dnodesize=auto is intentionally not used on the bpool. Note that large_dnode is not enabled there. (This is slightly harder to tell now that we're using compatibility=grub2.)

... okay. :)

I see you reordered in the Notes section to keep them in the order they are listed in the command. Do you think we should keep using the order listed, or alphabetize them in the notes? Either way will require moving some options.

i frankly have no idea. i tried to follow the order that was already present to not break anything.

i think having the lines ordered alphabetically would make sense, but that seems out of scope.

@rlaager
Copy link
Member

rlaager commented Jun 9, 2022

Merged as 726f07a. Thanks!

@rlaager rlaager closed this Jun 9, 2022
@anarcat anarcat deleted the reorder-regroup branch June 16, 2022 14:04
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.

2 participants