DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 377C820836
Message-ID: <b2f656abd4bac72f7ba896e1d6af1b944d28dab9.camel@redhat.com>
Subject: Re: [PATCH v1 02/11] VFS permit cross device vfs_copy_file_range
From:   Jeff Layton <jlayton@redhat.com>
To:     Matthew Wilcox <willy@infradead.org>,
        Olga Kornievskaia <olga.kornievskaia@gmail.com>
Cc:     linux-fsdevel@vger.kernel.org, linux-nfs@vger.kernel.org,
        fweimer@redhat.com, smfrench@gmail.com
Date:   Sun, 21 Oct 2018 10:10:22 -0400
In-Reply-To: <20181019175822.GB28891@bombadil.infradead.org>
References: <20181019153018.32507-1-olga.kornievskaia@gmail.com>
         <20181019153018.32507-2-olga.kornievskaia@gmail.com>
         <20181019175822.GB28891@bombadil.infradead.org>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Fri, 2018-10-19 at 10:58 -0700, Matthew Wilcox 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, 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.
> 
> > @@ -1591,7 +1587,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> >  	 * Try cloning first, this is supported by more file systems, and
> >  	 * more efficient if both clone and copy are supported (e.g. NFS).
> >  	 */
> > -	if (file_in->f_op->clone_file_range) {
> > +	if (inode_in->i_sb == inode_out->i_sb &&
> > +			file_in->f_op->clone_file_range) {
> 
> This reads weirdly to me.  I know it's the same order the tests were done
> in before, but it would feel more natural to me to test:
> 
> 	if (file_in->f_op->clone_file_range &&
> 			inode_in->i_sb == inode_out->i_sb)
> 
> Am I just suffering from "I would have done this differently"itis, or
> is it unnatural?
> 
> > @@ -1600,10 +1597,12 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> >  		}
> >  	}
> >  
> > -	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.
> 

Agreed. I think this really ought to be left up to the lower fs. Pass
the op both struct file pointers and let it sort it out.

We just need to document the expectation that file copy_file_range ops
vet file_in carefully as it could be from anything, and have them return
an appropriate error code if it's not something they can deal with.
-- 
Jeff Layton <jlayton@redhat.com>