Re: [PATCH v2] vfs: prevent copy_file_range to copy across devices

Amir Goldstein
2021-02-18T13:06:03Z

On Thu, Feb 18, 2021 at 2:14 PM Luis Henriques <lhenriques@suse.de> wrote:
>
> Luis Henriques <lhenriques@suse.de> writes:
>
> > Amir Goldstein <amir73il@gmail.com> writes:
> >
> >> On Thu, Feb 18, 2021 at 9:42 AM Christoph Hellwig <hch@infradead.org> wrote:
> >>>
> >>> Looks good:
> >>>
> >>> Reviewed-by: Christoph Hellwig <hch@lst.de>
> >>>
> >>> This whole idea of cross-device copie has always been a horrible idea,
> >>> and I've been arguing against it since the patches were posted.
> >>
> >> Ok. I'm good with this v2 as well, but need to add the fallback to
> >> do_splice_direct()
> >> in nfsd_copy_file_range(), because this patch breaks it.
> >>
> >> And the commit message of v3 is better in describing the reported issue.
> >
> > Except that, as I said in a previous email, v2 doesn't really fix the
> > issue: all the checks need to be done earlier in generic_copy_file_checks().
> >
> > I'll work on getting v4, based on v2 and but moving the checks and
> > implementing your review suggestions to v3 (plus this nfs change).
>
> There's something else:
>
> The filesystems (nfs, ceph, cifs, fuse) rely on the fallback to
> generic_copy_file_range() if something's wrong. And this "something's
> wrong" is fs specific. For example: in ceph it is possible to offload the
> file copy to the OSDs even if the files are in different filesystems as
> long as these filesystems are on the *same* ceph cluster. If the copy
> being done is across two different clusters, then the copy reverts to
> splice. This means that the boilerplate code being removed in v2 of this
> patch needs to be restored and replace by:
>
> ret = __ceph_copy_file_range(src_file, src_off, dst_file, dst_off,
> len, flags);
>
> if (ret == -EOPNOTSUPP || ret == -EXDEV)
> ret = do_splice_direct(src_file, &src_off, dst_file, &dst_off,
> len > MAX_RW_COUNT ? MAX_RW_COUNT : len,
> flags);
> return ret;
>

Why not leave the filesystem code as is and leave the
generic_copy_file_range() helper? Less churn.

Then nfsd_copy_file_range() can also fallback to generic_copy_file_range().

Thanks,
Amir.