DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4593520671
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>
In-Reply-To: <a4a3ac1e840444871f3bc87606b111668c66618b.camel@redhat.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Tue, 23 Oct 2018 09:05:33 +0300
Message-ID: <CAOQ4uxiRnyxtOXBrdCvUcTa8GkTb2OyaJYJ58Ddoik0=3Y9pyA@mail.gmail.com>
Subject: Re: [PATCH v1 02/11] VFS permit cross device vfs_copy_file_range
To:     Jeff Layton <jlayton@redhat.com>
Cc:     Olga Kornievskaia <olga.kornievskaia@gmail.com>,
        Matthew Wilcox <willy@infradead.org>,
        Al Viro <viro@zeniv.linux.org.uk>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Linux NFS Mailing List <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@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 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.

Thanks,
Amir.