> On Apr 17, 2023, at 5:48 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, 2023-04-17 at 21:41 +0000, Chuck Lever III wrote:
>>
>>
>>> On Apr 17, 2023, at 4:49 PM, Trond Myklebust
>>> <trondmy@xxxxxxxxxxxxxxx> wrote:
>>>
>>> On Fri, 2023-04-07 at 20:30 -0700, Dai Ngo wrote:
>>>> 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 <helen.chao@xxxxxxxxxx>
>>>> Tested-by: Helen Chao <helen.chao@xxxxxxxxxx>
>>>> Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx>
>>>> ---
>>>> 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
