Re: git: 8ee579abe09e - main - zfs: fall back if block_cloning feature is disabled

I agree that these are three individual fixes.

1.) pass ap->a_outcred instead of ap->a_fsizetd->td_ucred to 
zfs_clone_range()
I am ok with this, the way the argument is subsequently used it should 
be ap->a_outcred which is intended for the write.

2.) do a vn_generic_copy_file_range() in case of EXDEV

The comment vn_generic_copy_file_range() says:
/*
  * Copy a byte range of one file to another.  This function can handle the
  * case where invp and outvp are on different file systems.
  * It can also be called by a VOP_COPY_FILE_RANGE() to do the work, if 
there
  * is no better file system specific way to do it.
  */

That is actually our case. zfs_clone_range() exits with EXDEV if:
- source and destination are not on the same pool
- the block_cloning feature is not enabled
- input and output files have a different block size
- offset and len are not at block boundaries
- length is not a multiple of block size, with except for the end of file
- we are trying to clone a block created in the current transaction group
- we are cloning encrypted data not in the same dataset

IMO we can fallback to vn_generic_copy_file_range() in all of these cases.

As of the locks, we need to run vn_generic_copy_file_range() on unlocked 
vnodes,
just look into the function.
In both fuse_vnops.c and nfs_clvnops.c it does not run on locked vnodes.
Even the comment from pjd in zfs_vnops_os.c says:
         /*
          * TODO: If offset/length is not aligned to recordsize, use
          * vn_generic_copy_file_range() on this fragment.
          * It would be better to do this after we lock the vnodes, but 
then we
          * need something else than vn_generic_copy_file_range().
          */

So IMO it should be at the end after unlock.

3.) By doing the feature check early, we save locking the input vnode 
and calling mac_vnode_check_write() and vn_rlimit_fsize() at the cost of 
checking for the disabled feature twice. Maybe documented skipping of 
the check in zfs_clone_range()? The code of the early check looks ok to me.

On 4. 4. 2023 20:18, Cy Schubert wrote:
> In message <CAGudoHHe3FJmW_cEddozQScJcwfbdbbfEn=y+m6wwmzmvEMb-w@mail.gmail.c
> om>
> , Mateusz Guzik writes:
>> can you please post a review
> I could but I didn't write any of it. Rick Macklem and Martin Matuska wrote
> it. My patch was for discussion only.
>
> Martin and Rick, do you mind if I post this as a review. It should probably
> be two, maybe three separate commits, fixing two different problems.
>
>