Re: [PATCH v2 3/3] fuse: add COPY_FILE_RANGE_64 that allows large copies

[Joanne Koong <joannelkoong@gmail.com>]

On Wed, Aug 13, 2025 at 8:24 AM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> The FUSE protocol uses struct fuse_write_out to convey the return value of
> copy_file_range, which is restricted to uint32_t.  But the COPY_FILE_RANGE
> interface supports a 64-bit size copies and there's no reason why copies
> should be limited to 32-bit.
>
> Introduce a new op COPY_FILE_RANGE_64, which is identical, except the
> number of bytes copied is returned in a 64-bit value.
>
> If the fuse server does not support COPY_FILE_RANGE_64, fall back to
> COPY_FILE_RANGE.

Is it unacceptable to add a union in struct fuse_write_out that
accepts a uint64_t bytes_copied?
struct fuse_write_out {
    union {
        struct {
            uint32_t size;
            uint32_t padding;
        };
        uint64_t bytes_copied;
    };
};

Maybe a little ugly but that seems backwards-compatible to me and
would prevent needing a new FUSE_COPY_FILE_RANGE64.

>
> Reported-by: Florian Weimer <fweimer@redhat.com>
> Closes: https://lore.kernel.org/all/lhuh5ynl8z5.fsf@oldenburg.str.redhat.com/
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/fuse/file.c            | 44 ++++++++++++++++++++++++++++-----------
>  fs/fuse/fuse_i.h          |  3 +++
>  include/uapi/linux/fuse.h | 12 ++++++++++-
>  3 files changed, 46 insertions(+), 13 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 4adcf09d4b01..867b5fde1237 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -3013,33 +3015,51 @@ static ssize_t __fuse_copy_file_range(struct file *file_in, loff_t pos_in,
>         if (is_unstable)
>                 set_bit(FUSE_I_SIZE_UNSTABLE, &fi_out->state);
>
> -       args.opcode = FUSE_COPY_FILE_RANGE;
> +       args.opcode = FUSE_COPY_FILE_RANGE_64;
>         args.nodeid = ff_in->nodeid;
>         args.in_numargs = 1;
>         args.in_args[0].size = sizeof(inarg);
>         args.in_args[0].value = &inarg;
>         args.out_numargs = 1;
> -       args.out_args[0].size = sizeof(outarg);
> -       args.out_args[0].value = &outarg;
> +       args.out_args[0].size = sizeof(outarg_64);
> +       args.out_args[0].value = &outarg_64;
> +       if (fc->no_copy_file_range_64) {
> +fallback:
> +               /* Fall back to old op that can't handle large copy length */
> +               args.opcode = FUSE_COPY_FILE_RANGE;
> +               args.out_args[0].size = sizeof(outarg);
> +               args.out_args[0].value = &outarg;
> +               inarg.len = len = min_t(size_t, len, UINT_MAX & PAGE_MASK);
> +       }
>         err = fuse_simple_request(fm, &args);
>         if (err == -ENOSYS) {
> -               fc->no_copy_file_range = 1;
> -               err = -EOPNOTSUPP;
> +               if (fc->no_copy_file_range_64) {

Maybe clearer here to do the if check on the args.opcode? Then this
could just be
if (args.opcode == FUSE_COPY_FILE_RANGE) {

which imo is a lot easier to follow.

> +                       fc->no_copy_file_range = 1;
> +                       err = -EOPNOTSUPP;
> +               } else {
> +                       fc->no_copy_file_range_64 = 1;
> +                       goto fallback;
> +               }
>         }
> -       if (!err && outarg.size > len)
> -               err = -EIO;
> -
>         if (err)
>                 goto out;
>
> +       bytes_copied = fc->no_copy_file_range_64 ?
> +               outarg.size : outarg_64.bytes_copied;
> +
> +       if (bytes_copied > len) {
> +               err = -EIO;
> +               goto out;
> +       }
> +
>         truncate_inode_pages_range(inode_out->i_mapping,
>                                    ALIGN_DOWN(pos_out, PAGE_SIZE),
> -                                  ALIGN(pos_out + outarg.size, PAGE_SIZE) - 1);
> +                                  ALIGN(pos_out + bytes_copied, PAGE_SIZE) - 1);
>
>         file_update_time(file_out);
> -       fuse_write_update_attr(inode_out, pos_out + outarg.size, outarg.size);
> +       fuse_write_update_attr(inode_out, pos_out + bytes_copied, bytes_copied);
>
> -       err = outarg.size;
> +       err = bytes_copied;
>  out:
>         if (is_unstable)
>                 clear_bit(FUSE_I_SIZE_UNSTABLE, &fi_out->state);
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 122d6586e8d4..94621f68a5cc 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -1148,6 +1153,11 @@ struct fuse_copy_file_range_in {
>         uint64_t        flags;
>  };
>
> +/* For FUSE_COPY_FILE_RANGE_64 */
> +struct fuse_copy_file_range_out {

imo having the 64 in the struct name more explicitly makes it clearer
to the server which one they're supposed to use, eg struct
fuse_copy_file_range64_out

Thanks,
Joanne
> +       uint64_t        bytes_copied;
> +};
> +