DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 41D9B207DD
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>
 <CAN-5tyHqtV7YhfK7DY4rY643bbfV=8MOE6=mxHYJufCsdcN9Jw@mail.gmail.com>
 <20181022190620.GA8863@bombadil.infradead.org> <CAN-5tyF=voE2=DBLKvJCHZc9+zMzysqD8REYYb-z5jwq2BLqGQ@mail.gmail.com>
 <a4a3ac1e840444871f3bc87606b111668c66618b.camel@redhat.com>
 <CAOQ4uxiRnyxtOXBrdCvUcTa8GkTb2OyaJYJ58Ddoik0=3Y9pyA@mail.gmail.com> <CAN-5tyHQc3M2YprFJm35XUxJdAx11a85o1NygE5FQFyHq3SQew@mail.gmail.com>
In-Reply-To: <CAN-5tyHQc3M2YprFJm35XUxJdAx11a85o1NygE5FQFyHq3SQew@mail.gmail.com>
From:   Olga Kornievskaia <olga.kornievskaia@gmail.com>
Date:   Tue, 23 Oct 2018 11:30:13 -0400
Message-ID: <CAN-5tyEtRSQMS-u0gbheFn5f2siOy8Qva2vnXW6rwb7pr_GDkA@mail.gmail.com>
Subject: Re: [PATCH v1 02/11] VFS permit cross device vfs_copy_file_range
To:     Amir Goldstein <amir73il@gmail.com>
Cc:     Jeff Layton <jlayton@redhat.com>, willy@infradead.org,
        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 Tue, Oct 23, 2018 at 11:03 AM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
>
> On Tue, Oct 23, 2018 at 2:05 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Oct 23, 2018 at 2:39 AM Jeff Layton <jlayton@redhat.com> wrote:
> > >
> > > 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.
> > >
> >
> > Yes. Eventually. And even Matthew is (quote) "dubious" about that ever
> > happening. Changing the interface as Matthew proposed has a price
> > and we need to compare this price to the alleged backporting price
> > that nobody may ever need to pay.
> >
> > As far as I can tell, passing a struct file * on a file_operations method
> > that does not belong to that filesystem in unprecedented (*) and is a far
> > more lethal landmine than the alleged backporting landmine.
> >
> > (*) prior to v4.19-rc1, filesystems could get an overlayfs file, but
> >      file_inode(file) has always belonged to the filesystem.
> >
> > Olga,
> >
> > I do not strongly object to Matthew's proposal, so don't feel
> > obligated to choose my side of the argument. I am just trying
> > to offer a different perspective.
> >
> > In any case, my outstanding concerns with the patch are:
> >
> > 1. If you change syscall to support cross fs type copy (which is
> >     good IMO) need to document that in commit message
> >     and possibly follow up later with a note in man page
> >
> > 2. Commit message says:
> >     "This feature was of interest of ... NFS"
> >     "This feature is needed by NFSv4.2..."
> >     "NFS will allow for copies between different NFS servers."
> >     It is not clear to me if we are talking about present of future
> >     NFSv4.2 code. If NFSv4.2 currently does not support cross
> >     sb copy (??) than your patch need to enforce same sb
> >     in nfs4_copy_file_range(). If it does support cross sb copy
> >     than please edit the commit message to make that clear.
>
> I personally agree with Amir. I think it's far fetched that a file
> system would know how to handle something that's not of its type. When
> the copy_file_range() was checked in, there was a comment above the
> superblock check saying "we might want to relax this in the future".
> It deemed appropriate then to enforce the check since none of the file
> systems used it. Now, the future is here, and we are removing the
> check but proposing a different once because again the future isn't
> here and having a single check simplifies the code.

Sorry Ok I wrote this too fast. I think I'm changing my mind and
siding with the check by the file system.

> But I don't feel strongly about the check (or rather the location of
> it VFS vs each FS) and what I ultimately need is to removed same sb
> check. It sounds like if Amir isn't objecting, then the check for same
> file system type should be removed from VFS. And, for each of the file
> systems that currently support copy_file_range() -- CIFS, NFS, and
> overlayfs -- I need to modify them and add a check for the same
> fs_type.
>
> Amir to answer your question, only NFSv4.2 has copy_offload
> functionality (not earlier NFS versions). Furthermore, existing
> upstream only supports same sb copy offload. What this patch series
> adds is support for copy offload across different superblocks but NFS
> will not support (and would need a check) for copy offload across
> different file system types. Also I kinda stand behind the ideas
> stated: 1) this is of interest to NFS (where NFS here is to represent
> a community, and CIFS is used to represent the other community). 2)
> NFSv4.2 copy offload a specific feature that needs this functionality.
> 3rd statement is bad. Only NFSv4.2 will allow copies between different
> NFS servers (ie., after this patch +series), the emphasis was on "will
> allow" meaning what this patch will allow to be done (ie, patch's
> purpose). Or also, if the NFS server exports different exports, then a
> copy between them (assuming exports of the same file system type).
>
> In the next version of the patch, I'll do my best to specified what
> changed as the consequence of removing the cross sb check (ie, file
> system types of the passed in file can be from different file
> systems). I will add wording to the man page and add the suggested
> wording to the "porting" file.