openzfs
zpool create: warn on suboptimal pool layout
Motivation and Context
Its possible to create pools that are perfectly valid but are perhaps not the "best" choice for a given set of hardware.
An example is a raidz1 of two devices. I have seen inexperienced users create this because it looks on the surface like a traditional RAID-1, that is, a mirror. It even appears to work, but presents problems later when they want to upgrade the drives, and of course does not perform as well as a mirror.
It seems to me that if we can guide users towards the "right" way to use ZFS without too much effort, we should try, and this is an attempt at that.
Description
This changes zpool create to reject such "suboptimal" pool layouts, and suggest a possible better alternative. It adds a switch, --force-layout, to disable the check and the warning and return the old behaviour, for those who know what they're doing.
Only the above example is checked for, but with the structure in place it'll be easy to add more as we like.
How Has This Been Tested?
New tests added for the new feature.
Many existing tests attempt to create raidz1 vdevs with two devices, and those are now failing. Its not hard to identify and fix them by adding --force-layout, but that feels brittle. An alternative solution might be an environment variable to disable the checks (or maybe one of the test suite environment variables). Suggestions welcome.
Types of changes
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] 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)
- [x] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
- [x] Documentation (a change to man pages or other documentation)
Checklist:
- [x] My code follows the OpenZFS code style requirements.
- [x] I have updated the documentation accordingly.
- [x] I have read the contributing document.
- [x] I have added tests to cover my changes.
- [ ] I have run the ZFS Test Suite with this change applied.
- [x] All commit messages are properly formatted and contain
Signed-off-by.
Could I propose you change it from --force-layout to --force=layout, the better to extend it with more fine-grained force parameters later without requiring a sea of --force-a --force-b --force-c?
Maybe I'm alone in that preference, though. :(
edit: Might also be worth noting that raidz1 is not a mirror in the output, since a lot of people I've encountered who did this seemed to assume "raidz" meant 'ZFS magic raid of any kind".
Could I propose you change it from --force-layout to --force=layout
This would be nice since it finally provides a consist way to override a warnings by type. For example, creating a pool with different levels of parity among the top-level vdev needs to be forced. We've long wanted to be able to override just this warning and not all warnings.
I'm not sure which I prefer, but --allow=layout would be another option.
Thanks for the feedback.
I'm kind of indifferent to --force=... just for this PR, but I can certainly see the utility of a general facility to be able to express this kind of thing. So I'll put this hold until I can get around to putting that together. This weekend I hope.
That helps with the test case too; I think I'll make it default to the ZPOOL_FORCE= environment variable (or something of that shape).
Being able to pick and choose what -f overrides has been on my wish list since 2016.
I am even kind of partial to the --allow= instead of force.
for zpool create and zpool add
- different number of members
- different raidz level
- mix of raidz and not
- different disk sizes
- disk was part of a previous pool etc
so something like: --allow=count,used to cover 1 and 5, but NOT 4
and for import even, --allow=hostid to suppress that one check, but not others.
Alright, latest push sets up --force as a set of comma-separated flags, and adds two: vdevs, which is the old -f behaviour, and layout, which is the override flag for this PR.
root@quiz:/# zpool create tank raidz1 loop0 loop1
suboptimal vdev specification: a 'raidz1' vdev with 2 devices is inefficient; consider 'mirror' instead.
use '--force=layout' to ignore this.
root@quiz:/# zpool create --force=layout tank raidz1 loop0 loop1
root@quiz:/# zpool export -a
root@quiz:/# zpool create tank raidz1 loop0 loop1
invalid vdev specification
use '-f' to override the following errors:
/dev/loop0 is part of exported pool 'tank'
/dev/loop1 is part of exported pool 'tank'
root@quiz:/# zpool create -f tank raidz1 loop0 loop1
suboptimal vdev specification: a 'raidz1' vdev with 2 devices is inefficient; consider 'mirror' instead.
use '--force=layout' to ignore this.
root@quiz:/# zpool create --force=layout tank raidz1 loop0 loop1
invalid vdev specification
use '-f' to override the following errors:
/dev/loop0 is part of exported pool 'tank'
/dev/loop1 is part of exported pool 'tank'
root@quiz:/# zpool create --force=vdevs,layout tank raidz1 loop0 loop1
root@quiz:/# zpool status
pool: tank
state: ONLINE
config:
NAME STATE READ WRITE CKSUM
tank ONLINE 0 0 0
raidz1-0 ONLINE 0 0 0
loop0 ONLINE 0 0 0
loop1 ONLINE 0 0 0
errors: No known data errors
There's a new utility function, zpool_option_flag_apply(), which is set up to play nicely with long options. I haven't used it anywhere else (we don't have anything else yet!) and it would be better as a common function for zfs as well, but we don't have any shared code like that really. That can come later.
Tests will still fail because they try to make "suboptimal" pools in places. Probably I will just add --force=layout in any places that will definitely cause problems, since it can now be specified precisely. Possibly better would be an environment variable eg ZPOOL_FORCE=layout that is automatically hooked up, but I wouldn't want to do that without some agreement that there will be a common set of forcible things across all subcommands, so it makes sense in the future.
The flag parsing and the suboptimal layout warning are of course unrelated, and I would ordinarily split something like this into two PRs, but just adding the utility function in its own PR without anything using it is kind of equally silly, so it stays here for now.
Anyway, all comments welcome!