On Thu, Feb 18, 2021 at 2:14 PM Luis Henriques <lhenriques@xxxxxxx> wrote: > > Luis Henriques <lhenriques@xxxxxxx> writes: > > > Amir Goldstein <amir73il@xxxxxxxxx> writes: > > > >> On Thu, Feb 18, 2021 at 9:42 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > >>> > >>> Looks good: > >>> > >>> Reviewed-by: Christoph Hellwig <hch@xxxxxx> > >>> > >>> 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.
