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

J. Bruce Fields
2019-01-25T00:46:59Z

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