| Message ID | 20230608144933.412664-1-tigran.mkrtchyan@desy.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfs4: don't map EACCESS and EPERM to EIO | expand |
Hi Tigran, On Thu, 2023-06-08 at 16:49 +0200, Tigran Mkrtchyan wrote: > the nfs4_map_errors function converts NFS specific errors to userland > errors. However, it ignores NFS4ERR_PERM and EPERM, which then get > mapped to EIO. > > Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de> > --- > fs/nfs/nfs4proc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index d3665390c4cb..795205fe4f30 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -171,12 +171,14 @@ static int nfs4_map_errors(int err) > case -NFS4ERR_LAYOUTTRYLATER: > case -NFS4ERR_RECALLCONFLICT: > return -EREMOTEIO; > + case -NFS4ERR_PERM: > case -NFS4ERR_WRONGSEC: > case -NFS4ERR_WRONG_CRED: > return -EPERM; > case -NFS4ERR_BADOWNER: > case -NFS4ERR_BADNAME: > return -EINVAL; > + case -NFS4ERR_ACCESS: > case -NFS4ERR_SHARE_DENIED: > return -EACCES; > case -NFS4ERR_MINOR_VERS_MISMATCH: Hmm... Aren't both these cases covered by the exception at the top of the function? static int nfs4_map_errors(int err) { if (err >= -1000) return err; As I read it, that should mean that err = -NFS4ERR_ACCESS (= -13) and err = -NFS4ERR_PERM (= -1) will get returned verbatim. Are you seeing these NFS4ERR_ACCESS and NFS4ERR_PERM cases hitting the default: dprintk() when you turn it on?
On Thu, 2023-06-08 at 19:42 +0200, Tigran Mkrtchyan wrote: > Hi Trond, > > I will check and let you know. What we see is EACCESS on layoutget > reported as EIO to the applications > If this is for a write, then that might just be nfs_mapping_set_error(). In newer kernels, it tries to avoid sending errors that are unexpected for strictly POSIX applications. Cheers Trond > Best regards, > Tigran > > > On June 8, 2023 5:33:16 PM GMT+02:00, Trond Myklebust > <trondmy@hammerspace.com> wrote: > > Hi Tigran, > > > > On Thu, 2023-06-08 at 16:49 +0200, Tigran Mkrtchyan wrote: > > > the nfs4_map_errors function converts NFS specific errors to > > > userland > > > errors. However, it ignores NFS4ERR_PERM and EPERM, which then > > > get > > > mapped to EIO. > > > > > > Signed-off-by: Tigran Mkrtchyan > > > <tigran.mkrtchyan@desy.de> fs/nfs/nfs4proc.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > index d3665390c4cb..795205fe4f30 100644 > > > --- a/fs/nfs/nfs4proc.c > > > +++ b/fs/nfs/nfs4proc.c > > > @@ -171,12 +171,14 @@ static int nfs4_map_errors(int err) > > > case -NFS4ERR_LAYOUTTRYLATER: > > > case -NFS4ERR_RECALLCONFLICT: > > > return -EREMOTEIO; > > > + case -NFS4ERR_PERM: > > > case -NFS4ERR_WRONGSEC: > > > case -NFS4ERR_WRONG_CRED: > > > return -EPERM; > > > case -NFS4ERR_BADOWNER: > > > case -NFS4ERR_BADNAME: > > > return -EINVAL; > > > + case -NFS4ERR_ACCESS: > > > case -NFS4ERR_SHARE_DENIED: > > > return -EACCES; > > > case -NFS4ERR_MINOR_VERS_MISMATCH: > > > > > > > Hmm... Aren't both these cases covered by the exception at the top > > of > > the function? > > > > static int nfs4_map_errors(int err) > > { > > if (err >= -1000) > > return err; > > > > As I read it, that should mean that err = -NFS4ERR_ACCESS (= -13) > > and > > err = -NFS4ERR_PERM (= -1) will get returned verbatim. > > > > Are you seeing these NFS4ERR_ACCESS and NFS4ERR_PERM cases hitting > > the > > default: dprintk() when you turn it on? > >
Hi Trond, Obviously, the patch is incorrect. The behavior of the upstream kernel and RHEL kernels are different. Sorry for the noise, Tigran. ----- Original Message ----- > From: "Trond Myklebust" <trondmy@hammerspace.com> > To: anna@kernel.org, "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de> > Cc: "linux-nfs" <linux-nfs@vger.kernel.org> > Sent: Thursday, 8 June, 2023 19:53:07 > Subject: Re: [PATCH] nfs4: don't map EACCESS and EPERM to EIO > On Thu, 2023-06-08 at 19:42 +0200, Tigran Mkrtchyan wrote: >> Hi Trond, >> >> I will check and let you know. What we see is EACCESS on layoutget >> reported as EIO to the applications >> > > If this is for a write, then that might just be > nfs_mapping_set_error(). In newer kernels, it tries to avoid sending > errors that are unexpected for strictly POSIX applications. > > Cheers > Trond > >> Best regards, >> Tigran >> >> >> On June 8, 2023 5:33:16 PM GMT+02:00, Trond Myklebust >> <trondmy@hammerspace.com> wrote: >> > Hi Tigran, >> > >> > On Thu, 2023-06-08 at 16:49 +0200, Tigran Mkrtchyan wrote: >> > > the nfs4_map_errors function converts NFS specific errors to >> > > userland >> > > errors. However, it ignores NFS4ERR_PERM and EPERM, which then >> > > get >> > > mapped to EIO. >> > > >> > > Signed-off-by: Tigran Mkrtchyan >> > > <tigran.mkrtchyan@desy.de> fs/nfs/nfs4proc.c | 2 ++ >> > > 1 file changed, 2 insertions(+) >> > > >> > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> > > index d3665390c4cb..795205fe4f30 100644 >> > > --- a/fs/nfs/nfs4proc.c >> > > +++ b/fs/nfs/nfs4proc.c >> > > @@ -171,12 +171,14 @@ static int nfs4_map_errors(int err) >> > > case -NFS4ERR_LAYOUTTRYLATER: >> > > case -NFS4ERR_RECALLCONFLICT: >> > > return -EREMOTEIO; >> > > + case -NFS4ERR_PERM: >> > > case -NFS4ERR_WRONGSEC: >> > > case -NFS4ERR_WRONG_CRED: >> > > return -EPERM; >> > > case -NFS4ERR_BADOWNER: >> > > case -NFS4ERR_BADNAME: >> > > return -EINVAL; >> > > + case -NFS4ERR_ACCESS: >> > > case -NFS4ERR_SHARE_DENIED: >> > > return -EACCES; >> > > case -NFS4ERR_MINOR_VERS_MISMATCH: >> > > >> > >> > Hmm... Aren't both these cases covered by the exception at the top >> > of >> > the function? >> > >> > static int nfs4_map_errors(int err) >> > { >> > if (err >= -1000) >> > return err; >> > >> > As I read it, that should mean that err = -NFS4ERR_ACCESS (= -13) >> > and >> > err = -NFS4ERR_PERM (= -1) will get returned verbatim. >> > >> > Are you seeing these NFS4ERR_ACCESS and NFS4ERR_PERM cases hitting >> > the >> > default: dprintk() when you turn it on? >> > > > -- > Trond Myklebust > CTO, Hammerspace Inc > 1900 S Norfolk St, Suite 350 - #45 > San Mateo, CA 94403 > > www.hammerspace.com
On 9 Jun 2023, at 9:30, Mkrtchyan, Tigran wrote: > Hi Trond, > > Obviously, the patch is incorrect. The behavior of the upstream kernel and > RHEL kernels are different. RHEL-9 should be ok here. There's a few things we need to be fixing for RHEL-8.9. This is one of them. https://bugzilla.redhat.com/show_bug.cgi?id=2213828 Ben
Thanks, Ben! Tigran. ----- Original Message ----- > From: "Benjamin Coddington" <bcodding@redhat.com> > To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de> > Cc: "Trond Myklebust" <trondmy@hammerspace.com>, "anna" <anna@kernel.org>, "linux-nfs" <linux-nfs@vger.kernel.org> > Sent: Friday, 9 June, 2023 16:00:34 > Subject: Re: [PATCH] nfs4: don't map EACCESS and EPERM to EIO > On 9 Jun 2023, at 9:30, Mkrtchyan, Tigran wrote: > >> Hi Trond, >> >> Obviously, the patch is incorrect. The behavior of the upstream kernel and >> RHEL kernels are different. > > RHEL-9 should be ok here. > > There's a few things we need to be fixing for RHEL-8.9. This is one of them. > https://bugzilla.redhat.com/show_bug.cgi?id=2213828 > > Ben
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index d3665390c4cb..795205fe4f30 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -171,12 +171,14 @@ static int nfs4_map_errors(int err) case -NFS4ERR_LAYOUTTRYLATER: case -NFS4ERR_RECALLCONFLICT: return -EREMOTEIO; + case -NFS4ERR_PERM: case -NFS4ERR_WRONGSEC: case -NFS4ERR_WRONG_CRED: return -EPERM; case -NFS4ERR_BADOWNER: case -NFS4ERR_BADNAME: return -EINVAL; + case -NFS4ERR_ACCESS: case -NFS4ERR_SHARE_DENIED: return -EACCES; case -NFS4ERR_MINOR_VERS_MISMATCH:
the nfs4_map_errors function converts NFS specific errors to userland errors. However, it ignores NFS4ERR_PERM and EPERM, which then get mapped to EIO. Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de> --- fs/nfs/nfs4proc.c | 2 ++ 1 file changed, 2 insertions(+)