On Fri, 2018-10-19 at 12:06 -0700, Matthew Wilcox wrote: > On Fri, Oct 19, 2018 at 02:47:42PM -0400, Olga Kornievskaia wrote: > > On Fri, Oct 19, 2018 at 1:58 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > > > > > On Fri, Oct 19, 2018 at 11:30:18AM -0400, Olga Kornievskaia wrote: > > > > +++ b/Documentation/filesystems/vfs.txt > > > > @@ -958,7 +958,9 @@ otherwise noted. > > > > > > > > fallocate: called by the VFS to preallocate blocks or punch a hole. > > > > > > > > - copy_file_range: called by the copy_file_range(2) system call. > > > > + copy_file_range: called by copy_file_range(2) system call. This method > > > > + works on two file descriptors that might reside on > > > > + different superblocks of the same type of file system. > > > > > > I don't think this text is explicit enough about what has changed, > > > > Can you suggest a different wording that would be stronger? I can't > > say any more clear that copy is allowed between different superblock > > which wasn't the case before. > > I would leave this file alone. > > > > and I > > > think this is the wrong place for it. I think there should be a paragraph > > > in Documentation/filesystems/porting and it should follow the current style > > > in there. > > > > I have looked at the "porting" file and it's cryptic. I don't > > understand what [mandatory] [recommended] stanzas are. There is > > currently no copy_file_range. Is this just a "changelog" and you are > > looking for something like > > [mandatory] > > copy_file_range() now allows copying between different superblocks. > > > > I can add this wording to the "porting" file. > > The porting file is written from the point of view of someone who's trying > to port an old filesystem to current Linux. > > Maybe something like > > [mandatory] > ->copy_file_range() may now be passed files which belong to two > different filesystems. The destination's copy_file_range() is the > function which is called. If it cannot copy ranges from the source, > it should return -EXDEV. > > > > > - if (file_out->f_op->copy_file_range) { > > > > + if (file_out->f_op->copy_file_range && > > > > + (file_in->f_op->copy_file_range == > > > > + file_out->f_op->copy_file_range)) { > > > > > > Can we avoid this extra test here? I know the various stamdards groups > > > including T10 and the IETF have been trying to define a universal > > > identifier for the same blob of storage, no matter how it's accessed; > > > potentially allowing access to the same storage across iSCSI, CIFS > > > and NFS. If we ever get to a point where we support that (and I am > > > dubious), we'd want to remove this test again, and have to revalidate > > > all the filesystems. > > > > Well from previous submissions I was explicitly asked to add this check. > > I'm not sure that's exactly what Jeff was asking for. Or maybe it was > and my argument above will change his mind. Jeff, what do you think? > > If we do do this, cifs will need a modification as part of this patch to > reject non-CIFS files as it currently assumes that src_file->private_data > is a pointer to a struct cifsFileInfo. My suggestion to Olga was more of a "if you feel you have to have a check like this at the vfs layer...", but I think you and Al have convinced me that comparing operations like this is not a good idea. I still think that this check belongs down inside the fs copy_file_range operation itself. That allows the the freedom to implement xdev copies on a per-fs basis later without needing to alter vfs-level code. -- Jeff Layton <jlayton@xxxxxxxxxx>