As requested by rpokala, D39418 and D39419 have been submitted. Again, I am not the author of these. Only the curator.
We can continue our discussion there. -- Cheers, Cy Schubert <[email protected]> FreeBSD UNIX: <[email protected]> Web: https://FreeBSD.org NTP: <[email protected]> Web: https://nwtime.org e^(i*pi)+1=0 In message <[email protected]>, Martin Matuska w rites: > 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, b > ut > 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 <[email protected]. > 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. > > > >
