| Message ID | 20190411162747.4360-1-olga.kornievskaia@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] NFSv4.1 fix incorrect return value in copy_file_range | expand |
On Thu, 2019-04-11 at 12:27 -0400, Olga Kornievskaia wrote: > From: Olga Kornievskaia <kolga@netapp.com> > > 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@netapp.com> > --- > 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. 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?
On Thu, Apr 11, 2019 at 12:46 PM Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Thu, 2019-04-11 at 12:27 -0400, Olga Kornievskaia wrote: > > From: Olga Kornievskaia <kolga@netapp.com> > > > > 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@netapp.com> > > --- > > 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. > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
On Thu, Apr 11, 2019 at 12:52 PM Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote: > > On Thu, Apr 11, 2019 at 12:46 PM Trond Myklebust > <trondmy@hammerspace.com> wrote: > > > > On Thu, 2019-04-11 at 12:27 -0400, Olga Kornievskaia wrote: > > > From: Olga Kornievskaia <kolga@netapp.com> > > > > > > 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@netapp.com> > > > --- > > > 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. Urgh, I just realized a problem in my own check. It needs to be < 2 as there is no COPY in 4.1. > > 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. > > > > > -- > > Trond Myklebust > > Linux NFS client maintainer, Hammerspace > > trond.myklebust@hammerspace.com > > > >
On Thu, 2019-04-11 at 12:52 -0400, Olga Kornievskaia wrote: > On Thu, Apr 11, 2019 at 12:46 PM Trond Myklebust > <trondmy@hammerspace.com> wrote: > > On Thu, 2019-04-11 at 12:27 -0400, Olga Kornievskaia wrote: > > > From: Olga Kornievskaia <kolga@netapp.com> > > > > > > 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@netapp.com> > > > --- > > > 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.
On Thu, Apr 11, 2019 at 1:06 PM Trond Myklebust <trondmy@hammerspace.com> 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@hammerspace.com> wrote: > > > On Thu, 2019-04-11 at 12:27 -0400, Olga Kornievskaia wrote: > > > > From: Olga Kornievskaia <kolga@netapp.com> > > > > > > > > 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@netapp.com> > > > > --- > > > > 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. > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
On Thu, 2019-04-11 at 13:16 -0400, Olga Kornievskaia wrote: > On Thu, Apr 11, 2019 at 1:06 PM Trond Myklebust < > trondmy@hammerspace.com> 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@hammerspace.com> wrote: > > > > On Thu, 2019-04-11 at 12:27 -0400, Olga Kornievskaia wrote: > > > > > From: Olga Kornievskaia <kolga@netapp.com> > > > > > > > > > > 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@netapp.com> > > > > > --- > > > > > 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.
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);