Current zfs reader in loader does only check label features, which is not enough
to ensure the pool is really usable for booting. As new features are being
implemented and not yet implemented in boot loader, the zfs code should
be verbose about unsupported features and not fail with misleading error
messages.
Details
Details
Tested with illumos pool which has skein active - loader does detect this
and exclude this pool from device list (lsdev).
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Request for a few changes.
| sys/boot/zfs/libzfs.h | ||
|---|---|---|
| 68 ↗ | (On Diff #17617) | Second parameter should have __unused too. |
| sys/boot/zfs/zfsimpl.c | ||
| 1580 ↗ | (On Diff #17617) | Please use __unused and sys/cdefs.h. |
| 1875 ↗ | (On Diff #17617) | name shouldn't be necessary here. |
| 2029 ↗ | (On Diff #17617) | no space after strcmp (see style(9)). |
| 2056 ↗ | (On Diff #17617) | Please remove this. |
| 2088 ↗ | (On Diff #17617) | I think this would be too verbose (only useful for debugging)? i.e. why a system administrator care if the data pool is not supported by the boot block? |
| sys/boot/zfs/zfsimpl.c | ||
|---|---|---|
| 2088 ↗ | (On Diff #17617) | If someone like me is using different boot pools in same system, this is useful feedback, so I don't have to guess why some of the pools are not visible. Or even worse, if my only boot pool is rendered unusable for booting, it would be good to get some verbose message. That was the reason why I did insert this printf; also in feature check callback there is no access to pool name, otherwise I would have used it in check_feature(). |
| sys/boot/zfs/zfsimpl.c | ||
|---|---|---|
| 2088 ↗ | (On Diff #17617) | Can we check for the 'bootfs' property? If a pool doesn't have that set, no one is expecting to boot from it, but if it does have it, they should get an error. |
| sys/boot/zfs/zfsimpl.c | ||
|---|---|---|
| 2088 ↗ | (On Diff #17617) | Thats one possible option, but not entirely correct as well, because bootfs is just an hint for boot loader to support automatic load and boot, but human operator can still just set currdev and go;) TBH, I wouldn't really worry too much about the message itself, because a) beastie will hide it (poor excuse;), and well, on more serious note, we can get full feature support for loader, if we really want to - in fact, the illumos port I'm working on, already does have *full* current feature set supported - gzip, sha512, edonr, skein. Technically its no problem to support them, there are some other decisions to be made, however. |
Comment Actions
@tsoome_me.com, do you have a commit bit? Or do you depend on a committer to commit this change?
Comment Actions
Not sure if this is intentional, but if I have a dataset (not the one that I boot from) that has skein as the checksum, so the feature is 'active', this change makes booting fail "ZFS: unsupported feature: org.illumos:skein"
However, before this patch, it booted fine, as the feature was not active on the root dataset, only a secondary dataset that is not used during boot.
Comment Actions
This works as expected.
While this will fail to boot a pool that has activated a new unsupported feature, the zfs userland tools prevent a user from activating those features if the bootfs zpool property is set, so a user is protected from shooting off their own foot.