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

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

On Tue, Aug 12, 2025 at 05:08:18PM +0800, Chunsheng Luo wrote:
> On 8/6/25 19:43, Bernd Schubert wrote: 
> 
> > On 8/6/25 11:17, 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.
> >
> > I personally don't like the hard coded value too much and would use
> > 
> > inarg.len = min_t(size_t, len, (UINT_MAX - 4096));
> > 
> > (btw, 0xfffff000 is UINT_MAX - 4095, isn't it?).
> > 
> > Though that is all nitpick. The worst part that could happen are
> > applications that do not handle partial file copy and then wouldn't
> > copy the entire file. For these it probably would be better to return
> > -ENOSYS.
> > 
> > LGTM, 
> > 
> > Reviewed-by: Bernd Schubert <bschubert@ddn.com>
> 
> Abot "truncate the size to UINT_MAX - 4096":
> 4096 refers to PAGE_SIZE (the standard memory page size in most systems)?
> If so, wouldn't UINT_MAX & PAGE_MASK be more appropriate? 
> like: `#define MAX_RW_COUNT (INT_MAX & PAGE_MASK)`
> UINT_MAX & PAGE_MASK ensures the operation does not cross a page boundary.

Yeah, I was wondering that too -- if we're going to cap a
copy_file_range to something resembling MAX_RW_COUNT, then why not use
that symbol directly? :)

--D

> Thanks
> Chunsheng Luo
>