Re: [PATCH v1 02/11] VFS permit cross device vfs_copy_file_range

[Matthew Wilcox <willy@infradead.org>]

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.