Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Olga Kornievskaia <olga.kornievskaia@gmail.com>
Cc: Amir Goldstein <amir73il@gmail.com>,
	"J. Bruce Fields" <bfields@redhat.com>,
	linux-nfs <linux-nfs@vger.kernel.org>,
	linux-fsdevel@vger.kernel.org, willy@infradead.org,
	jlayton@kernel.org, stfrench@microsoft.com
Subject: Re: [PATCH v2 01/10] VFS generic copy_file_range() support
Date: Mon, 3 Dec 2018 07:47:49 +1100
Message-ID: <20181202204749.GS19305@dastard> (raw)
In-Reply-To: <CAN-5tyFBjVM6HOGZbjy+k4eCOHyDdvXsiGOHSR+gRXCHS3fosA@mail.gmail.com>

On Sat, Dec 01, 2018 at 10:12:05PM -0500, Olga Kornievskaia wrote:
> On Sat, Dec 1, 2018 at 5:00 PM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Sat, Dec 01, 2018 at 10:11:48AM +0200, Amir Goldstein wrote:
> > > On Fri, Nov 30, 2018 at 10:04 PM Olga Kornievskaia
> > > <olga.kornievskaia@gmail.com> wrote:
> > > >
> > > > Relax the condition that input files must be from the same
> > > > file systems.
> > > >
> > > > Add checks that input parameters adhere semantics.
> > > >
> > > > If no copy_file_range() support is found, then do generic
> > > > checks for the unsupported page cache ranges, LFS, limits,
> > > > and clear setuid/setgid if not running as root before calling
> > > > do_splice_direct(). Update atime,ctime,mtime afterwards.
> > > >
> > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > > ---
> > >
> > > This patch is either going to bring you down or make you stronger ;-)
> > >
> > > This is not how its done. Behavior change and refactoring mixed into
> > > one patch is wrong for several reasons. And when you relax same sb
> > > check you need to restrict it inside filesystems, like your previous patch
> > > did.
> > .....
> > > In any case, I hear that Dave is neck deep in fixing copy_file_range()
> > > so changes to this function should be collaborated with him. Or better
> > > yet, wait until he posts his fixes and carry on from there.
> >
> > Yeah, because I've heard nothing for a month and this is kinda
> > important
> 
> Dave I think that's unfair. It is important. NFS is actually the file
> system that needed VFS support for cross fs copy_file_range and I was
> working on it. If you were in doubt, you could have emailed and asked
> me.

Last I heard from you was "this isn't my problem and I don't have
time to deal with it". You were fairly unambiguous in saying you
weren't going to spend any time on it.

> I'm unsure now what does this mean. I have a patch series with a VFS
> patch that went thru the extensive review (people spend time on it)
> and an NFS patch series that depends on it that is ready for the
> upstream push. Are you saying that the VFS patch is no longer welcomed
> and thus NFS series is no longer viable either?

No, I'm saying that this is urgent work and needs to be separated
from the NFS patch series, of which there are now two and you've
split copy_file_range() changes across both patch sets.
copy_file_range() is broken for *everyone*, not just NFS.  i.e.
fixing these problems should not be tied to some other filesystem
feature patchset.

> , I have a series of 8-9 patches that make all the fixes we
> > need, push the cross-filesystem checks down into the filesystems,
> > and let filesystems handle the fallback to a splice based copy
> > themselves (because there are way more fallback cases than just
> > EOPNOPSUPP and EXDEV).
> 
> Are you saying it is each individual filesystem responsibility to
> fallback on splice? Isn't that a step backwards? Each individual
> filesystem is going to implement the same code of calling
> do_splice_direct() to do the functionally that could and should be in
> VFS?

I've done this because one of the problems I've found is that
different filesystems *do not fall back consistently*. e.g. the NFS
client will return -EINVAL if src/dst are the same file, but -EINVAL
is not one of the errors that the vfs code falls back to a data copy
on.

This is despite the fact that the fallback path can copy to/from
the same file, we support same file copy through the
->remap_file_range offload, etc. IOWs, the behaviour of the syscall
when it comes to single file ranges is completely inconsistent
because fallbacks are implemented on a filesystem-by-filesystem
basis.

I called the fallback generic_copy_file_range(), and filesystems that
implement ->copy_file_range() are responsible for calling it
themselves if they want a fallback. That's because there may be
different error/constraint conditions at the filesystem level that
prevent offloading the copy, and we can't distinguish at the VFs
between "-EINVAL means fallback because it was a single file copy"
and "-EINVAL means fail, parameter out of range".

