Re: [PATCH] SUNRPC: Set tk_rpc_status when RPC_TASK_SIGNALLED is detected

[Trond Myklebust <trondmy@hammerspace.com>]

On Fri, 2025-01-17 at 10:29 +0800, yangerkun wrote:
> 
> 
> 在 2025/1/17 4:52, Trond Myklebust 写道:
> > On Thu, 2025-01-16 at 19:43 +0800, yangerkun wrote:
> > > Hi,
> > > 
> > > Thanks for the patch.
> > > 
> > > Before 39494194f93b("SUNRPC: Fix races with rpc_killall_tasks()",
> > > every
> > > time we set RPC_TASK_SIGNALLED, when we go through __rpc_execute,
> > > this
> > > rpc_task will immediate break and exist.
> > > 
> > > However after that, __rpc_execute won't judge RPC_TASK_SIGNNALED,
> > > so
> > > for
> > > the case like you point out below, even after your commit
> > > rpc_check_timeout will help break and exist eventually, but this
> > > rpc_task has already do some work. I prefer reintroduce judging
> > > RPC_TASK_SIGNNALED in __rpc_execute to help exist immediatly.
> > > 
> > 
> > Better yet... Let's get rid of the RPC_TASK_SIGNALLED flag
> > altogether
> > and just replace
> > 
> > #define RPC_TASK_SIGNALLED(task) (READ_ONCE(task->tk_rpc_status) ==
> > -ERESTARTSYS)
> 
> Hi,
> 
> Thanks for your reply! Yeah, if all the places where tk_rpc_status is
> updated are by calling rpc_task_set_rpc_status, we can use
> task->tk_rpc_status == -ERESTARTSYS to determine whether rpc_task is
> RPC_TASK_SIGNALLED. But for the case like Li has provided,
> __rpc_restart_call won't do this, and will overwrite tk_rpc_status 
> unconditionally. This won't be a stable solution. Maybe it's better
> to
> change __rpc_restart_call calling rpc_task_set_rpc_status too? And
> __rpc_execute will be enough to help solve this case.
> 
> 

That would break __rpc_restart_call() to the point of rendering it
completely useless.
The whole purpose of that call is to give the NFS layer a chance to
handle errors in the exit callback, and then kick off a fresh call.
Your suggestion would mean that any RPC level error sticks around, and
causes the new call to immediately fail.

I see no point in doing anything more than fixing the looping
behaviour. Eliminating the redundant flag will do that.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com