[PATCH] SUNRPC: have soft RPC tasks return -ETIMEDOUT instead of -EIO on major connect timeout

[PATCH] SUNRPC: have soft RPC tasks return -ETIMEDOUT instead of -EIO on major connect timeout

[Jeff Layton]

NFSv4 background mounts do not currently work correctly. While we could
try to fix this in userspace, I think it's really a kernel problem...

When a soft RPC tasks experiences a major timeout during a connection
attempt, it does an rpc_exit with a return code of -EIO. For NFSv4
mounts, this makes the mount() syscall return -EIO. mount.nfs4 then
interprets that as a "permanent" error, and won't attempt a background
mount when bg is specified. Fix this by making call_timeout() do the
rpc_exit() with an error of -ETIMEDOUT.

This fixes the background mount issue, but does make other syscalls
on soft mounts return ETIMEDOUT instead of EIO in this situation.

Comments welcome.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/clnt.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 8c6a7f1..b6d409e 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1162,7 +1162,7 @@ call_timeout(struct rpc_task *task)
 	if (RPC_IS_SOFT(task)) {
 		printk(KERN_NOTICE "%s: server %s not responding, timed out\n",
 				clnt->cl_protname, clnt->cl_server);
-		rpc_exit(task, -EIO);
+		rpc_exit(task, -ETIMEDOUT);
 		return;
 	}
 
-- 
1.5.3.6

Re: [PATCH] SUNRPC: have soft RPC tasks return -ETIMEDOUT instead of -EIO on major connect timeout

[Trond Myklebust]

On Sat, 2008-03-29 at 08:49 -0400, Jeff Layton wrote:
> NFSv4 background mounts do not currently work correctly. While we could
> try to fix this in userspace, I think it's really a kernel problem...
> 
> When a soft RPC tasks experiences a major timeout during a connection
> attempt, it does an rpc_exit with a return code of -EIO. For NFSv4
> mounts, this makes the mount() syscall return -EIO. mount.nfs4 then
> interprets that as a "permanent" error, and won't attempt a background
> mount when bg is specified. Fix this by making call_timeout() do the
> rpc_exit() with an error of -ETIMEDOUT.
> 
> This fixes the background mount issue, but does make other syscalls
> on soft mounts return ETIMEDOUT instead of EIO in this situation.
> 
> Comments welcome.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  net/sunrpc/clnt.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 8c6a7f1..b6d409e 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -1162,7 +1162,7 @@ call_timeout(struct rpc_task *task)
>  	if (RPC_IS_SOFT(task)) {
>  		printk(KERN_NOTICE "%s: server %s not responding, timed out\n",
>  				clnt->cl_protname, clnt->cl_server);
> -		rpc_exit(task, -EIO);
> +		rpc_exit(task, -ETIMEDOUT);
>  		return;
>  	}

While that may be acceptable for the mount() syscall, I don't think
POSIX applications are quite ready to deal with ETIMEDOUT as an error
for stat() or chdir().

Userland has the clnt_geterr() function that returns more detailed 'RPC
level' errors. While that 'error function call' approach doesn't work in
a multi-threaded environment, we might still be able to add the
equivalent of a pointer to an 'rpc_err' structure to the rpc_task, and
then have functions like call_timeout() (and especially call_verify()!)
fill in more detailed error info if that pointer is non-zero?

Cheers
  Trond

Re: [PATCH] SUNRPC: have soft RPC tasks return -ETIMEDOUT instead of -EIO on major connect timeout

[Jeff Layton]

