Quoting Warner Losh <i...@bsdimp.com> (from Wed, 9 Nov 2022 08:54:33 -0700):
On Wed, Nov 9, 2022 at 5:46 AM Alexander Leidinger <alexan...@leidinger.net> wrote:Quoting Alexander Leidinger <alexan...@leidinger.net> (from Tue, 08 Nov 2022 10:50:53 +0100):
> Should the above list be sorted in some way? Maybe in the same order > as the zpool-features lists them (sort by feature name after the > colon), or alphabetical? Is it OK if I commit this alphabetical sorting?
[diff of feature-sorting]
This patch looks good because it's a nop and just tidies things up a bit. Reviewed by: imp
Will do later.
> As Mark already mentioned some flags, I checked the features marked > as read only (I checked in the zpool-features man page, including > the dependencies documented there) and here are those not listed in > zfsimpl.c. I would assume as they are read-only compatible, we > should add them: > com.delphix:async_destroy > com.delphix:bookmarks > org.openzfs:device_rebuild > com.delphix:empty_bpobj > com.delphix:enable_txg > com.joyent:filesystem_limits > com.delphix:livelist > com.delphix:log_spacemap > com.zfsonlinux:project_quota > com.zfsonlinux:userobj_accounting > com.openzfs:zilsaxattr If my understanding is correct that the read-only compatible parts (according to the zpool-features man page) are safe to add to the zfs boot, here is what I have only build-tested (relative to the above alphabetical sorting): ---snip--- --- zfsimpl.c_sorted 2022-11-09 12:55:06.346083000 +0100 +++ zfsimpl.c 2022-11-09 13:01:24.083364000 +0100 @@ -121,24 +121,35 @@ "com.datto:bookmark_v2", "com.datto:encryption", "com.datto:resilver_defer", + "com.delphix:async_destroy", "com.delphix:bookmark_written", + "com.delphix:bookmarks", "com.delphix:device_removal", "com.delphix:embedded_data", + "com.delphix:empty_bpobj", + "com.delphix:enable_txg", "com.delphix:extensible_dataset", "com.delphix:head_errlog", "com.delphix:hole_birth", + "com.delphix:livelist", + "com.delphix:log_spacemap", "com.delphix:obsolete_counts", "com.delphix:spacemap_histogram", "com.delphix:spacemap_v2", "com.delphix:zpool_checkpoint", "com.intel:allocation_classes", + "com.joyent:filesystem_limits", "com.joyent:multi_vdev_crash_dump", + "com.openzfs:zilsaxattr", + "com.zfsonlinux:project_quota", + "com.zfsonlinux:userobj_accounting", "org.freebsd:zstd_compress", "org.illumos:lz4_compress", "org.illumos:sha512", "org.illumos:skein", "org.open-zfs:large_blocks", "org.openzfs:blake3", + "org.openzfs:device_rebuild", "org.zfsonlinux:allocation_classes", "org.zfsonlinux:large_dnode", NULL ---snip--- Anyone able to test some of those or confirms my understanding is correct and would sign-off on a "reviewed by" level?I'm inclined to strongly NAK this patch, absent some way to test it. There's no issues today with any of them being absent causing problems on boot that have been reported. The ZFS that's in the boot loader is a reduced copy of what's in base and not everything is supported. There's no urgency here to rush into this. The ones that are on the list already are for things that we know we support in the boot loader because we've gone to the trouble to put blake3 or sha512 into it (note: Not all boot loaders will support all ZFS features in the future... x86 BIOS booting likely is going to have to be frozen at its current ZFS feature set due to code size issues). While most of these options look OK on the surface, I'd feel a lot better if there were tests for these to prove they work. I'd also feel better if the ZFS experts could explain how those come to be set on a zpool as well. I'd settle for a good script that could be run as root (better would be not as root) that would take a filesystem that was created by makefs -t zfs and turn on these features after an zpool upgrade. I have the vague outlines of a test suite for the boot loader that I could see about integrating something like that into, but most of my time these days is chasing after 'the last bug' in some kboot stuff I'm working on (which includes issues with our ZFS in the boot loader integration). So not a hard no, but I plea for additional scripts to create images that can be tested.
I didn't want to commit untested or unverified stuff. I fully agree with your reasoning.
Bye, Alexander. -- http://www.Leidinger.net alexan...@leidinger.net: PGP 0x8F31830F9F2772BF http://www.FreeBSD.org netch...@freebsd.org : PGP 0x8F31830F9F2772BF
pgppq4Jrt1La5.pgp
Description: Digitale PGP-Signatur
