linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Luis Henriques <lhenriques@suse.de>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Trond Myklebust <trondmy@hammerspace.com>,
	"samba-technical@lists.samba.org"
	<samba-technical@lists.samba.org>,
	"drinkcat@chromium.org" <drinkcat@chromium.org>,
	"iant@google.com" <iant@google.com>,
	"linux-cifs@vger.kernel.org" <linux-cifs@vger.kernel.org>,
	"darrick.wong@oracle.com" <darrick.wong@oracle.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"jlayton@kernel.org" <jlayton@kernel.org>,
	"anna.schumaker@netapp.com" <anna.schumaker@netapp.com>,
	"llozano@chromium.org" <llozano@chromium.org>,
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	"miklos@szeredi.hu" <miklos@szeredi.hu>,
	"viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
	"dchinner@redhat.com" <dchinner@redhat.com>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"sfrench@samba.org" <sfrench@samba.org>,
	"ceph-devel@vger.kernel.org" <ceph-devel@vger.kernel.org>
Subject: Re: [PATCH v2] vfs: prevent copy_file_range to copy across devices
Date: Tue, 16 Feb 2021 11:17:34 +0000	[thread overview]
Message-ID: <87r1lgjm7l.fsf@suse.de> (raw)
In-Reply-To: <CAOQ4uxjUf15fDjz11pCzT3GkFmw=2ySXR_6XF-Bf-TfUwpj77Q@mail.gmail.com> (Amir Goldstein's message of "Mon, 15 Feb 2021 21:43:03 +0200")

Amir Goldstein <amir73il@gmail.com> writes:

> On Mon, Feb 15, 2021 at 8:57 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>>
>> On Mon, 2021-02-15 at 19:24 +0200, Amir Goldstein wrote:
>> > On Mon, Feb 15, 2021 at 6:53 PM Trond Myklebust <
>> > trondmy@hammerspace.com> wrote:
>> > >
>> > > On Mon, 2021-02-15 at 18:34 +0200, Amir Goldstein wrote:
>> > > > On Mon, Feb 15, 2021 at 5:42 PM Luis Henriques <
>> > > > lhenriques@suse.de>
>> > > > wrote:
>> > > > >
>> > > > > Nicolas Boichat reported an issue when trying to use the
>> > > > > copy_file_range
>> > > > > syscall on a tracefs file.  It failed silently because the file
>> > > > > content is
>> > > > > generated on-the-fly (reporting a size of zero) and
>> > > > > copy_file_range
>> > > > > needs
>> > > > > to know in advance how much data is present.
>> > > > >
>> > > > > This commit restores the cross-fs restrictions that existed
>> > > > > prior
>> > > > > to
>> > > > > 5dae222a5ff0 ("vfs: allow copy_file_range to copy across
>> > > > > devices")
>> > > > > and
>> > > > > removes generic_copy_file_range() calls from ceph, cifs, fuse,
>> > > > > and
>> > > > > nfs.
>> > > > >
>> > > > > Fixes: 5dae222a5ff0 ("vfs: allow copy_file_range to copy across
>> > > > > devices")
>> > > > > Link:
>> > > > > https://lore.kernel.org/linux-fsdevel/20210212044405.4120619-1-drinkcat@chromium.org/
>> > > > > Cc: Nicolas Boichat <drinkcat@chromium.org>
>> > > > > Signed-off-by: Luis Henriques <lhenriques@suse.de>
>> > > >
>> > > > Code looks ok.
>> > > > You may add:
>> > > >
>> > > > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>> > > >
>> > > > I agree with Trond that the first paragraph of the commit message
>> > > > could
>> > > > be improved.
>> > > > The purpose of this change is to fix the change of behavior that
>> > > > caused the regression.
>> > > >
>> > > > Before v5.3, behavior was -EXDEV and userspace could fallback to
>> > > > read.
>> > > > After v5.3, behavior is zero size copy.
>> > > >
>> > > > It does not matter so much what makes sense for CFR to do in this
>> > > > case (generic cross-fs copy).  What matters is that nobody asked
>> > > > for
>> > > > this change and that it caused problems.
>> > > >
>> > >
>> > > No. I'm saying that this patch should be NACKed unless there is a
>> > > real
>> > > explanation for why we give crap about this tracefs corner case and
>> > > why
>> > > it can't be fixed.
>> > >
>> > > There are plenty of reasons why copy offload across filesystems
>> > > makes
>> > > sense, and particularly when you're doing NAS. Clone just doesn't
>> > > cut
>> > > it when it comes to disaster recovery (whereas backup to a
>> > > different
>> > > storage unit does). If the client has to do the copy, then you're
>> > > effectively doubling the load on the server, and you're adding
>> > > potentially unnecessary network traffic (or at the very least you
>> > > are
>> > > doubling that traffic).
>> > >
>> >
>> > I don't understand the use case you are describing.
>> >
>> > Which filesystem types are you talking about for source and target
>> > of copy_file_range()?
>> >
>> > To be clear, the original change was done to support NFS/CIFS server-
>> > side
>> > copy and those should not be affected by this change.
>> >
>>
>> That is incorrect:
>>
>> ssize_t nfsd_copy_file_range(struct file *src, u64 src_pos, struct file
>> *dst,
>>  u64 dst_pos, u64 count)
>> {
>>
>>  /*
>>  * Limit copy to 4MB to prevent indefinitely blocking an nfsd
>>  * thread and client rpc slot. The choice of 4MB is somewhat
>>  * arbitrary. We might instead base this on r/wsize, or make it
>>  * tunable, or use a time instead of a byte limit, or implement
>>  * asynchronous copy. In theory a client could also recognize a
>>  * 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);
>> }
>>
>> You are now explicitly changing the behaviour of knfsd when the source
>> and destination filesystem differ.
>>
>> For one thing, you are disallowing the NFSv4.2 copy offload use case of
>> copying from a local filesystem to a remote NFS server. However you are
>> also disallowing the copy from, say, an XFS formatted partition to an
>> ext4 partition.
>>
>
> Got it.

Ugh.  And I guess overlayfs may have a similar problem.

> This is easy to solve with a flag COPY_FILE_SPLICE (or something) that
> is internal to kernel users.
>
> FWIW, you may want to look at the loop in ovl_copy_up_data()
> for improvements to nfsd_copy_file_range().
>
> We can move the check out to copy_file_range syscall:
>
>         if (flags != 0)
>                 return -EINVAL;
>
> Leave the fallback from all filesystems and check for the
> COPY_FILE_SPLICE flag inside generic_copy_file_range().

Ok, the diff bellow is just to make sure I understood your suggestion.

The patch will also need to:

 - change nfs and overlayfs calls to vfs_copy_file_range() so that they
   use the new flag.

 - check flags in generic_copy_file_checks() to make sure only valid flags
   are used (COPY_FILE_SPLICE at the moment).

Also, where should this flag be defined?  include/uapi/linux/fs.h?

Cheers,
-- 
Luis

diff --git a/fs/read_write.c b/fs/read_write.c
index 75f764b43418..341d315d2a96 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1383,6 +1383,13 @@ ssize_t generic_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)
 {
+	if (!(flags & COPY_FILE_SPLICE)) {
+		if (!file_out->f_op->copy_file_range)
+			return -EOPNOTSUPP;
+		else if (file_out->f_op->copy_file_range !=
+			 file_in->f_op->copy_file_range)
+			return -EXDEV;
+	}
 	return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
 				len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
 }
@@ -1474,9 +1481,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 {
 	ssize_t ret;
 
-	if (flags != 0)
-		return -EINVAL;
-
 	ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len,
 				       flags);
 	if (unlikely(ret))

  reply	other threads:[~2021-02-16 11:17 UTC|newest]

Thread overview: 93+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CAOQ4uxiFGjdvX2-zh5o46pn7RZhvbGHH0wpzLPuPOom91FwWeQ@mail.gmail.com>
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 [this message]
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
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-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-18 20:41       ` [PATCH v2] vfs: prevent copy_file_range to copy across devices Steve French

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=87r1lgjm7l.fsf@suse.de \
    --to=lhenriques@suse.de \
    --cc=amir73il@gmail.com \
    --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=iant@google.com \
    --cc=jlayton@kernel.org \
    --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=trondmy@hammerspace.com \
    --cc=viro@zeniv.linux.org.uk \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).