Message-ID: <CAOQ4uxgYmSLY25WtQjHxvViG0eNSSsswF77djBJZsSJCq1OyLA@mail.gmail.com>
Date: Tue, 3 Jun 2025 12:56:41 +0200
From: Amir Goldstein <amir73il@...il.com>
To: wangtao <tao.wangtao@...or.com>
Cc: sumit.semwal@...aro.org, christian.koenig@....com, kraxel@...hat.com, 
	vivek.kasireddy@...el.com, viro@...iv.linux.org.uk, brauner@...nel.org, 
	hughd@...gle.com, akpm@...ux-foundation.org, benjamin.gaignard@...labora.com, 
	Brian.Starkey@....com, jstultz@...gle.com, tjmercier@...gle.com, jack@...e.cz, 
	baolin.wang@...ux.alibaba.com, linux-media@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org, 
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	linux-mm@...ck.org, bintian.wang@...or.com, yipengxiang@...or.com, 
	liulu.liu@...or.com, feng.han@...or.com
Subject: Re: [PATCH v4 1/4] fs: allow cross-FS copy_file_range for memory file
 with direct I/O

On Tue, Jun 3, 2025 at 11:53 AM wangtao <tao.wangtao@...or.com> wrote:
>
> Memory files can optimize copy performance via copy_file_range callbacks:
> -Compared to mmap&read: reduces GUP (get_user_pages) overhead
> -Compared to sendfile/splice: eliminates one memory copy
> -Supports dma-buf direct I/O zero-copy implementation
>
> Suggested by: Christian König <christian.koenig@....com>
> Suggested by: Amir Goldstein <amir73il@...il.com>
> Signed-off-by: wangtao <tao.wangtao@...or.com>
> ---
>  fs/read_write.c    | 64 +++++++++++++++++++++++++++++++++++++---------
>  include/linux/fs.h |  2 ++
>  2 files changed, 54 insertions(+), 12 deletions(-)
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index bb0ed26a0b3a..ecb4f753c632 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1469,6 +1469,31 @@ COMPAT_SYSCALL_DEFINE4(sendfile64, int, out_fd, int, in_fd,
>  }
>  #endif
>
> +static const struct file_operations *memory_copy_file_ops(
> +                       struct file *file_in, struct file *file_out)
> +{
> +       if ((file_in->f_op->fop_flags & FOP_MEMORY_FILE) &&
> +           (file_in->f_mode & FMODE_CAN_ODIRECT) &&
> +           file_in->f_op->copy_file_range && file_out->f_op->write_iter)
> +               return file_in->f_op;
> +       else if ((file_out->f_op->fop_flags & FOP_MEMORY_FILE) &&
> +                (file_out->f_mode & FMODE_CAN_ODIRECT) &&
> +                file_in->f_op->read_iter && file_out->f_op->copy_file_range)
> +               return file_out->f_op;
> +       else
> +               return NULL;
> +}
> +
> +static int essential_file_rw_checks(struct file *file_in, struct file *file_out)
> +{
> +       if (!(file_in->f_mode & FMODE_READ) ||
> +           !(file_out->f_mode & FMODE_WRITE) ||
> +           (file_out->f_flags & O_APPEND))
> +               return -EBADF;
> +
> +       return 0;
> +}
> +
>  /*
>   * Performs necessary checks before doing a file copy
>   *
> @@ -1484,9 +1509,16 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
>         struct inode *inode_out = file_inode(file_out);
>         uint64_t count = *req_count;
>         loff_t size_in;
> +       bool splice = flags & COPY_FILE_SPLICE;
> +       const struct file_operations *mem_fops;
>         int ret;
>
> -       ret = generic_file_rw_checks(file_in, file_out);
> +       /* The dma-buf file is not a regular file. */
> +       mem_fops = memory_copy_file_ops(file_in, file_out);
> +       if (splice || mem_fops == NULL)

nit: use !mem_fops please

Considering that the flag COPY_FILE_SPLICE is not allowed
from userspace and is only called by nfsd and ksmbd
I think we should assert and deny the combination of
mem_fops && splice because it is very much unexpected.

After asserting this, it would be nicer to write as:
        if (mem_fops)
               ret = essential_file_rw_checks(file_in, file_out);
        else
               ret = generic_file_rw_checks(file_in, file_out);

> +       else
> +               ret = essential_file_rw_checks(file_in, file_out);
>         if (ret)
>                 return ret;
>
> @@ -1500,8 +1532,10 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
>          * and several different sets of file_operations, but they all end up
>          * using the same ->copy_file_range() function pointer.
>          */
> -       if (flags & COPY_FILE_SPLICE) {
> +       if (splice) {
>                 /* cross sb splice is allowed */
> +       } else if (mem_fops != NULL) {

With the assertion that splice && mem_fops is not allowed
if (splice || mem_fops) {

would go well together because they both allow cross-fs
copy not only cross sb.

> +               /* cross-fs copy is allowed for memory file. */
>         } else if (file_out->f_op->copy_file_range) {
>                 if (file_in->f_op->copy_file_range !=
>                     file_out->f_op->copy_file_range)
> @@ -1554,6 +1588,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>         ssize_t ret;
>         bool splice = flags & COPY_FILE_SPLICE;
>         bool samesb = file_inode(file_in)->i_sb == file_inode(file_out)->i_sb;
> +       const struct file_operations *mem_fops;
>
>         if (flags & ~COPY_FILE_SPLICE)
>                 return -EINVAL;
> @@ -1574,18 +1609,27 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>         if (len == 0)
>                 return 0;
>
> +       if (splice)
> +               goto do_splice;
> +
>         file_start_write(file_out);
>

goto do_splice needs to be after file_start_write

Please wait for feedback from vfs maintainers before posting another
version addressing my review comments.

Thanks,
Amir.

Powered by blists - more mailing lists