Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
From: Amir Goldstein <amir73il@gmail.com>
To: Olga Kornievskaia <olga.kornievskaia@gmail.com>
Cc: bfields@redhat.com, Dave Chinner <david@fromorbit.com>,
	Jeff Layton <jlayton@kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2 01/10] VFS generic copy_file_range() support
Date: Sat, 1 Dec 2018 18:59:09 +0200
Message-ID: <CAOQ4uxjAq6Hj-MOh7AzGbUXn6KYpPzpEU=C4aOEySJ=O721cjg@mail.gmail.com> (raw)
In-Reply-To: <CAN-5tyFGV=fUCbAG5mSvy=LXDpdp8VG9Sh1aGMkBHQAG1Rp1sQ@mail.gmail.com>

On Sat, Dec 1, 2018 at 5:57 PM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
>
> On Sat, Dec 1, 2018 at 9:03 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> >
> >
> > On Sat, Dec 1, 2018, 3:44 PM Olga Kornievskaia <olga.kornievskaia@gmail.com wrote:
> >>
> >> On Sat, Dec 1, 2018 at 8:23 AM Olga Kornievskaia
> >> <olga.kornievskaia@gmail.com> wrote:
> >> >
> >> > On Sat, Dec 1, 2018 at 3:11 AM Amir Goldstein <amir73il@gmail.com> 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.
> >> > >
> >> > > You already had v7 patch reviewed-by 4 developers.
> >> > > What made you go and change it (and posted as v2)?
> >> > >
> >> > > Your intentions were good trying to fix the broken syscall, but
> >> > > I hope you understood that Dave didn't mean that you *have* to
> >> > > add the missing generic checks as part of your work. He just
> >> > > pointed out how broken the current interface is in the context of
> >> > > reviewing your patch.
> >> > >
> >> > > 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.
> >> > >
> >> > > If I were you, I would just go back to the reviewed v7 vfs patch.
> >> >
> >> > This is NOT a replacement to the v7 vfs patch??? This is a new patch
> >> > on top of that one.
> >> >
> >> > I assume that v7 patch has been OK-ed by everybody and is ready to go in???
> >> >
> >> > As you recall, what was left is to provide the functionality to relax
> >> > the check for the superblocks to be the same before calling the
> >> > do_splice_direct(). This patch attempt do this. I was under the
> >> > impression that to do so extra checks were needed to be added which I
> >> > added.
> >> >
> >>
> >> To clarify, previously I had a VFS patch with the client-side series
> >> to support "server to server" copy offload. It needed the
> >> functionality to be able to call copy_file_range with different super
> >> blocks.
> >>
> >> This patch series is for the server side support for the "server to
> >> server" copy offload. It requires ability to call copy_file_range()
> >> and do a copy between NFS and a local file system. Thus it needs
> >> generic_copy_file_range.
> >
> >
> > Ah. Sorry for the confusion.
> > My comment on change of behavior and refactoring in same patch still hold.
> > My comment about coordinate your work with Dave Chinner still hold.
>
> Understood. I will email Dave directly and coordinate.
>
> > Raise that with a comment about adding test coverage to the new
> > generic cross fs copy API to xfstest.
>
> What kind of extra coverage are you envisioning? Something that
> requires two different file systems mounted and then does a fs copy?
>

Yes, if you add this functionality you should add test coverage for the
added functionality. It's not going to be trivial to add cross fs type tests
to xfstests, but adding cross fs (same type) should be relatively easy
(copy_file_range from test fs to scratch fs).

> > Am I mistaken that this change affects any cross fs copy file range
> > by userspace and not only by kernel nfsd?
>
> That's correct, any cross fs copy is what I'm going for here.
>

Forgive me for being thick. After briefly going over the patches, I still don't
understand if you *need* to add generic cross fs copy to implement
server side copy support in nfsd? Or if you are adding it as an added bonus
to the community along with your SSC patch set?

The first two patches of the series seem unrelated to the rest, but maybe
I'm just not getting the connection?

Thanks,
Amir.

  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 [this message]
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
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='CAOQ4uxjAq6Hj-MOh7AzGbUXn6KYpPzpEU=C4aOEySJ=O721cjg@mail.gmail.com' \
    --to=amir73il@gmail.com \
    --cc=bfields@redhat.com \
    --cc=david@fromorbit.com \
    --cc=jlayton@kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=olga.kornievskaia@gmail.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