linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Amir Goldstein <amir73il@gmail.com>
To: Olga Kornievskaia <aglo@umich.edu>
Cc: Luis Henriques <lhenriques@suse.de>,
	Jeff Layton <jlayton@kernel.org>,
	Steve French <sfrench@samba.org>,
	Miklos Szeredi <miklos@szeredi.hu>,
	Trond Myklebust <trond.myklebust@hammerspace.com>,
	Anna Schumaker <anna.schumaker@netapp.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	Dave Chinner <dchinner@redhat.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	Nicolas Boichat <drinkcat@chromium.org>,
	Ian Lance Taylor <iant@google.com>,
	Luis Lozano <llozano@chromium.org>,
	Andreas Dilger <adilger@dilger.ca>,
	Christoph Hellwig <hch@infradead.org>,
	ceph-devel <ceph-devel@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	CIFS <linux-cifs@vger.kernel.org>,
	samba-technical <samba-technical@lists.samba.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v6] vfs: fix copy_file_range regression in cross-fs copies
Date: Fri, 19 Feb 2021 23:52:07 +0200	[thread overview]
Message-ID: <CAOQ4uxgeDx9M5YJJvxzJkbLEEyL0GioGFBggvvPEcpdjOoYSeA@mail.gmail.com> (raw)
In-Reply-To: <CAN-5tyGs9skFZ=ghd8Vz2F35S70QYi+kujdyRYLSkcEi8Jm9gw@mail.gmail.com>

