On 9/18/23 16:37, Tomoaki AOKI wrote:
At least, if I read the code correctly,
     "com.fudosecurity:block_cloning",
should be added to array
     *features_for_read[]
of stand/libsa/zfs/zfsimpl.c.

There are check codes like below, so without it, boot codes would
reject to boot from any pool having block_cloning feature enabled.
Am I missing something?

                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;
                }

Regards.


I'm pretty sure what he's trying to tell you is that this feature won't show up in the list to be compared against this in the first place. Presumably the inquiry it uses either just looks for `ZFEATURE_FLAG_MOS` or filters out `ZFEATURE_FLAG_READONLY_COMPAT`, which is described as:

90 /* Can open pool readonly even if this feature is not supported. */

Thanks,

Kyle Evans


On Mon, 18 Sep 2023 09:26:56 -0400
Alexander Motin <m...@freebsd.org> wrote:

block_cloning feature is marked as READONLY_COMPAT.  It should not
require any special handling from the boot code.

On 18.09.2023 07:22, Tomoaki AOKI wrote:
Really OK?

I cannot find block_cloning in array *features_for_read[] of
stand/libsa/zfs/zfsimpl.c, which possibly mean boot codes (including
loader) cannot boot from Root-on-ZFS pool having block_cloning active.

Not sure adding '"com.fudosecurity:block_cloning",' here is sufficient
or not. Possibly more works are needed.

IMHO, all default-enabled features should be safe for booting.
Implement features with disalded, impement boot codes to support them,
then finally enable them by default should be the only valid route.


[1] https://cgit.freebsd.org/src/tree/stand/libsa/zfs/zfsimpl.c


On Mon, 18 Sep 2023 07:31:46 +0200
Martin Matuska <m...@freebsd.org> wrote:

I vote for enabling block cloning on main :-)

mm

On 16. 9. 2023 19:14, Alexander Motin wrote:
On 16.09.2023 01:25, Graham Perrin wrote:
On 16/09/2023 01:28, Glen Barber wrote:
o A fix for the ZFS block_cloning feature has been implemented.

Thanks

I see
<https://github.com/openzfs/zfs/commit/5cc1876f14f90430b24f1ad2f231de936691940f>,
with
<https://github.com/freebsd/freebsd-src/commit/9dcf00aa404bb62052433c45aaa5475e2760f5ed>
in stable/14.

As vfs.zfs.bclone_enabled is still 0 (at least, with 15.0-CURRENT
n265350-72d97e1dd9cc): should we assume that additional fixes, not
necessarily in time for 14.0-RELEASE, will be required before
vfs.zfs.bclone_enabled can default to 1?

I am not aware of any block cloning issues now.  All this thread about
bclone_enabled actually started after I asked why it is still
disabled. Thanks to Mark Millard for spotting this issue I could fix,
but now we are back at the point of re-enabling it again.  Since the
tunable does not even exist anywhere outside of FreeBSD base tree, I'd
propose to give this code another try here too.  I see no point to
have it disabled at least in main unless somebody needs time to run
some specific tests first.


--
Alexander Motin



Reply via email to