All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg KH <gregkh@linuxfoundation.org>
To: Luis Henriques <lhenriques@suse.de>
Cc: Jeff Layton <jlayton@kernel.org>,
	Amir Goldstein <amir73il@gmail.com>,
	Nicolas Boichat <drinkcat@chromium.org>,
	"Darrick J . Wong" <djwong@kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Ian Lance Taylor <iant@google.com>,
	Luis Lozano <llozano@chromium.org>,
	Dave Chinner <david@fromorbit.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/6] fs: Add flag to file_system_type to indicate content is generated
Date: Fri, 12 Feb 2021 15:11:14 +0100	[thread overview]
Message-ID: <YCaMgtpCzPrLjw9c@kroah.com> (raw)
In-Reply-To: <87sg61ihkj.fsf@suse.de>

On Fri, Feb 12, 2021 at 12:41:48PM +0000, Luis Henriques wrote:
> Greg KH <gregkh@linuxfoundation.org> writes:
> 
> > On Fri, Feb 12, 2021 at 12:05:14PM +0000, Luis Henriques wrote:
> >> Greg KH <gregkh@linuxfoundation.org> writes:
> >> 
> >> > On Fri, Feb 12, 2021 at 10:22:16AM +0200, Amir Goldstein wrote:
> >> >> On Fri, Feb 12, 2021 at 9:49 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >> >> >
> >> >> > On Fri, Feb 12, 2021 at 12:44:00PM +0800, Nicolas Boichat wrote:
> >> >> > > Filesystems such as procfs and sysfs generate their content at
> >> >> > > runtime. This implies the file sizes do not usually match the
> >> >> > > amount of data that can be read from the file, and that seeking
> >> >> > > may not work as intended.
> >> >> > >
> >> >> > > This will be useful to disallow copy_file_range with input files
> >> >> > > from such filesystems.
> >> >> > >
> >> >> > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> >> >> > > ---
> >> >> > > I first thought of adding a new field to struct file_operations,
> >> >> > > but that doesn't quite scale as every single file creation
> >> >> > > operation would need to be modified.
> >> >> >
> >> >> > Even so, you missed a load of filesystems in the kernel with this patch
> >> >> > series, what makes the ones you did mark here different from the
> >> >> > "internal" filesystems that you did not?
> >> >> >
> >> >> > This feels wrong, why is userspace suddenly breaking?  What changed in
> >> >> > the kernel that caused this?  Procfs has been around for a _very_ long
> >> >> > time :)
> >> >> 
> >> >> That would be because of (v5.3):
> >> >> 
> >> >> 5dae222a5ff0 vfs: allow copy_file_range to copy across devices
> >> >> 
> >> >> The intention of this change (series) was to allow server side copy
> >> >> for nfs and cifs via copy_file_range().
> >> >> This is mostly work by Dave Chinner that I picked up following requests
> >> >> from the NFS folks.
> >> >> 
> >> >> But the above change also includes this generic change:
> >> >> 
> >> >> -       /* this could be relaxed once a method supports cross-fs copies */
> >> >> -       if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
> >> >> -               return -EXDEV;
> >> >> -
> >> >> 
> >> >> The change of behavior was documented in the commit message.
> >> >> It was also documented in:
> >> >> 
> >> >> 88e75e2c5 copy_file_range.2: Kernel v5.3 updates
> >> >> 
> >> >> I think our rationale for the generic change was:
> >> >> "Why not? What could go wrong? (TM)"
> >> >> I am not sure if any workload really gained something from this
> >> >> kernel cross-fs CFR.
> >> >
> >> > Why not put that check back?
> >> >
> >> >> In retrospect, I think it would have been safer to allow cross-fs CFR
> >> >> only to the filesystems that implement ->{copy,remap}_file_range()...
> >> >
> >> > Why not make this change?  That seems easier and should fix this for
> >> > everyone, right?
> >> >
> >> >> Our option now are:
> >> >> - Restore the cross-fs restriction into generic_copy_file_range()
> >> >
> >> > Yes.
> >> >
> >> 
> >> Restoring this restriction will actually change the current cephfs CFR
> >> behaviour.  Since that commit we have allowed doing remote copies between
> >> different filesystems within the same ceph cluster.  See commit
> >> 6fd4e6348352 ("ceph: allow object copies across different filesystems in
> >> the same cluster").
> >> 
> >> Although I'm not aware of any current users for this scenario, the
> >> performance impact can actually be huge as it's the difference between
> >> asking the OSDs for copying a file and doing a full read+write on the
> >> client side.
> >
> > Regression in performance is ok if it fixes a regression for things that
> > used to work just fine in the past :)
> >
> > First rule, make it work.
> 
> Sure, I just wanted to point out that *maybe* there are other options than
> simply reverting that commit :-)
> 
> Something like the patch below (completely untested!) should revert to the
> old behaviour in filesystems that don't implement the CFR syscall.
> 
> Cheers,
> -- 
> Luis
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 75f764b43418..bf5dccc43cc9 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1406,8 +1406,11 @@ static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in,
>  						       file_out, pos_out,
>  						       len, flags);
>  
> -	return generic_copy_file_range(file_in, pos_in, file_out, pos_out, len,
> -				       flags);
> +	if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
> +		return -EXDEV;
> +	else
> +		generic_copy_file_range(file_in, pos_in, file_out, pos_out, len,
> +					flags);
>  }
>  
>  /*

That would make much more sense to me.

  reply	other threads:[~2021-02-12 14:12 UTC|newest]

Thread overview: 146+ 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 [this message]
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
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  5:25                                                     ` [vfs] cb07c976be: ltp.copy_file_range01.fail kernel test robot
2021-02-24  5:25                                                       ` kernel test robot
2021-02-24  5:25                                                       ` [LTP] " kernel test robot
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-25 15:29                                                           ` AW: " Walter Harms
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  7:15                                       ` [LTP] " Amir Goldstein
2021-02-24  8:30                                       ` Petr Vorel
2021-02-24  8:30                                         ` [LTP] " 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 3/6] sysfs: " Nicolas Boichat
2021-02-12  4:44 ` [PATCH 4/6] debugfs: " Nicolas Boichat
2021-02-12  4:44 ` [PATCH 5/6] tracefs: " Nicolas Boichat
2021-02-12 14:47   ` Steven Rostedt
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=YCaMgtpCzPrLjw9c@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=amir73il@gmail.com \
    --cc=david@fromorbit.com \
    --cc=djwong@kernel.org \
    --cc=drinkcat@chromium.org \
    --cc=iant@google.com \
    --cc=jlayton@kernel.org \
    --cc=lhenriques@suse.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=llozano@chromium.org \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.