On Sat, 29 Mar 2008 12:44:11 -0400
Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> 
> On Sat, 2008-03-29 at 08:49 -0400, Jeff Layton wrote:
> > NFSv4 background mounts do not currently work correctly. While we could
> > try to fix this in userspace, I think it's really a kernel problem...
> > 
> > When a soft RPC tasks experiences a major timeout during a connection
> > attempt, it does an rpc_exit with a return code of -EIO. For NFSv4
> > mounts, this makes the mount() syscall return -EIO. mount.nfs4 then
> > interprets that as a "permanent" error, and won't attempt a background
> > mount when bg is specified. Fix this by making call_timeout() do the
> > rpc_exit() with an error of -ETIMEDOUT.
> > 
> > This fixes the background mount issue, but does make other syscalls
> > on soft mounts return ETIMEDOUT instead of EIO in this situation.
> > 
> > Comments welcome.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  net/sunrpc/clnt.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > index 8c6a7f1..b6d409e 100644
> > --- a/net/sunrpc/clnt.c
> > +++ b/net/sunrpc/clnt.c
> > @@ -1162,7 +1162,7 @@ call_timeout(struct rpc_task *task)
> >  	if (RPC_IS_SOFT(task)) {
> >  		printk(KERN_NOTICE "%s: server %s not responding, timed out\n",
> >  				clnt->cl_protname, clnt->cl_server);
> > -		rpc_exit(task, -EIO);
> > +		rpc_exit(task, -ETIMEDOUT);
> >  		return;
> >  	}
> 
> While that may be acceptable for the mount() syscall, I don't think
> POSIX applications are quite ready to deal with ETIMEDOUT as an error
> for stat() or chdir().
> 

Ugh. Good point.

> Userland has the clnt_geterr() function that returns more detailed 'RPC
> level' errors. While that 'error function call' approach doesn't work in
> a multi-threaded environment, we might still be able to add the
> equivalent of a pointer to an 'rpc_err' structure to the rpc_task, and
> then have functions like call_timeout() (and especially call_verify()!)
> fill in more detailed error info if that pointer is non-zero?
>

I'm not sure we really need this, do we?

Should it really be the business of the RPC layer to sanitize the
tk_status like this? It seems like the NFS layer ought to be
translating "illegal" errors from the RPC layer into more generic ones
where needed rather than relying on the RPC layer to do it, though
maybe I'm not thinking about the RPC layer in the right way here...

Thanks,
--
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] SUNRPC: have soft RPC tasks return -ETIMEDOUT instead of -EIO on major connect timeout

[Trond Myklebust]

On Sat, 2008-03-29 at 15:24 -0400, Jeff Layton wrote:
> On Sat, 29 Mar 2008 12:44:11 -0400
> Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> > Userland has the clnt_geterr() function that returns more detailed 'RPC
> > level' errors. While that 'error function call' approach doesn't work in
> > a multi-threaded environment, we might still be able to add the
> > equivalent of a pointer to an 'rpc_err' structure to the rpc_task, and
> > then have functions like call_timeout() (and especially call_verify()!)
> > fill in more detailed error info if that pointer is non-zero?
> >
> 
> I'm not sure we really need this, do we?
> 
> Should it really be the business of the RPC layer to sanitize the
> tk_status like this? It seems like the NFS layer ought to be
> translating "illegal" errors from the RPC layer into more generic ones
> where needed rather than relying on the RPC layer to do it, though
> maybe I'm not thinking about the RPC layer in the right way here...

