On Thu, 2019-04-11 at 13:16 -0400, Olga Kornievskaia wrote: > On Thu, Apr 11, 2019 at 1:06 PM Trond Myklebust < > trondmy@xxxxxxxxxxxxxxx> wrote: > > On Thu, 2019-04-11 at 12:52 -0400, Olga Kornievskaia wrote: > > > On Thu, Apr 11, 2019 at 12:46 PM Trond Myklebust > > > <trondmy@xxxxxxxxxxxxxxx> wrote: > > > > On Thu, 2019-04-11 at 12:27 -0400, Olga Kornievskaia wrote: > > > > > From: Olga Kornievskaia <kolga@xxxxxxxxxx> > > > > > > > > > > When VFS calls copy_file_range on a 4.0 mount (and when in > > > > > and > > > > > out > > > > > file is the same), we need to return ENOTSUPP instead of > > > > > EINVAL. > > > > > Since no COPY operation is defined in 4.0, then like 3.0, it > > > > > should > > > > > fallback to doing do_splice_direct(). > > > > > > > > > > Otherwise xfstest generic/075,091,112,263 fail. > > > > > > > > > > Signed-off-by: Olga Kornievskaia <kolga@xxxxxxxxxx> > > > > > --- > > > > > fs/nfs/nfs4file.c | 6 ++++++ > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c > > > > > index 45b2322..9a222b0 100644 > > > > > --- a/fs/nfs/nfs4file.c > > > > > +++ b/fs/nfs/nfs4file.c > > > > > @@ -133,6 +133,12 @@ static ssize_t > > > > > nfs4_copy_file_range(struct > > > > > file > > > > > *file_in, loff_t pos_in, > > > > > struct file *file_out, > > > > > loff_t > > > > > pos_out, > > > > > size_t count, unsigned int > > > > > flags) > > > > > { > > > > > + struct nfs_server *server = > > > > > NFS_SERVER(file_inode(file_in); > > > > > + struct nfs_client *client = server->nfs_client; > > > > > + > > > > > + if (client->cl_minorversion < 1) > > > > > + return -EOPNOTSUPP; > > > > > + > > > > > if (file_inode(file_in) == file_inode(file_out)) > > > > > return -EINVAL; > > > > > return nfs42_proc_copy(file_in, pos_in, file_out, > > > > > pos_out, > > > > > count); > > > > > > > > Let's please just move the test for NFS_CAP_COPY from > > > > nfs42_proc_copy() > > > > into the above function. > > > > > > But the return order of errors here for different conditions > > > matter. > > > For 4.1+ mounts, if in == out, we'd expect EINVAL even if server > > > isn't > > > capable. But we can't just put the in == out first because for > > > 4.0, > > > we > > > need to always return EOPNOTSUPP. > > > > > > > BTW: Why do we return -EINVAL in the file_in == file_out case? > > > > Is > > > > there > > > > any reason why we shouldn't just return EOPNOTSUPP there too in > > > > order > > > > to let splice() fulfil the operation? > > > > > > Yes: in == out -> EINVAL is a spec requirement. > > > > > > > Returning NFS4ERR_INVAL when the files are identical is a > > requirement > > for the NFSv4.2 server. > > > > On the other hand, the requirement for the Linux file copy > > operation is > > that it should succeed in this situation. So as far as I can tell, > > we > > will continue to fail xfstests until we work around the discrepancy > > with the protocol requirement here. The simplest way to do so would > > appear to be to return EOPNOTSUPP. > > Ok so you are arguing that those xfstest's for copy_file_range() on > the same file shouldn't fail for any of the NFS version regardless of > the NFSv4.2 req for not doing a copy on the same file. Let me send > out > another version that addresses both then. I will change the "in == > out" to return EOPNOTSUPP which will allow the fallback for 4.2+ and > check for the server capability before that. Right. The point on the client should be to fulfil the requirements of the copy_file_range() spec, so that applications can code to that spec without needing to know any details of how the NFS client tries to implement it. When there is a functionality gap with the NFS spec for COPY (such as is the case here) then we should fall back to emulation of the copy_file_range() spec. ...and again, I'd like to avoid any tests of cl_minorversion in the NFS code. We can and should always replace that with tests for server functionality, which is what the NFS_CAP_* flags are for. This is particularly true now that we're expected to code to the more flexible minor versioning scheme as described in RFC8178. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@xxxxxxxxxxxxxxx