DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5EE6B20651
MIME-Version: 1.0
References: <20181019153018.32507-1-olga.kornievskaia@gmail.com>
 <20181019153018.32507-2-olga.kornievskaia@gmail.com> <20181020040530.GG32577@ZenIV.linux.org.uk>
 <CAOQ4uxhAvypEvGap02CkH_bHtFt7tUVxTWeZUL5OV_V8xwWjDA@mail.gmail.com>
In-Reply-To: <CAOQ4uxhAvypEvGap02CkH_bHtFt7tUVxTWeZUL5OV_V8xwWjDA@mail.gmail.com>
From:   Olga Kornievskaia <olga.kornievskaia@gmail.com>
Date:   Mon, 22 Oct 2018 14:45:04 -0400
Message-ID: <CAN-5tyHqtV7YhfK7DY4rY643bbfV=8MOE6=mxHYJufCsdcN9Jw@mail.gmail.com>
Subject: Re: [PATCH v1 02/11] VFS permit cross device vfs_copy_file_range
To:     Amir Goldstein <amir73il@gmail.com>
Cc:     viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org,
        linux-nfs <linux-nfs@vger.kernel.org>, fweimer@redhat.com,
        Steve French <smfrench@gmail.com>,
        "Darrick J. Wong" <darrick.wong@oracle.com>,
        Christoph Hellwig <hch@lst.de>,
        Linux API <linux-api@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Sat, Oct 20, 2018 at 4:54 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sat, Oct 20, 2018 at 7:06 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Fri, Oct 19, 2018 at 11:30:18AM -0400, Olga Kornievskaia wrote:
> > > Allow copy_file_range to copy between different superblocks but only
> > > of the same file system types. This feature was of interest to CIFS
> > > as well as NFS.
> >
> > > +     if (file_out->f_op->copy_file_range &&
> > > +                     (file_in->f_op->copy_file_range ==
> > > +                             file_out->f_op->copy_file_range)) {
> >
> > That is seriously asking for trouble.  If at some point we add
> > a library function usable as ->copy_file_range() instance, this
> > will turn into a hard-to-spot problem.
> >
> > Comparing methods like that is best avoided.  If you want to compare
> > fs types, do just that - it's not hard.
>
> 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.

>
> >
> > Another potential issue here is the interplay with local filesystems
> > using vfs_clone_file_prep_inodes() (or Darrick's series lifting that
> > into generic code).  There we assume the same block size on both
> > sides; that is automatically true if they live on the same superblock,
> > but with your changes it's no longer true.
>
> Heh? I don't see that Darrick's work has anything to do with
> copy_file_range(). It only touched vfs_copy_file_range() for
> the ->clone_file_range() call, which Olga's patch protects with same sb
> check.
>
> Thanks,
> Amir.