Re: [PATCH] SUNRPC: increase max timeout for rebind to handle NFS server restart

On Apr 17, 2023, at 5:48 PM, Trond Myklebust [off-list ref] wrote:

On Mon, 2023-04-17 at 21:41 +0000, Chuck Lever III wrote:

quoted

quoted

On Apr 17, 2023, at 4:49 PM, Trond Myklebust
[off-list ref] wrote:

On Fri, 2023-04-07 at 20:30 -0700, Dai Ngo wrote:

quoted

Currently call_bind_status places a hard limit of 9 seconds for
retries
on EACCES error. This limit was done to prevent the RPC request
from
being retried forever if the remote server has problem and never
comes
up

However this 9 seconds timeout is too short, comparing to other
RPC
timeouts which are generally in minutes. This causes intermittent
failure
with EIO on the client side when there are lots of NLM activity
and
the
NFS server is restarted.

Instead of removing the max timeout for retry and relying on the
RPC
timeout mechanism to handle the retry, which can lead to the RPC
being
retried forever if the remote NLM service fails to come up. This
patch
simply increases the max timeout of call_bind_status from 9 to 90
seconds
which should allow enough time for NLM to register after a
restart,
and
not retrying forever if there is real problem with the remote
system.

Fixes: 0b760113a3a1 ("NLM: Don't hang forever on NLM unlock
requests")
Reported-by: Helen Chao <redacted>
Tested-by: Helen Chao <redacted>
Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 include/linux/sunrpc/clnt.h  | 3 +++
 include/linux/sunrpc/sched.h | 4 ++--
 net/sunrpc/clnt.c            | 2 +-
 net/sunrpc/sched.c           | 3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/linux/sunrpc/clnt.h

b/include/linux/sunrpc/clnt.h
index 770ef2cb5775..81afc5ea2665 100644

--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h

@@ -162,6 +162,9 @@ struct rpc_add_xprt_test {
 #define RPC_CLNT_CREATE_REUSEPORT      (1UL << 11)
 #define RPC_CLNT_CREATE_CONNECTED      (1UL << 12)
 
+#define        RPC_CLNT_REBIND_DELAY           3
+#define        RPC_CLNT_REBIND_MAX_TIMEOUT     90
+
 struct rpc_clnt *rpc_create(struct rpc_create_args *args);
 struct rpc_clnt        *rpc_bind_new_program(struct rpc_clnt *,
                                const struct rpc_program *, u32);

diff --git a/include/linux/sunrpc/sched.h

b/include/linux/sunrpc/sched.h
index b8ca3ecaf8d7..e9dc142f10bb 100644

--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h

@@ -90,8 +90,8 @@ struct rpc_task {
 #endif
        unsigned char           tk_priority : 2,/* Task priority

*/
                                tk_garb_retry : 2,
-                               tk_cred_retry : 2,
-                               tk_rebind_retry : 2;
+                               tk_cred_retry : 2;
+       unsigned char           tk_rebind_retry;
 };
 
 typedef void                   (*rpc_action)(struct rpc_task *);

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index fd7e1c630493..222578af6b01 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c

@@ -2053,7 +2053,7 @@ call_bind_status(struct rpc_task *task)
                if (task->tk_rebind_retry == 0)
                        break;
                task->tk_rebind_retry--;
-               rpc_delay(task, 3*HZ);
+               rpc_delay(task, RPC_CLNT_REBIND_DELAY * HZ);
                goto retry_timeout;
        case -ENOBUFS:
                rpc_delay(task, HZ >> 2);

diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index be587a308e05..5c18a35752aa 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c

@@ -817,7 +817,8 @@ rpc_init_task_statistics(struct rpc_task

*task)
        /* Initialize retry counters */
        task->tk_garb_retry = 2;
        task->tk_cred_retry = 2;
-       task->tk_rebind_retry = 2;
+       task->tk_rebind_retry = RPC_CLNT_REBIND_MAX_TIMEOUT /
+                                       RPC_CLNT_REBIND_DELAY;

Why not just implement an exponential back off? If the server is
slow
to come up, then pounding the rpcbind service every 3 seconds isn't
going to help.

Exponential backoff has the disadvantage that once we've gotten
to the longer retry times, it's easy for a client to wait quite
some time before it notices the rebind service is available.

But I have to wonder if retrying every 3 seconds is useful if
the request is going over TCP.

The problem isn't reliability: this is handling a case where we _are_
getting a reply from the server, just not one we wanted. EACCES here
means that the rpcbind server didn't return a valid entry for the
service we requested, presumably because the service hasn't been
registered yet.

So yes, an exponential back off is appropriate here.

OK, rpcbind is responding, but the registered service is not
ready yet.

But you've missed my point: exponential backoff is not desirable
if we want clients to recover quickly from a service restart.

If we keep the longest retry time short enough to keep recovery
responsive, it won't be all that much longer than 3 seconds.
Say, it might be 10 seconds, or 15 seconds. I don't see any
value in complicating the client's logic for that. Just make
RPC_CLNT_REBIND_DELAY longer.


--
Chuck Lever