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

[bfields@fieldses.org (J. Bruce Fields)]

On Mon, Jan 21, 2019 at 03:58:38PM -0500, Trond Myklebust wrote:
> If the parameter 'count' is non-zero, nfsd4_clone_file_range() will
> currently clobber all errors returned by vfs_clone_file_range() and
> replace them with EINVAL.

Oops, thanks for the fix.  I'm still a little confused, though:

> 
> Fixes: 42ec3d4c0218 ("vfs: make remap_file_range functions take and...")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> Cc: stable@vger.kernel.org # v4.20+
> ---
>  fs/nfsd/vfs.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 9824e32b2f23..7dc98e14655d 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -557,9 +557,11 @@ __be32 nfsd4_clone_file_range(struct file *src, u64 src_pos, struct file *dst,
>  	loff_t cloned;
>  
>  	cloned = vfs_clone_file_range(src, src_pos, dst, dst_pos, count, 0);
> +	if (cloned < 0)
> +		return nfserrno(cloned);
>  	if (count && cloned != count)
> -		cloned = -EINVAL;
> -	return nfserrno(cloned < 0 ? cloned : 0);
> +		return nfserrno(-EINVAL);
> +	return 0;

I still don't understand the cloned != count case.  I thought clone was
supposed to be all-or-nothing and atomic, can it really return a short
copy?  And how is that inval, shouldn't that be serverfault?

--b.

>  }
>  
>  ssize_t nfsd_copy_file_range(struct file *src, u64 src_pos, struct file *dst,
> -- 
> 2.20.1