DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0D8D220674
Message-ID: <a4a3ac1e840444871f3bc87606b111668c66618b.camel@redhat.com>
Subject: Re: [PATCH v1 02/11] VFS permit cross device vfs_copy_file_range
From:   Jeff Layton <jlayton@redhat.com>
To:     Olga Kornievskaia <olga.kornievskaia@gmail.com>,
        willy@infradead.org
Cc:     Amir Goldstein <amir73il@gmail.com>, 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>
Date:   Mon, 22 Oct 2018 19:39:01 -0400
In-Reply-To: <CAN-5tyF=voE2=DBLKvJCHZc9+zMzysqD8REYYb-z5jwq2BLqGQ@mail.gmail.com>
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>
         <CAN-5tyHqtV7YhfK7DY4rY643bbfV=8MOE6=mxHYJufCsdcN9Jw@mail.gmail.com>
         <20181022190620.GA8863@bombadil.infradead.org>
         <CAN-5tyF=voE2=DBLKvJCHZc9+zMzysqD8REYYb-z5jwq2BLqGQ@mail.gmail.com>
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 Mon, 2018-10-22 at 15:34 -0400, Olga Kornievskaia wrote:
> On Mon, Oct 22, 2018 at 3:06 PM Matthew Wilcox <willy@infradead.org> 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@gmail.com> 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@redhat.com>