Re: [PATCH 01/18] vfs: introduce new file range exchange ioctl

From: "Darrick J. Wong" <djwong@kernel.org>
To: Amir Goldstein <amir73il@gmail.com>
Cc: linux-xfs <linux-xfs@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linux API <linux-api@vger.kernel.org>
Subject: Re: [PATCH 01/18] vfs: introduce new file range exchange ioctl
Date: Thu, 1 Apr 2021 17:37:16 -0700	[thread overview]
Message-ID: <20210402003716.GG4090233@magnolia> (raw)
In-Reply-To: <CAOQ4uxgJVUXxJmzSfHRFTFfqrV+oGt8QV-E+_wq46DmS0QGZ_w@mail.gmail.com>

On Thu, Apr 01, 2021 at 06:32:02AM +0300, Amir Goldstein wrote:
> On Thu, Apr 1, 2021 at 4:13 AM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Introduce a new ioctl to handle swapping ranges of bytes between files.
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  Documentation/filesystems/vfs.rst |   16 ++
> >  fs/ioctl.c                        |   42 +++++
> >  fs/remap_range.c                  |  283 +++++++++++++++++++++++++++++++++++++
> >  fs/xfs/libxfs/xfs_fs.h            |    1
> >  include/linux/fs.h                |   14 ++
> >  include/uapi/linux/fiexchange.h   |  101 +++++++++++++
> >  6 files changed, 456 insertions(+), 1 deletion(-)
> >  create mode 100644 include/uapi/linux/fiexchange.h
> >
> >
> > diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
> > index 2049bbf5e388..9f16b260bc7e 100644
> > --- a/Documentation/filesystems/vfs.rst
> > +++ b/Documentation/filesystems/vfs.rst
> > @@ -1006,6 +1006,8 @@ This describes how the VFS can manipulate an open file.  As of kernel
> >                 loff_t (*remap_file_range)(struct file *file_in, loff_t pos_in,
> >                                            struct file *file_out, loff_t pos_out,
> >                                            loff_t len, unsigned int remap_flags);
> > +                int (*xchg_file_range)(struct file *file1, struct file *file2,
> > +                                       struct file_xchg_range *fxr);
> 
> An obvious question: why is the xchgn_file_range op not using the
> unified remap_file_range() method with REMAP_XCHG_ flags?
> Surely replacing the remap_flags arg with struct file_remap_range.
> 
> I went to look for reasons and I didn't find them.
> Can you share your reasons for that?

Code simplicity.  The file2 freshness parameters don't apply to clone or
dedupe, and the current set of remap flags don't apply to exchange.  I'd
have to hunt down all the ->remap_range implementations and modify them
to error out on REMAP_FILE_EXCHANGE.  Multiplexing flags in this manner
would also require additional remap_flags interpretation code to
safeguard against callers who mix up which flags go with what piece of
functionality.

IOWS: it's not hard to do, but not something I want to do for an RFC
because the goal here is to gauge interest in having a userspace
interface at all.  Until I get to that point, tangling up the code
diverts my time towards rebasing and dealing with merge conflicts, at
the cost of time I can spend concentrating on making the algorithms
right.

--D

> Thanks,
> Amir.