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

2023-09-18 Thread Mark Millard
On Sep 18, 2023, at 17:05, Alexander Motin  wrote:

> On 18.09.2023 19:21, Mark Millard wrote:
>> On Sep 18, 2023, at 15:51, Mark Millard  wrote:
>>> Alexander Motin  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  bclonesaved20.9M  -
> mavlab  bcloneratio2.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, , 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.

Thanks for the details in your notes, including that bclone_enabled=0
is not relevant.

As a result I found out about zhack feature stat POOLNAME that shows
the ZPOOL_CONFIG_FEATURES_FOR_READ separately from then ones for
write.

Looking at the pool that I used for testing block_cloning
via poudriere bulk build activity (and sorting the for_*_obj
lists produced) . . .

The for_read_obj list (with matching line added as a suffix):

com.datto:bookmark_v2 = 0   "com.datto:bookmark_v2", // 
READ-ONLY COMPATIBLE no
com.datto:encryption = 0"com.datto:encryption", // 
READ-ONLY 

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

2023-09-18 Thread Alexander Motin

On 18.09.2023 19:21, Mark Millard wrote:

On Sep 18, 2023, at 15:51, Mark Millard  wrote:

Alexander Motin  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  bclonesaved20.9M  -
mavlab  bcloneratio2.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, , 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



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

2023-09-18 Thread Mark Millard



On Sep 18, 2023, at 15:51, Mark Millard  wrote:

> Alexander Motin  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_e: vfs.zfs.bclone_enabled

Sorry, a stupid error of mine messed up the above line.
It should have been:

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.
> 
> This matches up with stand/libsa/zfs/zfsimpl.c 's:
> 
> static int
> nvlist_check_features_for_read(nvlist_t *nvl)
> {
>nvlist_t *features = NULL;
>nvs_data_t *data;
>nvp_header_t *nvp;
>nv_string_t *nvp_name;
>int rc;
> 
>rc = nvlist_find(nvl, ZPOOL_CONFIG_FEATURES_FOR_READ,
>DATA_TYPE_NVLIST, NULL, , NULL);
>switch (rc) {
>case 0:
>break;  /* Continue with checks */
> 
>case ENOENT:
>return (0); /* All features are disabled */
> 
>default:
>return (rc);/* Error while reading nvlist */
>}
> 
>data = (nvs_data_t *)features->nv_data;
>nvp = >nvl_pair;  /* first pair in nvlist */
> 
>while (nvp->encoded_size != 0 && nvp->decoded_size != 0) {
>int i, found;
> 
>nvp_name = (nv_string_t *)((uintptr_t)nvp + sizeof(*nvp));
>found = 0;
> 
>for (i = 0; features_for_read[i] != NULL; i++) {
>if (memcmp(nvp_name->nv_data, features_for_read[i],
>nvp_name->nv_size) == 0) {
>found = 1;
>break;
>}
>}
> 
>if (!found) {
>printf("ZFS: unsupported feature: %.*s\n",
>nvp_name->nv_size, nvp_name->nv_data);
>rc = EIO;
>}
>nvp = (nvp_header_t *)((uint8_t *)nvp + nvp->encoded_size);
>}
>nvlist_destroy(features);
> 
>return (rc);
> }
> 
> I do not know if vfs.zfs.bclone_enabled=0 leads the loader
> to see vs. not-see a "com.fudosecurity:block_cloning".

It appears that 2 additions afeter opebzfas-2.1-freebsd are
missing from the above list:

com.fudosecurity:block_cloning
org.openzfs:zilsaxattr

See, for reference (but shorter naming):

# diff -u99 
/usr/main-src/sys/contrib/openzfs/cmd/zpool/compatibility.d/openzfs-2.1-freebsd 
/usr/main-src/sys/contrib/openzfs/cmd/zpool/compatibility.d/openzfs-2.2
--- 
/usr/main-src/sys/contrib/openzfs/cmd/zpool/compatibility.d/openzfs-2.1-freebsd 
2021-06-24 20:08:57.206621000 -0700
+++ /usr/main-src/sys/contrib/openzfs/cmd/zpool/compatibility.d/openzfs-2.2 
2023-06-10 15:51:13.220607000 -0700
@@ -1,34 +1,40 @@
-# Features supported by OpenZFS 2.1 on FreeBSD
+# Features supported by OpenZFS 2.2 on Linux and FreeBSD
 allocation_classes
 async_destroy
+blake3
+block_cloning
 bookmark_v2