[1/1] NFSv4.1 fix incorrect return value in copy_file_range
diff mbox series

Message ID 20190411162747.4360-1-olga.kornievskaia@gmail.com
State New
Headers show
Series
  • [1/1] NFSv4.1 fix incorrect return value in copy_file_range
Related show

Commit Message

Olga Kornievskaia April 11, 2019, 4:27 p.m. UTC
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(+)

Comments

Trond Myklebust April 11, 2019, 4:46 p.m. UTC | #1
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?
Olga Kornievskaia April 11, 2019, 4:52 p.m. UTC | #2
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
>
>
Olga Kornievskaia April 11, 2019, 4:53 p.m. UTC | #3
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
> >
> >
Trond Myklebust April 11, 2019, 5:06 p.m. UTC | #4
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.
Olga Kornievskaia April 11, 2019, 5:16 p.m. UTC | #5
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
>
>
Trond Myklebust April 11, 2019, 5:32 p.m. UTC | #6
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.

Patch
diff mbox series

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);