On Fri, Feb 19, 2021 at 11:18 PM Olga Kornievskaia <aglo@umich.edu> wrote:
>
> On Thu, Feb 18, 2021 at 12:33 PM Luis Henriques <lhenriques@suse.de> wrote:
> >
> > A regression has been reported by Nicolas Boichat, found while using the
> > copy_file_range syscall to copy a tracefs file.  Before commit
> > 5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices") the
> > kernel would return -EXDEV to userspace when trying to copy a file across
> > different filesystems.  After this commit, the syscall doesn't fail anymore
> > and instead returns zero (zero bytes copied), as this file's content is
> > generated on-the-fly and thus reports a size of zero.
> >
> > This patch restores some cross-filesystem copy restrictions that existed
> > prior to commit 5dae222a5ff0 ("vfs: allow copy_file_range to copy across
> > devices").  Filesystems are still allowed to fall-back to the VFS
> > generic_copy_file_range() implementation, but that has now to be done
> > explicitly.
> >
> > nfsd is also modified to fall-back into generic_copy_file_range() in case
> > vfs_copy_file_range() fails with -EOPNOTSUPP or -EXDEV.
> >
> > Fixes: 5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices")
> > Link: https://lore.kernel.org/linux-fsdevel/20210212044405.4120619-1-drinkcat@chromium.org/
> > Link: https://lore.kernel.org/linux-fsdevel/CANMq1KDZuxir2LM5jOTm0xx+BnvW=ZmpsG47CyHFJwnw7zSX6Q@mail.gmail.com/
> > Link: https://lore.kernel.org/linux-fsdevel/20210126135012.1.If45b7cdc3ff707bc1efa17f5366057d60603c45f@changeid/
> > Reported-by: Nicolas Boichat <drinkcat@chromium.org>
> > Signed-off-by: Luis Henriques <lhenriques@suse.de>
> > ---
> > And v6 is upon us.  Behold!
>
>
> > Changes since v5
> > - check if ->copy_file_range is NULL before calling it
> > Changes since v4
> > - nfsd falls-back to generic_copy_file_range() only *if* it gets -EOPNOTSUPP
> >   or -EXDEV.
> > Changes since v3
> > - dropped the COPY_FILE_SPLICE flag
> > - kept the f_op's checks early in generic_copy_file_checks, implementing
> >   Amir's suggestions
> > - modified nfsd to use generic_copy_file_range()
> > Changes since v2
> > - do all the required checks earlier, in generic_copy_file_checks(),
> >   adding new checks for ->remap_file_range
> > - new COPY_FILE_SPLICE flag
> > - don't remove filesystem's fallback to generic_copy_file_range()
> > - updated commit changelog (and subject)
> > Changes since v1 (after Amir review)
> > - restored do_copy_file_range() helper
> > - return -EOPNOTSUPP if fs doesn't implement CFR
> > - updated commit description
> >
> >  fs/nfsd/vfs.c   |  8 +++++++-
> >  fs/read_write.c | 53 ++++++++++++++++++++++++-------------------------
> >  2 files changed, 33 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 04937e51de56..23dab0fa9087 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -568,6 +568,7 @@ __be32 nfsd4_clone_file_range(struct nfsd_file *nf_src, u64 src_pos,
> >  ssize_t nfsd_copy_file_range(struct file *src, u64 src_pos, struct file *dst,
> >                              u64 dst_pos, u64 count)
> >  {
> > +       ssize_t ret;
> >
> >         /*
> >          * Limit copy to 4MB to prevent indefinitely blocking an nfsd
> > @@ -578,7 +579,12 @@ ssize_t nfsd_copy_file_range(struct file *src, u64 src_pos, struct file *dst,
> >          * limit like this and pipeline multiple COPY requests.
> >          */
> >         count = min_t(u64, count, 1 << 22);
> > -       return vfs_copy_file_range(src, src_pos, dst, dst_pos, count, 0);
> > +       ret = vfs_copy_file_range(src, src_pos, dst, dst_pos, count, 0);
> > +
> > +       if (ret == -EOPNOTSUPP || ret == -EXDEV)
> > +               ret = generic_copy_file_range(src, src_pos, dst, dst_pos,
> > +                                             count, 0);
> > +       return ret;
> >  }
> >
> >  __be32 nfsd4_vfs_fallocate(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index 75f764b43418..0348aaa9e237 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -1388,28 +1388,6 @@ ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
> >  }
> >  EXPORT_SYMBOL(generic_copy_file_range);
> >
> > -static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in,
> > -                                 struct file *file_out, loff_t pos_out,
> > -                                 size_t len, unsigned int flags)
> > -{
> > -       /*
> > -        * Although we now allow filesystems to handle cross sb copy, passing
> > -        * a file of the wrong filesystem type to filesystem driver can result
> > -        * in an attempt to dereference the wrong type of ->private_data, so
> > -        * avoid doing that until we really have a good reason.  NFS defines
> > -        * several different file_system_type structures, but they all end up
> > -        * using the same ->copy_file_range() function pointer.
> > -        */
> > -       if (file_out->f_op->copy_file_range &&
> > -           file_out->f_op->copy_file_range == file_in->f_op->copy_file_range)
> > -               return file_out->f_op->copy_file_range(file_in, pos_in,
> > -                                                      file_out, pos_out,
> > -                                                      len, flags);
> > -
> > -       return generic_copy_file_range(file_in, pos_in, file_out, pos_out, len,
> > -                                      flags);
> > -}
> > -
> >  /*
> >   * Performs necessary checks before doing a file copy
> >   *
> > @@ -1427,6 +1405,25 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
> >         loff_t size_in;
> >         int ret;
> >
> > +       /*
> > +        * Although we now allow filesystems to handle cross sb copy, passing
> > +        * a file of the wrong filesystem type to filesystem driver can result
> > +        * in an attempt to dereference the wrong type of ->private_data, so
> > +        * avoid doing that until we really have a good reason.  NFS defines
> > +        * several different file_system_type structures, but they all end up
> > +        * using the same ->copy_file_range() function pointer.
> > +        */
> > +       if (file_out->f_op->copy_file_range) {
> > +               if (file_in->f_op->copy_file_range !=
> > +                   file_out->f_op->copy_file_range)
> > +                       return -EXDEV;
> > +       } else if (file_in->f_op->remap_file_range) {
> > +               if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
> > +                       return -EXDEV;
> > +       } else {
> > +                return -EOPNOTSUPP;
> > +       }
> > +
> >         ret = generic_file_rw_checks(file_in, file_out);
> >         if (ret)
> >                 return ret;
> > @@ -1499,8 +1496,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> >          * Try cloning first, this is supported by more file systems, and
> >          * more efficient if both clone and copy are supported (e.g. NFS).
> >          */
> > -       if (file_in->f_op->remap_file_range &&
> > -           file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) {
> > +       if (file_in->f_op->remap_file_range) {
> >                 loff_t cloned;
>
> This chunk breaks NFS. You are removing the check that the source and
> destination for the CLONE operation are the same superblock and that
> leads to the fact that when NFS does a copy between 2 different NFS
> servers, it would try CLONE first which is not allowed. NFS relied on
> this check to be done by the VFS layer. Either don't remove it or,
> otherwise, fix the NFS clone's code to not send the CLONE and error
> accordingly so that the COPY is done as it should have been.
>

Right, we need to add this check back (not only for NFS).

However, I was looking at the change that introduced this opportunistic
call for clone_file_range() into copy_file_range():

commit a76b5b04375f974579c83433b06466758c0c552c
Author: Christoph Hellwig <hch@lst.de>
Date:   Fri Dec 9 16:17:19 2016 -0800

    fs: try to clone files first in vfs_copy_file_range

    A clone is a perfectly fine implementation of a file copy, so most
    file systems just implement the copy that way.  Instead of duplicating
    this logic move it to the VFS.  Currently btrfs and XFS implement copies
    the same way as clones and there is no behavior change for them, cifs
    only implements clones and grow support for copy_file_range with this
    patch.  NFS implements both, so this will allow copy_file_range to work
    on servers that only implement CLONE and be lot more efficient on servers
    that implements CLONE and COPY.

And I was thinking to myself that like the change that brought us here
("vfs: allow copy_file_range to copy across devices"), this change was done
for a certain purpose (serve copy_file_range() by fs that implement CLONE),
but that last part (prefer CLONE over COPY) also sounds like an optimization
that nobody asked for and could lead to unexpected behavior down the road.

I think that if a filesystem implements both methods (COPY and CLONE)
and user called to COPY API, we need to call the more specialized COPY
method and not try the CLONE method, because filesystem should be very
capable of making this optimization internally.

This could have been a hypothetical question, but there are actually
two filesystems that implement both COPY and CLONE, so let's ask the
developers what they think VFS should call.

Olga, Trond, Steve, which methods of your filesystem do you think that
vfs_copy_file_range() should call?
1. Only copy_file_range()?
2. Both copy_file_range() and remap_file_range()?
3. CLONE before COPY or the other way around?

Thanks,
Amir.

  reply	other threads:[~2021-02-19 21:53 UTC|newest]

Thread overview: 136+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-12  4:43 [PATCH 0/6] Add generated flag to filesystem struct to block copy_file_range Nicolas Boichat
2021-02-12  4:44 ` [PATCH 1/6] fs: Add flag to file_system_type to indicate content is generated Nicolas Boichat
2021-02-12  7:46   ` Greg KH
2021-02-12  8:20     ` Nicolas Boichat
2021-02-12  8:37       ` Greg KH
2021-02-12 15:33         ` Ian Lance Taylor
2021-02-12 15:45           ` Greg KH
2021-02-12 15:59             ` Ian Lance Taylor
2021-02-12 16:28               ` Greg KH
2021-02-12 20:22                 ` Ian Lance Taylor
2021-02-12 23:03             ` Dave Chinner
2021-02-12 23:07               ` Ian Lance Taylor
2021-02-12 23:27                 ` Dave Chinner
2021-02-12 23:54                   ` Darrick J. Wong
2021-02-15  0:38                     ` Dave Chinner
2021-02-15  1:12                       ` Ian Lance Taylor
2021-02-15  1:25                         ` Nicolas Boichat
2021-02-15  5:56                           ` Amir Goldstein
2021-02-15  8:30                           ` Greg KH
2021-02-12  8:22     ` Amir Goldstein
2021-02-12  8:39       ` Greg KH
2021-02-12 12:05         ` Luis Henriques
2021-02-12 12:18           ` Greg KH
2021-02-12 12:41             ` Luis Henriques
2021-02-12 14:11               ` Greg KH
2021-02-12 15:01                 ` Luis Henriques
2021-02-15  6:12               ` Amir Goldstein
2021-02-15 10:39                 ` Luis Henriques
2021-02-15 12:22                   ` Luis Henriques
2021-02-15 14:23                     ` Amir Goldstein
2021-02-15 14:51                       ` Luis Henriques
2021-02-15 15:43                       ` [PATCH v2] vfs: prevent copy_file_range to copy across devices Luis Henriques
2021-02-15 16:02                         ` Trond Myklebust
2021-02-16  0:25                           ` Steve French
2021-02-15 16:34                         ` Amir Goldstein
2021-02-15 16:53                           ` Trond Myklebust
2021-02-15 17:24                             ` Amir Goldstein
2021-02-15 18:57                               ` Trond Myklebust
2021-02-15 19:43                                 ` Amir Goldstein
2021-02-16 11:17                                   ` Luis Henriques
2021-02-16 11:28                                     ` gregkh
2021-02-16 12:01                                       ` Luis Henriques
2021-02-16 12:08                                         ` Greg KH
2021-02-16 13:51                                     ` Amir Goldstein
2021-02-16 16:42                                       ` Luis Henriques
2021-02-16 17:44                                         ` Amir Goldstein
2021-02-16 18:55                                           ` Luis Henriques
2021-02-16 19:20                                             ` Amir Goldstein
2021-02-16 19:27                                               ` Anna Schumaker
2021-02-16 19:31                                                 ` Steve French
2021-02-16 19:40                                                   ` Amir Goldstein
2021-02-16 21:15                                                     ` Steve French
2021-02-17  8:08                                                       ` Amir Goldstein
2021-02-17 17:26                                                         ` [PATCH v3] vfs: fix copy_file_range regression in cross-fs copies Luis Henriques
2021-02-17 20:47                                                           ` Amir Goldstein
2021-02-18  0:56                                                           ` Nicolas Boichat
2021-02-18  5:32                                                           ` Olga Kornievskaia
2021-02-18  6:47                                                             ` Amir Goldstein
2021-02-18 16:28                                                               ` Olga Kornievskaia
2021-02-18  7:43                                                           ` Christoph Hellwig
2021-02-18  0:50                                                         ` [PATCH v2] vfs: prevent copy_file_range to copy across devices Andreas Dilger
2021-02-18  7:34                                                           ` gregkh
2021-02-16 18:54                                       ` Andreas Dilger
2021-02-17  4:45                         ` Nicolas Boichat
2021-02-18  7:42                         ` Christoph Hellwig
2021-02-18  9:10                           ` Amir Goldstein
2021-02-18 10:29                             ` Luis Henriques
2021-02-18 12:15                               ` Luis Henriques
2021-02-18 12:49                                 ` Amir Goldstein
2021-02-18 14:36                                   ` [PATCH v4] vfs: fix copy_file_range regression in cross-fs copies Luis Henriques
2021-02-18 14:58                                     ` Amir Goldstein
2021-02-18 15:17                                       ` [PATCH v5] " Luis Henriques
2021-02-18 15:53                                         ` Amir Goldstein
2021-02-18 16:35                                           ` Luis Henriques
2021-02-18 17:18                                             ` [PATCH v6] " Luis Henriques
2021-02-19 21:18                                               ` Olga Kornievskaia
2021-02-19 21:52                                                 ` Amir Goldstein [this message]
2021-02-21 19:58                                                 ` [PATCH v7] " Luis Henriques
2021-02-22  3:00                                                   ` Nicolas Boichat
2021-02-22 10:24                                                   ` [PATCH v8] " Luis Henriques
2021-02-22 10:46                                                     ` Amir Goldstein
2021-02-22 16:25                                                     ` dai.ngo
2021-02-23 10:32                                                       ` Luis Henriques
2021-02-23 15:28                                                         ` Amir Goldstein
2021-02-23 15:29                                                         ` dai.ngo
2021-02-23 16:02                                                           ` dai.ngo
2021-02-23 16:47                                                             ` Amir Goldstein
2021-02-23 16:57                                                               ` dai.ngo
     [not found]                                                                 ` <e3eed18b-fc7e-e687-608b-7f662017329c@oracle.com>
2021-02-23 17:33                                                                   ` Amir Goldstein
2021-02-24  0:13                                                                     ` dai.ngo
2021-02-23 17:56                                                                 ` Luis Henriques
2021-02-23 17:13                                                             ` Olga Kornievskaia
2021-02-24  1:00                                                     ` Olga Kornievskaia
2021-02-24 10:23                                                       ` Luis Henriques
2021-02-24 10:44                                                         ` Nicolas Boichat
2021-04-09  5:23                                                           ` Nicolas Boichat
2021-04-09 13:39                                                             ` Luis Henriques
2021-04-09 13:50                                                               ` Amir Goldstein
2021-04-23  4:40                                                                 ` Nicolas Boichat
2021-05-03  8:54                                                                   ` Luis Henriques
2021-05-10  4:59                                                                     ` Amir Goldstein
2021-05-10  9:10                                                                       ` Luis Henriques
2021-02-24 14:23                                                     ` [PATCH] copy_file_range.2: Kernel v5.12 updates Luis Henriques
2021-02-24 16:10                                                       ` Amir Goldstein
2021-02-25 10:21                                                         ` Luis Henriques
2021-02-26 10:13                                                           ` Alejandro Colomar (man-pages)
2021-02-26 10:34                                                             ` Amir Goldstein
2021-02-26 11:15                                                               ` Alejandro Colomar (man-pages)
2021-02-26 13:59                                                                 ` Jeff Layton
2021-02-26 21:26                                                                   ` Alejandro Colomar (man-pages)
2021-02-26 22:18                                                         ` Alejandro Colomar (man-pages)
2021-02-27  5:41                                                           ` Amir Goldstein
2021-02-27 12:20                                                             ` Alejandro Colomar (man-pages)
2021-02-27 13:49                                                               ` [RFC v2] copy_file_range.2: Update cross-filesystem support for 5.12 Alejandro Colomar
2021-02-27 16:00                                                                 ` Amir Goldstein
2021-02-27 23:08                                                             ` [PATCH] copy_file_range.2: Kernel v5.12 updates Steve French
2021-02-28  7:35                                                               ` Amir Goldstein
2021-02-28 22:25                                                                 ` Steve French
2021-03-01  6:18                                                                   ` Amir Goldstein
2021-03-01 14:41                                                       ` [RFC v3] copy_file_range.2: Update cross-filesystem support for 5.12 Alejandro Colomar
2021-03-01 14:58                                                         ` Amir Goldstein
2021-03-04  9:38                                                       ` [RFC v4] " Alejandro Colomar
2021-03-04 17:13                                                         ` Darrick J. Wong
2021-03-04 18:24                                                           ` Alejandro Colomar (man-pages)
2021-03-04 23:50                                                             ` Darrick J. Wong
2021-02-24  7:15                                     ` [PATCH v4] vfs: fix copy_file_range regression in cross-fs copies Amir Goldstein
2021-02-24  8:30                                       ` Petr Vorel
2021-02-18 20:41                             ` [PATCH v2] vfs: prevent copy_file_range to copy across devices Steve French
2021-02-12 23:15       ` [PATCH 1/6] fs: Add flag to file_system_type to indicate content is generated Dave Chinner
2021-02-12  7:54   ` Amir Goldstein
2021-02-12  4:44 ` [PATCH 2/6] proc: Add FS_GENERATED_CONTENT to filesystem flags Nicolas Boichat
2021-02-12  4:44 ` [PATCH 6/6] vfs: Disallow copy_file_range on generated file systems Nicolas Boichat
2021-02-12  4:53   ` Darrick J. Wong
2021-02-12  4:59     ` Darrick J. Wong
2021-02-12  5:24       ` Nicolas Boichat
2021-02-14 23:09 ` [PATCH 0/6] Add generated flag to filesystem struct to block copy_file_range Al Viro

Reply instructions:

You may reply publicly 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=CAOQ4uxgeDx9M5YJJvxzJkbLEEyL0GioGFBggvvPEcpdjOoYSeA@mail.gmail.com \
    --to=amir73il@gmail.com \
    --cc=adilger@dilger.ca \
    --cc=aglo@umich.edu \
    --cc=anna.schumaker@netapp.com \
    --cc=ceph-devel@vger.kernel.org \
    --cc=darrick.wong@oracle.com \
    --cc=dchinner@redhat.com \
    --cc=drinkcat@chromium.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=iant@google.com \
    --cc=jlayton@kernel.org \
    --cc=lhenriques@suse.de \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=llozano@chromium.org \
    --cc=miklos@szeredi.hu \
    --cc=samba-technical@lists.samba.org \
    --cc=sfrench@samba.org \
    --cc=trond.myklebust@hammerspace.com \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH v6] vfs: fix copy_file_range regression in cross-fs copies' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).