Improvements to the 'compatibility' property.

[colmbuckley]

Several improvements to the operation of the 'compatibility' property:

1. Improved handling of unrecognized features:
   Change the way unrecognized features in compatibility files are handled.

• invalid features in files under /usr/share/zfs/compatibility.d
  only get a warning (as these may refer to future features not yet in
  the library),
• invalid features in files under /etc/zfs/compatibility.d
  get an error (as these are presumed to refer to the current system).

2. Improved error reporting from zpool_load_compat.
   Note: slight ABI change to zpool_load_compat for better error reporting.

3. compatibility=legacy inhibits all 'zpool upgrade' operations.

4. Detect when features are enabled outside current compatibility set

   • zpool set compatibility=foo <-- print a warning
   • zpool set feature@xxx=enabled <-- error
   • zpool status <-- indicate this state

Signed-off-by: Colm Buckley [email protected]

Motivation and Context

Description

How Has This Been Tested?

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)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[colmbuckley]

Some examples:

root@zfstest:~# zpool create -f -o compatibility=grub2 tpool /root/testpool
root@zfstest:~# zpool status tpool
  pool: tpool
 state: ONLINE
config:

        NAME              STATE     READ WRITE CKSUM
        tpool             ONLINE       0     0     0
          /root/testpool  ONLINE       0     0     0

errors: No known data errors
root@zfstest:~# zpool set feature@large_dnode=enabled tpool
Error: cannot enable feature 'large_dnode' on pool 'tpool' as it is not specified in this pool's
current compatibility set.
Consider setting 'compatibility' to a less restrictive set, or to 'off'.
root@zfstest:~# zpool set compatibility=off tpool
root@zfstest:~# zpool set feature@large_dnode=enabled tpool
root@zfstest:~# zpool set compatibility=grub2 tpool
Warning: one or more features already enabled on pool 'tpool'
are not present in this compatibility set.
root@zfstest:~# zpool status tpool
  pool: tpool
 state: ONLINE
status: One or more features are enabled on the pool despite not being
        requested by the 'compatibility' property.
action: Consider setting 'compatibility' to an appropriate value, or
        adding needed features to the relevant file in
        /etc/zfs/compatibility.d or /usr/share/zfs/compatibility.d.
config:

        NAME              STATE     READ WRITE CKSUM
        tpool             ONLINE       0     0     0
          /root/testpool  ONLINE       0     0     0

errors: No known data errors

[colmbuckley]

A few comments:

• set_callback() (in zpool_main.c) seems to be the most appropriate place to check for setting out-of-bounds features. I first considered putting this check into zpool_valid_proplist but the error reporting for the various use cases of that function quickly became cumbersome.
• I decided to allow enabling out-of-bounds features during zpool create as this is unlikely to be done unintentionally. This will print a warning and put the pool straight into ZPOOL_STATUS_INCOMPATIBLE_FEAT status, but will still succeed.
• On the other hand, enabling an out-of-bounds feature using zpool set feature@xx=enabled will cause an error, as its more likely that this is accidental. The feature can still be enabled by setting compatibility=off and then enabling it.
• Setting compatibility to something more restrictive than the currently-enabled features is allowed (with a warning).

I played around with various combinations of silence, warning and error for each of these situations; the above is my best guess as to the best balance of usability and safety. Let me know what you think.

[colmbuckley]

Little bit more on my thinking around errors vs warnings:

I think we should throw an error when the user does something:

• irreversible, and
• possibly accidental

while actions which are easily reversible should only show warnings, even if contrary to the user's apparent intent.

So the following actions will show errors:

• Enabling a new feature outside what the current compatibility property allows
  • this is irreversible
• Setting compatibility to a value which has invalid feature data in /etc/zfs/compatibility.d
  • these files are likely maintained by the local administrator, so an invalid feature is probably an accident
• Performing a zpool upgrade while compatibility=legacy
  • irreversible, clearly against the intention of the compatibility property
  • similar to how individual feature upgrades are inhibited by various compatibility settings

And the following actions will show only warnings:

• Specifying an additional feature (beyond compatibility) at creation time
  • this is unlikely to be accidental and, while it is irreversible, the new pool won't yet contain any critical data; the admin can simply re-create it with different features if needed
• Setting compatibility to a more-restrictive value even if out-of-bounds features are currently enabled
  • easily reversible
  • sometimes needed (eg: to deliberately enable "one additional feature", or for testing)
  • less likely to be accidental

[behlendorf]

Thanks for following up with these improvements. I agree, what you're proposing here strikes me a pretty good balance between between usability and safety. When testing this locally everything worked well and the behavior was in line with what I would expect as a normal user.

This looks good to me. While this does contain an small ABI change I think we can still reasonably get this in for the next release candidate. Thanks!

[colmbuckley]

Cool. Let me know if you need a rebase against head before merging.

[behlendorf]

@colmbuckley thanks, yes would you mind rebasing this. We had another small ABI change to libzfs which caused a conflict.

[colmbuckley]

@colmbuckley thanks, yes would you mind rebasing this. We had another small ABI change to libzfs which caused a conflict.

Done.