On Mon, Oct 29, 2018 at 5:52 PM Olga Kornievskaia <olga.kornievskaia@xxxxxxxxx> wrote: > > On Mon, Oct 29, 2018 at 10:25 AM Olga Kornievskaia > <olga.kornievskaia@xxxxxxxxx> wrote: > > > > On Sat, Oct 27, 2018 at 9:33 PM Dave Chinner <david@xxxxxxxxxxxxx> wrote: > > > > > > On Sat, Oct 27, 2018 at 06:23:39AM -0700, Matthew Wilcox wrote: > > > > On Sat, Oct 27, 2018 at 08:12:40PM +1100, Dave Chinner wrote: > > > > > > @@ -131,7 +132,8 @@ There is not enough space on the target filesystem to complete the copy. > > > > > > .B EXDEV > > > > > > The files referred to by > > > > > > .IR file_in " and " file_out > > > > > > -are not on the same mounted filesystem. > > > > > > +are not on the same mounted filesystem when the kernel does not support > > > > > > +cross device file copy. > > > > > > > > > > Kernel can support cross device file copy, the filesystem may not. > > > > > > > > > > EXDEV > > > > > One of the files specified by file_in and file_out are on a > > > > > filesystem that does not support cross device copies. > > > > > > > > I mentioned this in my last review, and Olga pointed out that one of > > > > the changes in this patch means the kernel will do the copy using > > > > do_splice_direct if the filesystem doesn't support cross-device copying. > > > > We should keep this error documented for those on old kernels, but the > > > > kernel will never return -EXDEV any more. > > > > > > Uh, wot? Where's the patch named "VFS: enable copy_file_range() for > > > all filesystems"? That shoul dnot be a detail hidden inside some > > > other patch that multiple people completely miss during review. > > > > The fact that cross-device check was moved is what allowed for all > > filesystems to use copy_file_range(). I think choosing the words of > > "moving cross device check" was also appropriate title for the VFS > > patch. > > I wasn't completely correct. It was the check removal and then also > allowing for -EXDEV error to keep going with the do_splice_direct(). > I could have left EXDEV being an error but thought it would be > beneficial to add such ability. One reason to keep all the changes in > one patch was said to be so that porting back all of this > functionality is included. > Olga, There are many details in this patch series and repeated miscommunications about the details. Accuracy is important - "the reason to keep all changes" - that was a commetn of mine referring *only* to the move of same sb check from VFS to different filesystems. The *extra* change of do_splice_direct cross fs was a very nice low hanging fruit, but completely unrelated to your original motivation. I completely agree with Dave that it should be in a separate patch. I also side with Dave that it would make sense as the first patch of the series. > So one option would be keep EXDEV as an error if that's what folks > want then this side-effect would not be a problem and the focus would > be on what it was in the first place : removal (or relocation) of the > cross device check into the filesystems. > This is up to you. It's an extra change and not related to your NFS work and you may submit it or not. I think it will be nice if you did submit it. There was no objection to this change, only objection was to bundle it together with a different unrelated patch. Thanks, Amir.
