Re: vfs.zfs.bclone_enabled (was: FreeBSD 14.0-BETA2 Now Available) [block_cloning and zilsaxattr missing from loader's features_for_read]

On 18.09.2023 19:21, Mark Millard wrote:
> On Sep 18, 2023, at 15:51, Mark Millard <marklmi@yahoo.com> wrote:
>> Alexander Motin <mav_at_FreeBSD.org> wrote on
>> Date: Mon, 18 Sep 2023 13:26:56 UTC :
>>> block_cloning feature is marked as READONLY_COMPAT. It should not
>>> require any special handling from the boot code.
>>
>>  From stand/libsa/zfs/zfsimpl.c but adding a comment about the
>> read-only compatibility status of each entry:
>>
>> /*
>> * List of ZFS features supported for read
>> */
> static const char *features_for_read[] = {
>>         "com.datto:bookmark_v2", // READ-ONLY COMPATIBLE no
>>         "com.datto:encryption", // READ-ONLY COMPATIBLE no
>>         "com.datto:resilver_defer", // READ-ONLY COMPATIBLE yes
>>         "com.delphix:bookmark_written", // READ-ONLY COMPATIBLE no
>>         "com.delphix:device_removal", // READ-ONLY COMPATIBLE no
>>         "com.delphix:embedded_data", // READ-ONLY COMPATIBLE no
>>         "com.delphix:extensible_dataset", // READ-ONLY COMPATIBLE no
>>         "com.delphix:head_errlog", // READ-ONLY COMPATIBLE no
>>         "com.delphix:hole_birth", // READ-ONLY COMPATIBLE no
>>         "com.delphix:obsolete_counts", // READ-ONLY COMPATIBLE yes
>>         "com.delphix:spacemap_histogram", // READ-ONLY COMPATIBLE yes
>>         "com.delphix:spacemap_v2", // READ-ONLY COMPATIBLE yes
>>         "com.delphix:zpool_checkpoint", // READ-ONLY COMPATIBLE yes
>>         "com.intel:allocation_classes", // READ-ONLY COMPATIBLE yes
>>         "com.joyent:multi_vdev_crash_dump", // READ-ONLY COMPATIBLE no
>>         "com.klarasystems:vdev_zaps_v2", // READ-ONLY COMPATIBLE no
>>         "org.freebsd:zstd_compress", // READ-ONLY COMPATIBLE no
>>         "org.illumos:lz4_compress", // READ-ONLY COMPATIBLE no
>>         "org.illumos:sha512", // READ-ONLY COMPATIBLE no
>>         "org.illumos:skein", // READ-ONLY COMPATIBLE no
>>         "org.open-zfs:large_blocks", // READ-ONLY COMPATIBLE no
>>         "org.openzfs:blake3", // READ-ONLY COMPATIBLE no
>>         "org.zfsonlinux:allocation_classes", // READ-ONLY COMPATIBLE yes
>>         "org.zfsonlinux:large_dnode", // READ-ONLY COMPATIBLE no
>>         NULL
>> };
>>
>> So it appears that the design is that both "no" and "yes" ones
>> that are known to be supported are listed and anything else is
>> supposed to lead to rejection until explicitly added as
>> known-compatibile.

I don't think so.  I think somebody by mistake added first featured that 
should not be here, and then others continued this irrelevant routine. 
My own development server/builder is happily running latest main with 
ZFS root without any patches and with block cloning not only enabled, 
but even active.  So as I have told, it is not needed:

mav@srv:/root# zpool get all | grep clon
mavlab  bcloneused                     20.5M                          -
mavlab  bclonesaved                    20.9M                          -
mavlab  bcloneratio                    2.02x                          -
mavlab  feature@block_cloning          active                         local

Somebody should go through the list and clean in up from read-compatible 
features and document it, unless there are some features that were 
re-qualified at some point, I haven't checked if it could be.

>> This matches up with stand/libsa/zfs/zfsimpl.c 's:
>>
>> static int
>> nvlist_check_features_for_read(nvlist_t *nvl)
>> {
...
>>         rc = nvlist_find(nvl, ZPOOL_CONFIG_FEATURES_FOR_READ,
>>             DATA_TYPE_NVLIST, NULL, &features, NULL);

Take a note it reads ZPOOL_CONFIG_FEATURES_FOR_READ.  Same time features 
declared as READONLY_COMPAT are stored in FEATURES_FOR_WRITE, that boot 
loader does not even care.

>> I do not know if vfs.zfs.bclone_enabled=0 leads the loader
>> to see vs. not-see a "com.fudosecurity:block_cloning".

bclone_enabled=0 block copy_file_range() usage, that should keep the 
feature enabled, but not active.  It could be related if the feature 
would be in FEATURES_FOR_WRITE, but here and now it is not.

> It appears that 2 additions afeter opebzfas-2.1-freebsd are
> missing from the above list:
> 
> com.fudosecurity:block_cloning
> org.openzfs:zilsaxattr

Nothing of ZIL is required for read-only import.  So no, it is also not 
needed.

-- 
Alexander Motin