On Tue, Nov 14, 2023 at 01:30:25PM -0800, Rick Macklem wrote:
> On Tue, Nov 14, 2023 at 1:20 PM Konstantin Belousov <kostik...@gmail.com>
> wrote:
> >
> > On Tue, Nov 14, 2023 at 06:47:46PM +0100, Mateusz Guzik wrote:
> > > On 11/14/23, Alexander Motin <m...@freebsd.org> wrote:
> > > > On 14.11.2023 12:39, Mateusz Guzik wrote:
> > > >> One of the vnodes is probably not zfs, I suspect this will do it
> > > >> (untested):
> > > >>
> > > >> diff --git a/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c
> > > >> b/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c
> > > >> index 107cd69c756c..e799a7091b8e 100644
> > > >> --- a/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c
> > > >> +++ b/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c
> > > >> @@ -6270,6 +6270,11 @@ zfs_freebsd_copy_file_range(struct
> > > >> vop_copy_file_range_args *ap)
> > > >> goto bad_write_fallback;
> > > >> }
> > > >> }
> > > >> +
> > > >> + if (invp->v_mount->mnt_vfc != outvp->v_mount->mnt_vfc) {
> > > >> + goto bad_write_fallback;
> > > >> + }
> > > >> +
> > > >> if (invp == outvp) {
> > > >> if (vn_lock(outvp, LK_EXCLUSIVE) != 0) {
> > > >> goto bad_write_fallback;
> > > >>
> > > >
> > > > vn_copy_file_range() verifies for that:
> > > >
> > > > /*
> > > > * If the two vnodes are for the same file system type, call
> > > > * VOP_COPY_FILE_RANGE(), otherwise call
> > > > vn_generic_copy_file_range()
> > > > * which can handle copies across multiple file system types.
> > > > */
> > > > *lenp = len;
> > > > if (inmp == outmp || strcmp(inmp->mnt_vfc->vfc_name,
> > > > outmp->mnt_vfc->vfc_name) == 0)
> > > > error = VOP_COPY_FILE_RANGE(invp, inoffp, outvp,
> > > > outoffp,
> > > > lenp, flags, incred, outcred, fsize_td);
> > > > else
> > > > error = vn_generic_copy_file_range(invp, inoffp, outvp,
> > > > outoffp, lenp, flags, incred, outcred, fsize_td);
> > > >
> > > >
> > >
> > > The crash at hand comes from nullfs. If "outward" vnodes are both
> > > nullfs, but only one underlying vnode is zfs, you get the above.
> >
> > If this is the reason, the check must be done by nullfs bypass for
> > vop_copy_file_range().
> I suppose this is a reasonable alternative, although it means that
> all stacked file systems will need the check.
> It just seems easier to do it in the actual VOPs, but it is up to others.
In theory, eventually we can have much more implementations for VOP
than VOP' callers. I.e. fixing all stacked fs means adding unionfs to
my patch. Forcing this requirements on all future VOP_COPY_FILE_RANGE
implementations is IMO not good.
BTW, we already have zfs, nfs, and fuse implementing the VOP.
>
> Btw, the stuff above the VOP_COPY_FILE_RANGE() that busies the
> mounts and checks mnt_vfc being the same could be dropped, if the
> VOP_COPY_FILE_RANGE() calls like NFS were careful to lock the
> vnodes before doing a "same fs type or same mount" check.
> (I suppose that would be a subtle change in VOP semantics that
> is arguably not allowed for a minor version.)
>
> Anyhow, I am happy with whatever others decide.
>
> rick
>
> >
