Re: [PATCH] SUNRPC: add transport details to timeout diagnostics

[manjunath.b.patil@oracle.com]

On 8/31/26 12:33 PM, manjunath.b.patil@oracle.com wrote:
> On 8/20/26 11:59 AM, Petr Vorel wrote:
>>  > On 8/20/26 4: 13 AM, Petr Vorel wrote: > > > The existing RPC 
>> timeout messages identify only the RPC program and > > > server name. 
>> That makes it difficult to distinguish a server-side > > > response 
>> stall from
>> 
>>
>>> On 8/20/26 4:13 AM, Petr Vorel wrote:
>>> >  > The existing RPC timeout messages identify only the RPC program 
>>> and >
>>> > server name. That makes it difficult to distinguish a server-side >
>>> > response stall from a transport that is disconnected, reconnecting, 
>>> > or
>>> > using a different
>>
>>
>>> > > The existing RPC timeout messages identify only the RPC program and
>>> > > server name. That makes it difficult to distinguish a server-side
>>> > > response stall from a transport that is disconnected, reconnecting,
>>> > > or using a different connection.
>>
>>> > > Include the RPC client id, transport id, current transport 
>>> connection
>>> > > cookie, and RPC task owner in the ratelimited timeout messages. 
>>> These
>>> > > fields provide a compact way to correlate a timeout message with
>>> > > rpcctl and other transport state.
>>
>>> > > For example, an NFS timeout reports:
>>
>>> > >   nfs: server 192.168.50.1 clid=4 xprt=1 cc=1 owner=6245
>>> > >   not responding, timed out
>>
>>> > Nice improvement, thanks!
>>
>>
>>> > > Assisted-by: Codex:gpt-5
>>> > > Signed-off-by: Manjunath Patil <manjunath.b.patil@oracle.com>
>>> > > ---
>>> > >  net/sunrpc/clnt.c | 20 +++++++++++++-------
>>> > >  1 file changed, 13 insertions(+), 7 deletions(-)
>>
>>> > > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>>> > > index efa26899bc7d..6b5e2787dc81 100644
>>> > > --- a/net/sunrpc/clnt.c
>>> > > +++ b/net/sunrpc/clnt.c
>>> > > @@ -2534,17 +2534,21 @@ static void
>>> > >  rpc_check_timeout(struct rpc_task *task)
>>> > >  {
>>> > >      struct rpc_clnt    *clnt = task->tk_client;
>>> > > +    struct rpc_rqst    *req;
>>> > > +    struct rpc_xprt    *xprt;
>>
>>> > nit: Would it work to assign req and task earlier (just readability?)
>>> > Or is it not safe before checking RPC_SIGNALLED(task)?
>>
>>> >     struct rpc_clnt    *clnt = task->tk_client;
>>> >     struct rpc_rqst    *req = task->tk_rqstp;
>>> >     struct rpc_xprt    *xprt = req->rq_xprt;
>>
>>> > The rest LGTM.
>>> > Reviewed-by: Petr Vorel <pvorel@suse.cz>
>>
> 
> Hi Trond, Anna, and Chuck,
> 
> Could you please let me know the next steps for this patch submission? 
> Should I wait longer for review, or are there any formatting or 
> submission issues that I need to fix?
> 
> I also noticed that the Patchwork event shows the patch as "Not 
> Applicable":
> 
> https://patchwork.kernel.org/api/1.2/events/?patch=14757794
> 
> Could you clarify what that means for this submission?
> 
> Thanks,
> Manjunath

Hi Trond and Anna,

I am following up on this SUNRPC client patch. Petr Vorel reviewed it, 
and his question about the placement of the request and transport 
assignments was resolved in the thread.

Chuck clarified that the Patchwork "Not Applicable" state reflects NFSD 
tracking and does not indicate a decision on this NFS client patch.

Could you let me know whether this patch is suitable for the NFS client 
queue, or if you would like any changes or additional testing? I am 
happy to send a revised version if needed.

Thanks,
Manjunath
> 
>>> > Kind regards,
>>> > Petr
>>
>>> Hi Petr,
>>
>> Hi Manjunath,
>>
>>> Thanks for the review and the Reviewed-by tag.
>>
>> yw.
>>
>>> I kept the assignments after RPC_SIGNALLED(task) intentionally, 
>>> preserving
>>> the existing behavior where a signalled task returns before we 
>>> dereference
>>> task->tk_rqstp. xprt is also needed only after xprt_adjust_timeout(req)
>>> reports an expired timeout.
>>
>> Understand, fair enough.
>>
>>> I would prefer to retain the current ordering for those reasons. Also 
>>> I am
>>> open if maintainers chose to alter it.
>>
>> +1
>>
>> Kind regards,
>> Petr
>>
>>> Thanks,                   Manjunath
>>
>>
>>> > >      if (RPC_SIGNALLED(task))
>>> > >          return;
>>> > > -    if (xprt_adjust_timeout(task->tk_rqstp) == 0)
>>> > > +    req = task->tk_rqstp;
>>> > > +    if (xprt_adjust_timeout(req) == 0)
>>> > >          return;
>>> > > +    xprt = req->rq_xprt;
>>> > >      trace_rpc_timeout_status(task);
>>> > >      task->tk_timeouts++;
>>> > > -    if (RPC_IS_SOFTCONN(task) && !rpc_check_connected(task- 
>>> >tk_rqstp)) {
>>> > > +    if (RPC_IS_SOFTCONN(task) && !rpc_check_connected(req)) {
>>> > >          rpc_call_rpcerror(task, -ETIMEDOUT);
>>> > >          return;
>>> > >      }
>>> > > @@ -2556,14 +2560,15 @@ rpc_check_timeout(struct rpc_task *task)
>>> > >           * connection gets terminally broken.
>>> > >           */
>>> > >          if ((task->tk_flags & RPC_TASK_NO_RETRANS_TIMEOUT) &&
>>> > > -            rpc_check_connected(task->tk_rqstp))
>>> > > +            rpc_check_connected(req))
>>> > >              return;
>>> > >          if (clnt->cl_chatty) {
>>> > >              pr_notice_ratelimited(
>>> > > -                "%s: server %s not responding, timed out\n",
>>> > > +                "%s: server %s clid=%u xprt=%u cc=%u owner=%d 
>>> not responding, timed out\n",
>>> > >                  clnt->cl_program->name,
>>> > > -                task->tk_xprt->servername);
>>> > > +                xprt->servername, clnt->cl_clid, xprt->id,
>>> > > +                READ_ONCE(xprt->connect_cookie), task->tk_owner);
>>> > >          }
>>> > >          if (task->tk_flags & RPC_TASK_TIMEOUT)
>>> > >              rpc_call_rpcerror(task, -ETIMEDOUT);
>>> > > @@ -2576,9 +2581,10 @@ rpc_check_timeout(struct rpc_task *task)
>>> > >          task->tk_flags |= RPC_CALL_MAJORSEEN;
>>> > >          if (clnt->cl_chatty) {
>>> > >              pr_notice_ratelimited(
>>> > > -                "%s: server %s not responding, still trying\n",
>>> > > +                "%s: server %s clid=%u xprt=%u cc=%u owner=%d 
>>> not responding, still trying\n",
>>> > >                  clnt->cl_program->name,
>>> > > -                task->tk_xprt->servername);
>>> > > +                xprt->servername, clnt->cl_clid, xprt->id,
>>> > > +                READ_ONCE(xprt->connect_cookie), task->tk_owner);
>>> > >          }
>>> > >      }
>>> > >      rpc_force_rebind(clnt);
>>
>>
>