Re: [PATCH 1/2] fuse: fix COPY_FILE_RANGE interface

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Aug 06, 2025 at 08:48:41PM +0100, Luis Henriques wrote:
> On Wed, Aug 06 2025, Darrick J. Wong wrote:
> 
> > On Wed, Aug 06, 2025 at 10:17:06AM +0100, Luis Henriques wrote:
> >> On Tue, Aug 05 2025, Miklos Szeredi 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.
> >> >
> >> > Currently the number of bytes copied is silently truncated to 32-bit, which
> >> > is unfortunate at best.
> >> >
> >> > 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 and truncate the size to UINT_MAX - 4096.
> >> 
> >> I was wondering if it wouldn't make more sense to truncate the size to
> >> MAX_RW_COUNT instead.  My reasoning is that, if I understand the code
> >> correctly (which is probably a big 'if'!), the VFS will fallback to
> >> splice() if the file system does not implement copy_file_range.  And in
> >> this case splice() seems to limit the operation to MAX_RW_COUNT.
> >
> > It doesn't, because copy_file_range implementations can do other things
> > (like remapping/reflinking file blocks) that produce a very small amount
> > of disk IO for what is effectively a very large change to file contents.
> > That's why the VFS doesn't cap len at MAX_RW_COUNT bytes.
> 
> Oh, OK.  So looks like I misunderstood that code.  In vfs_copy_file_range(),
> I assumed that the fallback to splice ('splice = true;') would cap the IO
> with the following:
> 
> 	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
> 			       min_t(size_t, len, MAX_RW_COUNT), 0);
> 
> And that's why I suggested to do the same here instead of UINT_MAX - 4096.

(/me stumbles back in after FOSSY)

Yeah -- splice actually /does/ dirty pages, because it's actually doing
a buffer copy in a (hopefully) more efficiently than havng the userspace
process do malloc/read/write and endure syscall overhead.  That's why
it's ok to limit a splice to MAX_RW_COUNT, because it's basically a
write().

--D

> Cheers,
> -- 
> Luís
> 
> 
> > --D
> >
> >> Cheers,
> >> -- 
> >> Luís
> >> 
> >> 
> >> > 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            | 34 ++++++++++++++++++++++++++--------
> >> >  fs/fuse/fuse_i.h          |  3 +++
> >> >  include/uapi/linux/fuse.h | 12 +++++++++++-
> >> >  3 files changed, 40 insertions(+), 9 deletions(-)
> >> >
> >> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> >> > index adc4aa6810f5..bd6624885855 100644
> >> > --- a/fs/fuse/file.c
> >> > +++ b/fs/fuse/file.c
> >> > @@ -3017,6 +3017,8 @@ static ssize_t __fuse_copy_file_range(struct file *file_in, loff_t pos_in,
> >> >  		.flags = flags
> >> >  	};
> >> >  	struct fuse_write_out outarg;
> >> > +	struct fuse_copy_file_range_out outarg_64;
> >> > +	u64 bytes_copied;
> >> >  	ssize_t err;
> >> >  	/* mark unstable when write-back is not used, and file_out gets
> >> >  	 * extended */
> >> > @@ -3066,30 +3068,46 @@ 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 = min_t(size_t, len, 0xfffff000);
> >> > +	}
> >> >  	err = fuse_simple_request(fm, &args);
> >> >  	if (err == -ENOSYS) {
> >> > -		fc->no_copy_file_range = 1;
> >> > -		err = -EOPNOTSUPP;
> >> > +		if (fc->no_copy_file_range_64) {
> >> > +			fc->no_copy_file_range = 1;
> >> > +			err = -EOPNOTSUPP;
> >> > +		} else {
> >> > +			fc->no_copy_file_range_64 = 1;
> >> > +			goto fallback;
> >> > +		}
> >> >  	}
> >> >  	if (err)
> >> >  		goto out;
> >> >  
> >> > +	bytes_copied = fc->no_copy_file_range_64 ?
> >> > +		outarg.size : outarg_64.bytes_copied;
> >> > +
> >> >  	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/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> >> > index b54f4f57789f..a8be19f686b1 100644
> >> > --- a/fs/fuse/fuse_i.h
> >> > +++ b/fs/fuse/fuse_i.h
> >> > @@ -850,6 +850,9 @@ struct fuse_conn {
> >> >  	/** Does the filesystem support copy_file_range? */
> >> >  	unsigned no_copy_file_range:1;
> >> >  
> >> > +	/** Does the filesystem support copy_file_range_64? */
> >> > +	unsigned no_copy_file_range_64:1;
> >> > +
> >> >  	/* Send DESTROY request */
> >> >  	unsigned int destroy:1;
> >> >  
> >> > 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
> >> > @@ -235,6 +235,10 @@
> >> >   *
> >> >   *  7.44
> >> >   *  - add FUSE_NOTIFY_INC_EPOCH
> >> > + *
> >> > + *  7.45
> >> > + *  - add FUSE_COPY_FILE_RANGE_64
> >> > + *  - add struct fuse_copy_file_range_out
> >> >   */
> >> >  
> >> >  #ifndef _LINUX_FUSE_H
> >> > @@ -270,7 +274,7 @@
> >> >  #define FUSE_KERNEL_VERSION 7
> >> >  
> >> >  /** Minor version number of this interface */
> >> > -#define FUSE_KERNEL_MINOR_VERSION 44
> >> > +#define FUSE_KERNEL_MINOR_VERSION 45
> >> >  
> >> >  /** The node ID of the root inode */
> >> >  #define FUSE_ROOT_ID 1
> >> > @@ -657,6 +661,7 @@ enum fuse_opcode {
> >> >  	FUSE_SYNCFS		= 50,
> >> >  	FUSE_TMPFILE		= 51,
> >> >  	FUSE_STATX		= 52,
> >> > +	FUSE_COPY_FILE_RANGE_64	= 53,
> >> >  
> >> >  	/* CUSE specific operations */
> >> >  	CUSE_INIT		= 4096,
> >> > @@ -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 {
> >> > +	uint64_t	bytes_copied;
> >> > +};
> >> > +
> >> >  #define FUSE_SETUPMAPPING_FLAG_WRITE (1ull << 0)
> >> >  #define FUSE_SETUPMAPPING_FLAG_READ (1ull << 1)
> >> >  struct fuse_setupmapping_in {
> >> > -- 
> >> > 2.49.0
> >> >
> >> 
> >> 
> 
>