IOWs, if you implement ->copy_file_range() you take full
resposnsibility for implementing the copying function. This is
exactly what we do for all the other file methods, so this is just
making the implementation behaviour consistent with the rest of the
code.

FWIW, this also points out a problem with the copy_file_range()
definition - it does not say WTF should happen if the copy ranges
/overlap/ in the same file. clone is clear on that - support is
determined by the filesystem (i.e. "EINVAL [...] XFS and Btrfs do
not support overlapping reflink ranges in the same file."). For
copying, the fallback code can't copy the file data correctly if the
ranges overlap, so I've added checks to make this illegal and added
that overlapping ranges are not supported to the man page.....

These are the sort of API definition problems that I'm fixing with
right now, and I'm writing tests to make sure that all filesystems
will behave the same way for given copy scenarios.

i.e. I'm not doing this so I can get a NFS feature patchset merged,
I'm doing this to make the copy_file_range API well defined and
robust and allow implementations to be verified against the
specification the man page lays out.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  parent reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-30 20:03 [PATCH v2 00/10] server-side support for "inter" SSC copy Olga Kornievskaia
2018-11-30 20:03 ` [PATCH v2 01/10] VFS generic copy_file_range() support Olga Kornievskaia
2018-12-01  8:11   ` Amir Goldstein
2018-12-01 13:23     ` Olga Kornievskaia
2018-12-01 13:44       ` Olga Kornievskaia
     [not found]         ` <CAOQ4uxgENLCDH7QwtBPxA60dKEXvLVknBMY_Lgoetq_uQ=7gwA@mail.gmail.com>
     [not found]           ` <CAN-5tyFGV=fUCbAG5mSvy=LXDpdp8VG9Sh1aGMkBHQAG1Rp1sQ@mail.gmail.com>
2018-12-01 16:59             ` Amir Goldstein
2018-12-01 22:00     ` Dave Chinner
2018-12-02  3:12       ` Olga Kornievskaia
2018-12-02 15:19         ` Olga Kornievskaia
2018-12-02 20:47         ` Dave Chinner [this message]
2018-12-01 21:18   ` Matthew Wilcox
2018-12-01 22:36     ` Dave Chinner
2018-11-30 20:03 ` [PATCH v2 02/10] NFS fallback to generic_copy_file_range Olga Kornievskaia
2018-11-30 20:03 ` [PATCH v2 03/10] NFSD fill-in netloc4 structure Olga Kornievskaia
2018-11-30 20:03 ` [PATCH v2 04/10] NFSD add ca_source_server<> to COPY Olga Kornievskaia
2019-02-19 16:17   ` bfields
2018-11-30 20:03 ` [PATCH v2 05/10] NFSD return nfs4_stid in nfs4_preprocess_stateid_op Olga Kornievskaia
2018-11-30 20:03 ` [PATCH v2 06/10] NFSD add COPY_NOTIFY operation Olga Kornievskaia
2019-02-20  1:44   ` bfields
2019-02-20  2:07   ` bfields
2019-02-20 14:04     ` J. Bruce Fields
2019-02-20  2:12   ` bfields
2019-02-20  2:35   ` bfields
2019-06-14 19:11     ` Olga Kornievskaia
2018-11-30 20:03 ` [PATCH v2 07/10] NFSD check stateids against copy stateids Olga Kornievskaia
2018-11-30 20:03 ` [PATCH v2 08/10] NFSD generalize nfsd4_compound_state flag names Olga Kornievskaia
2018-11-30 20:03 ` [PATCH v2 09/10] NFSD: allow inter server COPY to have a STALE source server fh Olga Kornievskaia
2018-12-05 18:05   ` Olga Kornievskaia
2019-02-19 15:53   ` bfields
2018-11-30 20:03 ` [PATCH v2 10/10] NFSD add nfs4 inter ssc to nfsd4_copy Olga Kornievskaia
2019-02-19 15:54   ` bfields
2018-12-20 18:42 ` [PATCH v2 00/10] server-side support for "inter" SSC copy Olga Kornievskaia
2018-12-21 19:08   ` J. Bruce Fields
2019-01-14 14:53     ` Olga Kornievskaia
2019-01-16 22:08       ` J. Bruce Fields
2019-01-17 17:03         ` Olga Kornievskaia
2019-02-19 16:03         ` bfields

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181202204749.GS19305@dastard \
    --to=david@fromorbit.com \
    --cc=amir73il@gmail.com \
    --cc=bfields@redhat.com \
    --cc=jlayton@kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=olga.kornievskaia@gmail.com \
    --cc=stfrench@microsoft.com \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org linux-nfs@archiver.kernel.org
	public-inbox-index linux-nfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox