On Wed, Aug 13, 2025 at 10:03:17AM -0700, Joanne Koong wrote:
> On Wed, Aug 13, 2025 at 8:24 AM Miklos Szeredi <mszeredi@xxxxxxxxxx> 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.
I wonder, does fuse_args::out_argvar==1 imply that you could create a
new 64-bit fuse_write64_out:
struct fuse_write64_out {
uint64_t size;
uint64_t padding;
};
and then fuse_copy_file_range declares a union:
union fuse_cfr_out {
struct fuse_write_out out;
struct fuse_write64_out out64;
};
passes that into fuse_args:
union fuse_cfr_out outarg;
args.out_argvar = 1;
args.out_numargs = 1;
args.out_args[0].size = sizeof(outarg);
args.out_args[0].value = &outarg;
and then we can switch on the results:
if (args.out_args[0].size == sizeof(fuse_write64_out))
/* 64-bit return */
else if (args.out_args[0].size == sizeof(fuse_write_out))
/* 32-bit return */
else
/* error */
I guess the problem is that userspace has to know that the kernel will
accept a fuse_write64_out, because on an old kernel it'll get -EINVAL
and ... then what? I think an error return ends the request and the
fuse server can't just try again with fuse_write_out.
<shrug> Maybe I'm speculating stupi^Wwildly. ;)
--D
> >
> > Reported-by: Florian Weimer <fweimer@xxxxxxxxxx>
> > Closes: https://lore.kernel.org/all/lhuh5ynl8z5.fsf@xxxxxxxxxxxxxxxxxxxxxxxx/
> > Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> > ---
> > 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;
> > +};
> > +
>
