From nobody Wed Nov 9 15:54:33 2022 X-Original-To: current@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4N6qKf0r30z4d989 for ; Wed, 9 Nov 2022 15:54:46 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ej1-x634.google.com (mail-ej1-x634.google.com [IPv6:2a00:1450:4864:20::634]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4N6qKd6xGCz4LJ5 for ; Wed, 9 Nov 2022 15:54:45 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ej1-x634.google.com with SMTP id k2so47910424ejr.2 for ; Wed, 09 Nov 2022 07:54:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=13tPxxBOugeaIMyUngvHrAuU0+t7XVxunInCMO8D008=; b=7cj5JaWv2sVoHoCvAHSLkJgZey36StKVLIHN4IOGuFf+CasZrdbEu0LNsIKCNqXQus vw/vxmcoKDjVxALzDb46zCtkF6ua2lH9WjmEYXm8idJCk0h6HpTd3NUuoeVq+tItM2Mw KKaQGzCCmHjL9ORDqPnip34apDV3uS9Ehg3LYDd2sE/TTb9mdOZ9Qjhu3nrAgOa8Sy8S DE+Wgw3y9BvGQTAX0yZDovhMaz5Up9UjGPQx8oeZbs14g5PQ40MiEO9mduUYXpFkOpUs O8GECOuqYoeC2vZSY3v0cMS/8AKGpn6gcDckYyZuli7hzXCRsNQTyXLim+kVrSV5Wc10 KhaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=13tPxxBOugeaIMyUngvHrAuU0+t7XVxunInCMO8D008=; b=r/RZytlj4NIJMe3So8RoiaubQmP9RTUg19ZEolbM/lmQd2hYLwmZQTl67aCRyvaXDY OLB5cWvuZ0Ib3zJJgcptQ9mmi8BxXazSPzCgqDL6qysgcPQMgZT2IiuRKnIsNC0b2luk OWpIQ+zyeJw3KU/08Vdr5WP8Qz0ZW1aRTtmQgolJxEaX1uVIwIWGr6gkL+M4EnDY8xFN YrZx2g1/NmqL3pjR38UG6Bqqr7LBLMtbYiGX+8A5vGc3KbK8ICgr7jZwpjfaCDECf68X TOfO6tgt5LhukSTpO3Dc9AuukE9xAZvFtMW3EyInwvCfvZRqRgNKkEFd/0QPDxLI8eIC X2fw== X-Gm-Message-State: ACrzQf1YCQUvCGuok51IRdSQ/pfyqx2928nvKpCtxHR9zK8O/nzsXZg4 K2tg5nIn5uudmqTS/mWaWdtckakLXSD7qsaSwMIgYEoWeck= X-Google-Smtp-Source: AMsMyM4YlmnvWbrqJ4Av/wuhRVtvuAVjXFCMV62BSFp20iYI2yJJdqR8PBmJpkRyiejKpOca+UvvBFjj/guBQj4eamI= X-Received: by 2002:a17:906:3b17:b0:7ad:b645:9e3e with SMTP id g23-20020a1709063b1700b007adb6459e3emr53327255ejf.140.1668009284557; Wed, 09 Nov 2022 07:54:44 -0800 (PST) List-Id: Discussions about the use of FreeBSD-current List-Archive: https://lists.freebsd.org/archives/freebsd-current List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-current@freebsd.org MIME-Version: 1.0 References: <202211070339.2A73dJlO027991@gitrepo.freebsd.org> <20221107121514.Horde.nulS9Wg1s3yzAsXXkuJRBa9@webmail.leidinger.net> <20221108105053.Horde.eqgFiBJe2ngGAj6BkXcv5-Z@webmail.leidinger.net> <20221109134610.Horde.JB7ibQTWprHbmIUfhg7JY7f@webmail.leidinger.net> In-Reply-To: <20221109134610.Horde.JB7ibQTWprHbmIUfhg7JY7f@webmail.leidinger.net> From: Warner Losh Date: Wed, 9 Nov 2022 08:54:33 -0700 Message-ID: Subject: Re: changes to the zfs boot (was: Re: git: 72a1cb05cd23 - main - rc(8): Add a zpoolupgrade rc.d script) To: Alexander Leidinger Cc: marklmi@yahoo.com, tsoome@freebsd.org, Li-Wen Hsu , current@freebsd.org Content-Type: multipart/alternative; boundary="000000000000e37acf05ed0bacb8" X-Rspamd-Queue-Id: 4N6qKd6xGCz4LJ5 X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US] X-ThisMailContainsUnwantedMimeParts: N --000000000000e37acf05ed0bacb8 Content-Type: text/plain; charset="UTF-8" On Wed, Nov 9, 2022 at 5:46 AM Alexander Leidinger wrote: > Quoting Alexander Leidinger (from Tue, 08 > Nov 2022 10:50:53 +0100): > > > Quoting Warner Losh (from Mon, 7 Nov 2022 14:23:11 > -0700): > > > >> > >> > >> On Mon, Nov 7, 2022 at 4:15 AM Alexander Leidinger > >> wrote: > >> > >>> Quoting Li-Wen Hsu (from Mon, 7 Nov 2022 03:39:19 > GMT): > >>> > >>>> The branch main has been updated by lwhsu: > >>>> > >>>> URL: > >>>> > https://cgit.FreeBSD.org/src/commit/?id=72a1cb05cd230ce0d12a7180ae65ddbba2e0cb6d > >>>> > >>>> commit 72a1cb05cd230ce0d12a7180ae65ddbba2e0cb6d > >>>> Author: Li-Wen Hsu > >>>> AuthorDate: 2022-11-07 03:30:09 +0000 > >>>> Commit: Li-Wen Hsu > >>>> CommitDate: 2022-11-07 03:30:09 +0000 > >>>> > >>>> rc(8): Add a zpoolupgrade rc.d script > >>>> > >>>> If a zpool is created by makefs(8), its version is 5000, i.e., > all > >>>> feature flags are off. Introduce an rc script to run `zpool > upgrade` > >>>> over the assigned zpools on the first boot. This is useful to > the > >>>> ZFS based VM images built from release(7). > >>> > >>>> diff --git a/share/man/man5/rc.conf.5 b/share/man/man5/rc.conf.5 > >>>> index f9ceabc83120..43fa44a5f1cb 100644 > >>>> --- a/share/man/man5/rc.conf.5 > >>>> +++ b/share/man/man5/rc.conf.5 > >>>> @@ -24,7 +24,7 @@ > >>>> .\" > >>>> .\" $FreeBSD$ > >>>> .\" > >>>> -.Dd August 28, 2022 > >>>> +.Dd November 7, 2022 > >>>> .Dt RC.CONF 5 > >>>> .Os > >>>> .Sh NAME > >>>> @@ -2109,6 +2109,13 @@ A space-separated list of ZFS pool names for > >>>> which new pool GUIDs should be > >>>> assigned upon first boot. > >>>> This is useful when using a ZFS pool copied from a template, such > >>>> as a virtual > >>>> machine image. > >>>> +.It Va zpool_upgrade > >>>> +.Pq Vt str > >>>> +A space-separated list of ZFS pool names for which version should > >>>> be upgraded > >>>> +upon first boot. > >>>> +This is useful when using a ZFS pool generated by > >>>> +.Xr makefs 8 > >>>> +utility. > >>> > >>> For someone who knows ZFS well, it is clear that only a zpool upgrade > >>> is done. Not so experienced people may assume there is a combination > >>> of zpool upgrade and zfs upgrade (more so for people which do not know > >>> what the difference is). Maybe you want to add some explicit > >>> documentation, that zfs upgrade + feature flags needs to be done by > >>> hand. > >>> > >>> And this brings me to a second topic, we don't have an explicit list > >>> of features which are supported by the bootloader (I had a look at the > >>> zfs and the boot related man pages, if I overlooked a place, then the > >>> other places should reference this important part with some text). > >> > >> > >> There is a fixed list of features we support in the boot loader: > >> > >> /* > >> * List of ZFS features supported for read > >> */ > >> static const char *features_for_read[] = { > >> "org.illumos:lz4_compress", > >> "com.delphix:hole_birth", > >> "com.delphix:extensible_dataset", > >> "com.delphix:embedded_data", > >> "org.open-zfs:large_blocks", > >> "org.illumos:sha512", > >> "org.illumos:skein", > >> "org.zfsonlinux:large_dnode", > >> "com.joyent:multi_vdev_crash_dump", > >> "com.delphix:spacemap_histogram", > >> "com.delphix:zpool_checkpoint", > >> "com.delphix:spacemap_v2", > >> "com.datto:encryption", > >> "com.datto:bookmark_v2", > >> "org.zfsonlinux:allocation_classes", > >> "com.datto:resilver_defer", > >> "com.delphix:device_removal", > >> "com.delphix:obsolete_counts", > >> "com.intel:allocation_classes", > >> "org.freebsd:zstd_compress", > >> "com.delphix:bookmark_written", > >> "com.delphix:head_errlog", > >> "org.openzfs:blake3", > >> NULL > >> }; > >> > >> Any feature not on this list will cause the boot loader to > >> reject the pool. > >> > >> Whether or not it should do that by default, always, or never is an > open > >> question. I've thought there should be a 'shoot footing' > >> override that isn't > >> there today. > >> > > > > Thanks for the list. For those interested, it is in > > $SRC/stand/libsa/zfs/zfsimpl.c > > > > Just to make my opinion expressed before explicit again, this should > > be documented in a boot / bootloader related man-page, but isn't. > > > > 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? > ---snip--- > diff --git a/stand/libsa/zfs/zfsimpl.c b/stand/libsa/zfs/zfsimpl.c > index 6b961f3110a..36c90613e82 100644 > --- a/stand/libsa/zfs/zfsimpl.c > +++ b/stand/libsa/zfs/zfsimpl.c > @@ -118,29 +118,29 @@ static vdev_list_t zfs_vdevs; > * List of ZFS features supported for read > */ > static const char *features_for_read[] = { > - "org.illumos:lz4_compress", > - "com.delphix:hole_birth", > - "com.delphix:extensible_dataset", > - "com.delphix:embedded_data", > - "org.open-zfs:large_blocks", > - "org.illumos:sha512", > - "org.illumos:skein", > - "org.zfsonlinux:large_dnode", > - "com.joyent:multi_vdev_crash_dump", > - "com.delphix:spacemap_histogram", > - "com.delphix:zpool_checkpoint", > - "com.delphix:spacemap_v2", > - "com.datto:encryption", > "com.datto:bookmark_v2", > - "org.zfsonlinux:allocation_classes", > + "com.datto:encryption", > "com.datto:resilver_defer", > + "com.delphix:bookmark_written", > "com.delphix:device_removal", > + "com.delphix:embedded_data", > + "com.delphix:extensible_dataset", > + "com.delphix:head_errlog", > + "com.delphix:hole_birth", > "com.delphix:obsolete_counts", > + "com.delphix:spacemap_histogram", > + "com.delphix:spacemap_v2", > + "com.delphix:zpool_checkpoint", > "com.intel:allocation_classes", > + "com.joyent:multi_vdev_crash_dump", > "org.freebsd:zstd_compress", > - "com.delphix:bookmark_written", > - "com.delphix:head_errlog", > + "org.illumos:lz4_compress", > + "org.illumos:sha512", > + "org.illumos:skein", > + "org.open-zfs:large_blocks", > "org.openzfs:blake3", > + "org.zfsonlinux:allocation_classes", > + "org.zfsonlinux:large_dnode", > NULL > }; > ---snip--- > This patch looks good because it's a nop and just tidies things up a bit. Reviewed by: imp > > 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. Warner > Bye, > Alexander. > -- > http://www.Leidinger.net Alexander@Leidinger.net: PGP 0x8F31830F9F2772BF > http://www.FreeBSD.org netchild@FreeBSD.org : PGP 0x8F31830F9F2772BF > --000000000000e37acf05ed0bacb8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Wed, Nov 9, 2022 at 5:46 AM Alexan= der Leidinger <Alexander@leid= inger.net> wrote:
Quoting Alexander Leidinger <Alexander@leidinger.net> (from Tue, 08= =C2=A0
Nov 2022 10:50:53 +0100):

> Quoting Warner Losh <imp@bsdimp.com> (from Mon, 7 Nov 2022 14:23:11 -0700):
>
>> =C2=A0
>>
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0On Mon, Nov 7, 2022 at 4:15 AM Alexander= Leidinger=C2=A0
>> <A= lexander@leidinger.net> wrote:
>>
>>> Quoting Li-Wen Hsu <lwhsu@freebsd.org> (from Mon, 7 Nov 2022 03:39:19 GM= T):
>>>
>>>> The branch main has been updated by lwhsu:
>>>>
>>>> URL:=C2=A0
>>>> h= ttps://cgit.FreeBSD.org/src/commit/?id=3D72a1cb05cd230ce0d12a7180ae65ddbba2= e0cb6d
>>>>
>>>> commit 72a1cb05cd230ce0d12a7180ae65ddbba2e0cb6d
>>>> Author:=C2=A0 =C2=A0 =C2=A0Li-Wen Hsu <lwhsu@FreeBSD.or= g>
>>>> AuthorDate: 2022-11-07 03:30:09 +0000
>>>> Commit:=C2=A0 =C2=A0 =C2=A0Li-Wen Hsu <lwhsu@FreeBSD.or= g>
>>>> CommitDate: 2022-11-07 03:30:09 +0000
>>>>
>>>> =C2=A0 =C2=A0 =C2=A0rc(8): Add a zpoolupgrade rc.d script<= br> >>>>
>>>> =C2=A0 =C2=A0 =C2=A0If a zpool is created by makefs(8), it= s version is 5000, i.e., all
>>>> =C2=A0 =C2=A0 =C2=A0feature flags are off.=C2=A0 Introduce= an rc script to run `zpool upgrade`
>>>> =C2=A0 =C2=A0 =C2=A0over the assigned zpools on the first = boot.=C2=A0 This is useful to the
>>>> =C2=A0 =C2=A0 =C2=A0ZFS based VM images built from release= (7).
>>>
>>>> diff --git a/share/man/man5/rc.conf.5 b/share/man/man5/rc.= conf.5
>>>> index f9ceabc83120..43fa44a5f1cb 100644
>>>> --- a/share/man/man5/rc.conf.5
>>>> +++ b/share/man/man5/rc.conf.5
>>>> @@ -24,7 +24,7 @@
>>>> =C2=A0 .\"
>>>> =C2=A0 .\" $FreeBSD$
>>>> =C2=A0 .\"
>>>> -.Dd August 28, 2022
>>>> +.Dd November 7, 2022
>>>> =C2=A0 .Dt RC.CONF 5
>>>> =C2=A0 .Os
>>>> =C2=A0 .Sh NAME
>>>> @@ -2109,6 +2109,13 @@ A space-separated list of ZFS pool = names for=C2=A0
>>>> which new pool GUIDs should be
>>>> =C2=A0 assigned upon first boot.
>>>> =C2=A0 This is useful when using a ZFS pool copied from a = template, such=C2=A0
>>>> as a virtual
>>>> =C2=A0 machine image.
>>>> +.It Va zpool_upgrade
>>>> +.Pq Vt str
>>>> +A space-separated list of ZFS pool names for which versio= n should=C2=A0
>>>> be upgraded
>>>> +upon first boot.
>>>> +This is useful when using a ZFS pool generated by
>>>> +.Xr makefs 8
>>>> +utility.
>>>
>>> For someone who knows ZFS well, it is clear that only a zpool = upgrade=C2=A0
>>> is done. Not so experienced people may assume there is a combi= nation=C2=A0
>>> of zpool upgrade and zfs upgrade (more so for people which do = not know=C2=A0
>>> what the difference is). Maybe you want to add some explicit= =C2=A0
>>> documentation, that zfs upgrade + feature flags needs to be do= ne by=C2=A0
>>> hand.
>>>
>>> And this brings me to a second topic, we don't have an exp= licit list=C2=A0
>>> of features which are supported by the bootloader (I had a loo= k at the=C2=A0
>>> zfs and the boot related man pages, if I overlooked a place, t= hen the=C2=A0
>>> other places should reference this important part with some te= xt).
>>
>>=C2=A0 =C2=A0 =C2=A0
>>=C2=A0 =C2=A0 There is a fixed list of features we support in the b= oot loader:
>>=C2=A0 =C2=A0 =C2=A0
>>=C2=A0 =C2=A0 /*
>> =C2=A0* List of ZFS features supported for read
>> =C2=A0*/
>> static const char *features_for_read[] =3D {
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.illumos:lz4_compress",<= br> >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:hole_birth", >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:extensible_dataset&q= uot;,
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:embedded_data",=
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.open-zfs:large_blocks",=
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.illumos:sha512",
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.illumos:skein",
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.zfsonlinux:large_dnode"= ,
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.joyent:multi_vdev_crash_dump= ",
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:spacemap_histogram&q= uot;,
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:zpool_checkpoint&quo= t;,
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:spacemap_v2", >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.datto:encryption",
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.datto:bookmark_v2",
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.zfsonlinux:allocation_classe= s",
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.datto:resilver_defer",<= br> >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:device_removal"= ,
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:obsolete_counts"= ;,
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.intel:allocation_classes&quo= t;,
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.freebsd:zstd_compress",=
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:bookmark_written&quo= t;,
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:head_errlog", >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.openzfs:blake3",
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 NULL
>> };
>>=C2=A0 =C2=A0 =C2=A0
>>=C2=A0 =C2=A0 Any feature not on this list will cause the boot load= er to=C2=A0
>> reject the pool.
>>=C2=A0 =C2=A0 =C2=A0
>>=C2=A0 =C2=A0 Whether or not it should do that by default, always, = or never is an open
>>=C2=A0 =C2=A0 question. I've thought there should be a 'sho= ot footing'=C2=A0
>> override that isn't
>>=C2=A0 =C2=A0 there today.
>>=C2=A0 =C2=A0 =C2=A0
>
> Thanks for the list. For those interested, it is in
> =C2=A0 =C2=A0=C2=A0$SRC/stand/libsa/zfs/zfsimpl.c
>
> Just to make my opinion expressed before explicit again, this should= =C2=A0
> be documented in a boot / bootloader related man-page, but isn't.<= br> >
> Should the above list be sorted in some way? Maybe in the same order= =C2=A0
> as the zpool-features lists them (sort by feature name after the=C2=A0=
> colon), or alphabetical?

Is it OK if I commit this alphabetical sorting?
---snip---
diff --git a/stand/libsa/zfs/zfsimpl.c b/stand/libsa/zfs/zfsimpl.c
index 6b961f3110a..36c90613e82 100644
--- a/stand/libsa/zfs/zfsimpl.c
+++ b/stand/libsa/zfs/zfsimpl.c
@@ -118,29 +118,29 @@ static vdev_list_t zfs_vdevs;
=C2=A0 =C2=A0* List of ZFS features supported for read
=C2=A0 =C2=A0*/
=C2=A0 static const char *features_for_read[] =3D {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.illumos:lz4_compress",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:hole_birth",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:extensible_dataset", -=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:embedded_data",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.open-zfs:large_blocks",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.illumos:sha512",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.illumos:skein",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.zfsonlinux:large_dnode",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.joyent:multi_vdev_crash_dump",<= br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:spacemap_histogram", -=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:zpool_checkpoint",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:spacemap_v2",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.datto:encryption",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.datto:bookmark_v2",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.zfsonlinux:allocation_classes",=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.datto:encryption",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.datto:resilver_defer", +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:bookmark_written",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:device_removal",<= br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:embedded_data",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:extensible_dataset", +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:head_errlog",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:hole_birth",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:obsolete_counts",=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:spacemap_histogram", +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:spacemap_v2",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:zpool_checkpoint",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.intel:allocation_classes"= ,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.joyent:multi_vdev_crash_dump",<= br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.freebsd:zstd_compress", -=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:bookmark_written",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:head_errlog",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.illumos:lz4_compress",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.illumos:sha512",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.illumos:skein",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.open-zfs:large_blocks",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.openzfs:blake3",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.zfsonlinux:allocation_classes",=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.zfsonlinux:large_dnode",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 NULL
=C2=A0 };
---snip---

This patch looks good becaus= e it's a nop and just tidies things up a bit.

= Reviewed by: imp
=C2=A0
> As Mark already mentioned some flags, I checked the features marked=C2= =A0
> as read only (I checked in the zpool-features man page, including=C2= =A0
> the dependencies documented there) and here are those not listed in=C2= =A0
> zfsimpl.c. I would assume as they are read-only compatible, we=C2=A0 <= br> > should add them:
> =C2=A0 =C2=A0 com.delphix:async_destroy
> =C2=A0 =C2=A0 com.delphix:bookmarks
> =C2=A0 =C2=A0 org.openzfs:device_rebuild
> =C2=A0 =C2=A0 com.delphix:empty_bpobj
> =C2=A0 =C2=A0 com.delphix:enable_txg
> =C2=A0 =C2=A0 com.joyent:filesystem_limits
> =C2=A0 =C2=A0 com.delphix:livelist
> =C2=A0 =C2=A0 com.delphix:log_spacemap
> =C2=A0 =C2=A0 com.zfsonlinux:project_quota
> =C2=A0 =C2=A0 com.zfsonlinux:userobj_accounting
> =C2=A0 =C2=A0 com.openzfs:zilsaxattr

If my understanding is correct that the read-only compatible parts=C2=A0 (according to the zpool-features man page) are safe to add to the zfs=C2=A0=
boot, here is what I have only build-tested (relative to the above=C2=A0 alphabetical sorting):
---snip---
--- zfsimpl.c_sorted=C2=A0 =C2=A0 =C2=A0 =C2=A0 2022-11-09 12:55:06.3460830= 00 +0100
+++ zfsimpl.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 2022-11-09 13:01:24.083364000 +010= 0
@@ -121,24 +121,35 @@
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.datto:bookmark_v2",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.datto:encryption",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.datto:resilver_defer", +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:async_destroy",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:bookmark_written"= ,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:bookmarks",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:device_removal",<= br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:embedded_data", +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:empty_bpobj",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:enable_txg",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:extensible_dataset&quo= t;,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:head_errlog",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:hole_birth",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:livelist",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:log_spacemap",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:obsolete_counts",=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:spacemap_histogram&quo= t;,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:spacemap_v2",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.delphix:zpool_checkpoint"= ,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.intel:allocation_classes"= ,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.joyent:filesystem_limits",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.joyent:multi_vdev_crash_dump&q= uot;,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.openzfs:zilsaxattr",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.zfsonlinux:project_quota",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "com.zfsonlinux:userobj_accounting",=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.freebsd:zstd_compress", =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.illumos:lz4_compress", =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.illumos:sha512",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.illumos:skein",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.open-zfs:large_blocks", =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.openzfs:blake3",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.openzfs:device_rebuild",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.zfsonlinux:allocation_classes&= quot;,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "org.zfsonlinux:large_dnode",<= br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 NULL
---snip---

Anyone able to test some of those or confirms my understanding is=C2=A0 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 the= m being absent causing
problems on boot that have been reported. = The ZFS that's in the
boot loader is a reduced copy of what&#= 39;s in base and not everything is
supported. There's no urge= ncy 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 becaus= e 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
f= uture... 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 t= hat 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 integr= ating 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 a= dditional scripts to create images
that can be tested.
=
Warner
=C2=A0
Bye,
Alexander.
--
h= ttp://www.Leidinger.net Alexander@Leidinger.net: PGP 0x8F31830F9F2772BF=
htt= p://www.FreeBSD.org=C2=A0 =C2=A0 netchild@FreeBSD.org=C2=A0 : PGP 0x8F3= 1830F9F2772BF
--000000000000e37acf05ed0bacb8--