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

[Li Lingfeng <lilingfeng3@huawei.com>]

在 2025/1/17 11:15, Trond Myklebust 写道:
> 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,

I'm not quite clear on how this can resolve the issue.
If we remove the RPC_TASK_SIGNALLED flag and replace setting tk_runstate
to RPC_TASK_SIGNALLED with setting tk_rpc_status to -ERESTARTSYS in
rpc_signal_task, wouldn't setting tk_rpc_status back to 0 in
__rpc_restart_call still lead to an infinite loop in the rpc_task?
Could you please provide a more detailed explanation?

Thanks.

>> 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.
>