Re: [PATCH] NFS: Restore -EIO as error to return when "umount -f" aborts request.

[Trond Myklebust <trondmy@hammerspace.com>]

On Wed, 2024-02-28 at 14:46 +1100, NeilBrown wrote:
> 
> When "umount -f" is used to abort all outstanding requests on an NFS
> mount, some pending systemcalls can be expected to return an error.
> Currently this error is ERESTARTSYS which should never be exposed to
> applications (it should only be returned due to a signal).
> 
> Prior to Linux v5.2 EIO would be returned in these cases, which it is
> more likely that applications will handle.
> 
> This patch restores that behaviour so EIO is returned.
> 
> Reported-and-tested-by: Zhitao Li <zhitao.li@smartx.com>
> Closes:
> https://lore.kernel.org/linux-nfs/CAPKjjnrYvzH8hEk9boaBt-fETX3VD2cjjN-Z6iNgwZpHqYUjWw@mail.gmail.com/
> Fixes: ae67bd3821bb ("SUNRPC: Fix up task signalling")
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  include/linux/sunrpc/sched.h | 2 +-
>  net/sunrpc/clnt.c            | 2 +-
>  net/sunrpc/sched.c           | 6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/sunrpc/sched.h
> b/include/linux/sunrpc/sched.h
> index 2d61987b3545..ed3a116efd5d 100644
> --- a/include/linux/sunrpc/sched.h
> +++ b/include/linux/sunrpc/sched.h
> @@ -222,7 +222,7 @@ void		rpc_put_task(struct rpc_task
> *);
>  void		rpc_put_task_async(struct rpc_task *);
>  bool		rpc_task_set_rpc_status(struct rpc_task *task, int
> rpc_status);
>  void		rpc_task_try_cancel(struct rpc_task *task, int
> error);
> -void		rpc_signal_task(struct rpc_task *);
> +void		rpc_signal_task(struct rpc_task *, int);
>  void		rpc_exit_task(struct rpc_task *);
>  void		rpc_exit(struct rpc_task *, int);
>  void		rpc_release_calldata(const struct rpc_call_ops *,
> void *);
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index cda0935a68c9..cdbdfae13030 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -895,7 +895,7 @@ void rpc_killall_tasks(struct rpc_clnt *clnt)
>  	trace_rpc_clnt_killall(clnt);
>  	spin_lock(&clnt->cl_lock);
>  	list_for_each_entry(rovr, &clnt->cl_tasks, tk_task)
> -		rpc_signal_task(rovr);
> +		rpc_signal_task(rovr, -EIO);

rpc_killall_tasks() is called by rpc_shutdown_client(). It is not
obvious to me that EIO is an appropriate return value in all the cases
where that is being called.

If we're going to replace ERESTARTSYS, then why would we not want to go
for EINTR?

>  	spin_unlock(&clnt->cl_lock);
>  }
>  EXPORT_SYMBOL_GPL(rpc_killall_tasks);
> diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
> index 6debf4fd42d4..e4f36fe16808 100644
> --- a/net/sunrpc/sched.c
> +++ b/net/sunrpc/sched.c
> @@ -852,14 +852,14 @@ void rpc_exit_task(struct rpc_task *task)
>  	}
>  }
>  
> -void rpc_signal_task(struct rpc_task *task)
> +void rpc_signal_task(struct rpc_task *task, int err)
>  {
>  	struct rpc_wait_queue *queue;
>  
>  	if (!RPC_IS_ACTIVATED(task))
>  		return;
>  
> -	if (!rpc_task_set_rpc_status(task, -ERESTARTSYS))
> +	if (!rpc_task_set_rpc_status(task, err))
>  		return;
>  	trace_rpc_task_signalled(task, task->tk_action);
>  	set_bit(RPC_TASK_SIGNALLED, &task->tk_runstate);
> @@ -992,7 +992,7 @@ static void __rpc_execute(struct rpc_task *task)
>  			 * clean up after sleeping on some queue, we
> don't
>  			 * break the loop here, but go around once
> more.
>  			 */
> -			rpc_signal_task(task);
> +			rpc_signal_task(task, -ERESTARTSYS);
>  		}
>  		trace_rpc_task_sync_wake(task, task->tk_action);
>  	}

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