On Fri, Jan 25, 2019 at 11:42:17AM -0500, Olga Kornievskaia wrote: > On Fri, Jan 25, 2019 at 11:32 AM J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote: > > > > On Fri, Jan 25, 2019 at 12:50:09AM -0500, Trond Myklebust wrote: > > > On Thu, 2019-01-24 at 19:46 -0500, J. Bruce Fields wrote: > > > > 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: > > ... > > > > > 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? > > > > > > That, quite frankly, seems like more of a question for Darrick, not me. > > > I haven't changed that part of the code. > > > > > > The main thing I care about is being able to correctly report > > > EOPNOTSUPP errors for the vast majority of filesystems that don't > > > support clone() or dedup(). > > > > Makes sense, and I'm happy just to apply this and then sort out the rest in a > > subsequent patch, but I'd really like to understand; Darrick?: > > > > ioctl_file_clone also converts short copies to EINVAL: > > > > if (cloned < 0) > > ret = cloned; > > else if (olen && cloned != olen) > > ret = -EINVAL; > > else > > ret = 0; > > > > Maybe that happens iff we hit EOF in the short file? > > > > Does that mean we can successfully copy up to EOF and then return -EINVAL? > > That sounds wrong. > > > > There's a man page (IOCTL-FICLONERANGE(2)) but it doesn't cover this case. > > I thought cloned by definition was all or nothing meaning there can't > be a "short" clone. If you allow for less then asked bytes to be > returned, then your next offsets might not be block aligned. 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. --b.