Yes and no. The RPC error reports are sometimes more complex than what
we can fit into a single 32-bit error code. I'm thinking in particular
of the RPC_AUTH_* errors (EACCESS just doesn't do them justice), and
RPC_PROG_MISMATCH error.

In the case of RPC_PROG_MISMATCH, it would, for instance, be really
useful for the in-kernel mount code to be able to also retrieve the
'mismatch_info' structure (see RFC1831), which tells you exactly which
program versions are actually supported on that port.

An extra optional pointer to a structure in the rpc_task won't cost us
much, but could possibly provide a lot of interesting information that
we are currently ignoring.

Cheers
  Trond

Re: [PATCH] SUNRPC: have soft RPC tasks return -ETIMEDOUT instead of -EIO on major connect timeout

[Jeff Layton]

On Sat, 29 Mar 2008 16:05:49 -0400
Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> 
> On Sat, 2008-03-29 at 15:24 -0400, Jeff Layton wrote:
> > On Sat, 29 Mar 2008 12:44:11 -0400
> > Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> > > Userland has the clnt_geterr() function that returns more detailed 'RPC
> > > level' errors. While that 'error function call' approach doesn't work in
> > > a multi-threaded environment, we might still be able to add the
> > > equivalent of a pointer to an 'rpc_err' structure to the rpc_task, and
> > > then have functions like call_timeout() (and especially call_verify()!)
> > > fill in more detailed error info if that pointer is non-zero?
> > >
> > 
> > I'm not sure we really need this, do we?
> > 
> > Should it really be the business of the RPC layer to sanitize the
> > tk_status like this? It seems like the NFS layer ought to be
> > translating "illegal" errors from the RPC layer into more generic ones
> > where needed rather than relying on the RPC layer to do it, though
> > maybe I'm not thinking about the RPC layer in the right way here...
> 
> Yes and no. The RPC error reports are sometimes more complex than what
> we can fit into a single 32-bit error code. I'm thinking in particular
> of the RPC_AUTH_* errors (EACCESS just doesn't do them justice), and
> RPC_PROG_MISMATCH error.
> 
> In the case of RPC_PROG_MISMATCH, it would, for instance, be really
> useful for the in-kernel mount code to be able to also retrieve the
> 'mismatch_info' structure (see RFC1831), which tells you exactly which
> program versions are actually supported on that port.
> 
> An extra optional pointer to a structure in the rpc_task won't cost us
> much, but could possibly provide a lot of interesting information that
> we are currently ignoring.
> 

Ok, that makes sense. Yes, adding a new pointer to the rpc_task doesn't
sound too costly. The hard part is getting this pointer from the higher
layers that would interpret this info to the correct RPC functions that
could populate it.

I don't really have a good feel for how we'd populate this new pointer
as well. For instance, you'd want to hang a mismatch_info struct on it 
for the case of RPC_PROG_MISMATCH, but in the case I was trying to fix
above, we'd just want a simple 32-bit error code. How will the RPC
engine know what this pointer points to? Or should we just make it a
new struct that has fields for each possibility?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] SUNRPC: have soft RPC tasks return -ETIMEDOUT instead of -EIO on major connect timeout

[Chuck Lever]

On Mar 29, 2008, at 12:44 PM, Trond Myklebust wrote:
> On Sat, 2008-03-29 at 08:49 -0400, Jeff Layton wrote:
>> NFSv4 background mounts do not currently work correctly. While we  
>> could
>> try to fix this in userspace, I think it's really a kernel problem...
>>
>> When a soft RPC tasks experiences a major timeout during a connection
>> attempt, it does an rpc_exit with a return code of -EIO. For NFSv4
>> mounts, this makes the mount() syscall return -EIO. mount.nfs4 then
>> interprets that as a "permanent" error, and won't attempt a  
>> background
>> mount when bg is specified. Fix this by making call_timeout() do the
>> rpc_exit() with an error of -ETIMEDOUT.
>>
>> This fixes the background mount issue, but does make other syscalls
>> on soft mounts return ETIMEDOUT instead of EIO in this situation.
>>
>> Comments welcome.
>>
>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>> ---
>>  net/sunrpc/clnt.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index 8c6a7f1..b6d409e 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -1162,7 +1162,7 @@ call_timeout(struct rpc_task *task)
>>  	if (RPC_IS_SOFT(task)) {
>>  		printk(KERN_NOTICE "%s: server %s not responding, timed out\n",
>>  				clnt->cl_protname, clnt->cl_server);
>> -		rpc_exit(task, -EIO);
>> +		rpc_exit(task, -ETIMEDOUT);
>>  		return;
>>  	}
>
> While that may be acceptable for the mount() syscall, I don't think
> POSIX applications are quite ready to deal with ETIMEDOUT as an error
> for stat() or chdir().

Having the RPC client throw -EIO on a timeout always seemed a little  
crude to me.  EIO is quite overloaded -- the same error is returned  
if there's a XDR decoding error, for example.  Clearly other  
consumers of RPC (mount, for example) would like a distinction  
between a timeout and an outright I/O error.

The fact that applications using NFS files can't deal with -ETIMEDOUT  
should probably be managed in the NFS client, not in the RPC client.   
Perhaps it could be handled with a wrapper function, like the NFS  
client handles EJUKEBOX.

So I agree that Jeff's patch is insufficient as it stands, but the  
underlying idea is probably a good one.

> Userland has the clnt_geterr() function that returns more detailed  
> 'RPC
> level' errors. While that 'error function call' approach doesn't  
> work in
> a multi-threaded environment, we might still be able to add the
> equivalent of a pointer to an 'rpc_err' structure to the rpc_task, and
> then have functions like call_timeout() (and especially call_verify 
> ()!)
> fill in more detailed error info if that pointer is non-zero?


That's not a bad idea either.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com