On Mon, 2018-10-22 at 15:34 -0400, Olga Kornievskaia wrote: > On Mon, Oct 22, 2018 at 3:06 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > > > On Mon, Oct 22, 2018 at 02:45:04PM -0400, Olga Kornievskaia wrote: > > > On Sat, Oct 20, 2018 at 4:54 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > Another thing is the commit message claims to: > > > > "Allow copy_file_range to copy between different superblocks but only > > > > of the same file system types" > > > > > > > > But what the patch actually does is: > > > > "Allow copy_file_range() syscall to copy between different filesystems > > > > AND allow calling the filesystems' copy_file_range() method > > > > between different superblocks but only of the same file system types" > > > > > > > > It's probably OK and quite useful to do the former, but maybe man page > > > > should be fixed to explicitly mention that the copy is expected to work > > > > across filesystems since kernel version XXX (?) > > > > > > > > If you don't wish to change cross filesystem type behavior and only > > > > relax cross super block limitation, then you should replace the > > > > same inode->i_sb check above with same inode->i_sb->s_type > > > > check instead of doing the check only for calling the filesystem > > > > copy_file_range() method. > > > > > > Thank you for the feedback. In the next version, I will remove the > > > check for the functions and instead check for the same file system > > > types. > > > > Jeff and I agree that this is the wrong way to go. Instead, the > > cross-device check should be in the individual instances, not the top > > level code. > > So remove the check all together for the VFS (that was my original > patch to begin with (like #1 not this one). So am I missing the point > again, I keep getting different corrections every time. Sorry if I wasn't clear before: Basically, I think Willy and I are both envisioning that some copy_file_range implementations may eventually not be subject to the limitations of the checks you're adding. All of the current and currently proposed ones are however, so we need these checks in place. We have two options. We can either do them globally at the vfs layer or in the individual filesystem "drivers". I argue that it's better to do it at the driver level, because if we ever want to implement one that is not subject to these limitations, you'll effectively have to push these checks down into the drivers later. That's where things become ugly for backports and such. You'll basically be changing a subtle expectation in the driver interface, which could be a source of bugs later. If we set the expectation now that the drivers need to do this checking then we can (hopefully) ensure that they all do before they're ever merged. -- Jeff Layton <jlayton@xxxxxxxxxx>
