Re: [PATCH] nfsd: Fix error return values for nfsd4_clone_file_range()

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Jan 25, 2019 at 03:15:51PM -0500, J. Bruce Fields wrote:
> On Fri, Jan 25, 2019 at 03:10:37PM -0500, J. Bruce Fields wrote:
> > Yeah.  I was assuming it could happen in the case you ask to clone
> > beyond the end of the source file.  But looking at the code, there's a
> > check for that case in generic_remap_checks() before doing the clone,
> > and while holding a write lock on i_rwsem (I assume that's enough to
> > hold the file size constant).  At least that's true in the cases (btrfs
> > & xfs) that I checked.
> > 
> > So, I don't know, maybe that check is just dead code.
> 
> In the xfs case it looks like the main work of the clone is done in
> xfs_reflink_remap_blocks(), where there's a loop like:
> 
> 	while (len) {
> 		... mysterious code that clones range_len worth of
> 		    extents?
> 		if (fatal_signal_pending(current)) {
> 			error =-EINTR;
> 			break;
> 		}
> 		...
> 		len -= range_len;
> 		remapped_len += range_len;
> 	}
> 
> And then it ends up returning remapped_len if it's positive.

Hmm?  In xfs_reflink_remap_blocks, *remapped (an out parameter) is set
to remapped_len just prior to returning whatever error is.  The caller
(xfs_file_remap_range) can see how many bytes were remapped, as well as
any error that might have cut short the remapping process...

> So it looks to me like if you do a big clone on xfs and kill the
> process, it can clone part of the range, return the amount cloned, and
> then the ioctl code will throw away that amount and just return EINVAL,

...and so xfs_file_remap_range will return the number of bytes remapped
before it returns any error codes.  This was done so that
copy_file_range can call remap_file_range and report a short but
otherwise successful copy.

Yes, it's sort of dumb that we pass the "bytes remapped" information all
the way up the call stack only to have the clonerange ioctl spit back
EINVAL on a short remap, but we're stuck with that (poorly thought out)
artifact of btrfs.

Note that XFS can return cloned < count if (for example) it runs out of
space trying to expand the extent map btree to add more extents.

> with the result that the application thinks the operation failed
> completely actually it cloned a bunch of data.

(Yes, the ioctl is dumb; I would say that programs should use
copy_file_range instead as a less bad interface, but the splice copy
fallback is ...  yuck.)

--D

> --b.