Received-SPF: pass (google.com: domain of linux-nfs+bounces-2156-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Trond Myklebust" <trondmy@hammerspace.com>
Cc: "anna@kernel.org" <anna@kernel.org>,
 "zhitao.li@smartx.com" <zhitao.li@smartx.com>,
 "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] NFS: Restore -EIO as error to return when "umount -f"
 aborts request.
In-reply-to: <81a96f8af0eb1b4d6ddaa44a451d8b2af4e7fa16.camel@hammerspace.com>
References: <170909199843.24797.6320949640369986924@noble.neil.brown.name>,
 <81a96f8af0eb1b4d6ddaa44a451d8b2af4e7fa16.camel@hammerspace.com>
Date: Mon, 04 Mar 2024 15:50:22 +1100
Message-id: <170952782221.24797.9882801008066120476@noble.neil.brown.name>

On Thu, 29 Feb 2024, Trond Myklebust wrote:
> On Wed, 2024-02-28 at 14:46 +1100, NeilBrown wrote:
> >=20
> > 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).
> >=20
> > Prior to Linux v5.2 EIO would be returned in these cases, which it is
> > more likely that applications will handle.
> >=20
> > This patch restores that behaviour so EIO is returned.
> >=20
> > Reported-and-tested-by: Zhitao Li <zhitao.li@smartx.com>
> > Closes:
> > https://lore.kernel.org/linux-nfs/CAPKjjnrYvzH8hEk9boaBt-fETX3VD2cjjN-Z6i=
NgwZpHqYUjWw@mail.gmail.com/
> > Fixes: ae67bd3821bb ("SUNRPC: Fix up task signalling")
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> > =C2=A0include/linux/sunrpc/sched.h | 2 +-
> > =C2=A0net/sunrpc/clnt.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 | 2 +-
> > =C2=A0net/sunrpc/sched.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0 | 6 +++---
> > =C2=A03 files changed, 5 insertions(+), 5 deletions(-)
> >=20
> > 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
> > *);
> > =C2=A0void		rpc_put_task_async(struct rpc_task *);
> > =C2=A0bool		rpc_task_set_rpc_status(struct rpc_task *task, int
> > rpc_status);
> > =C2=A0void		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);
> > =C2=A0void		rpc_exit_task(struct rpc_task *);
> > =C2=A0void		rpc_exit(struct rpc_task *, int);
> > =C2=A0void		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)
> > =C2=A0	trace_rpc_clnt_killall(clnt);
> > =C2=A0	spin_lock(&clnt->cl_lock);
> > =C2=A0	list_for_each_entry(rovr, &clnt->cl_tasks, tk_task)
> > -		rpc_signal_task(rovr);
> > +		rpc_signal_task(rovr, -EIO);
>=20
> 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.
>=20
> If we're going to replace ERESTARTSYS, then why would we not want to go
> for EINTR?

I have no particular opinion on which error code is correct, though I am
confident that ERESTARTSYS is not.  I do see the attraction of EINTR,
though "man 3 errno" suggests that is appropriate when a signal is
received, which is not the case here.

With this patch I was primarily reverting what appeared to be an
unintentional change in a previous patch.  Such a reversion can then be
used in -stable kernels if that seems appropriate.

If this exercise suggests to you that a different error code might be
more appropriate, then I think any such change should be in a separate
patch which only goes to mainline.

Thanks,
NeilBrown


>=20
> > =C2=A0	spin_unlock(&clnt->cl_lock);
> > =C2=A0}
> > =C2=A0EXPORT_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)
> > =C2=A0	}
> > =C2=A0}
> > =C2=A0
> > -void rpc_signal_task(struct rpc_task *task)
> > +void rpc_signal_task(struct rpc_task *task, int err)
> > =C2=A0{
> > =C2=A0	struct rpc_wait_queue *queue;
> > =C2=A0
> > =C2=A0	if (!RPC_IS_ACTIVATED(task))
> > =C2=A0		return;
> > =C2=A0
> > -	if (!rpc_task_set_rpc_status(task, -ERESTARTSYS))
> > +	if (!rpc_task_set_rpc_status(task, err))
> > =C2=A0		return;
> > =C2=A0	trace_rpc_task_signalled(task, task->tk_action);
> > =C2=A0	set_bit(RPC_TASK_SIGNALLED, &task->tk_runstate);
> > @@ -992,7 +992,7 @@ static void __rpc_execute(struct rpc_task *task)
> > =C2=A0			 * clean up after sleeping on some queue, we
> > don't
> > =C2=A0			 * break the loop here, but go around once
> > more.
> > =C2=A0			 */
> > -			rpc_signal_task(task);
> > +			rpc_signal_task(task, -ERESTARTSYS);
> > =C2=A0		}
> > =C2=A0		trace_rpc_task_sync_wake(task, task->tk_action);
> > =C2=A0	}
>=20
> --=20
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>=20
>=20
>=20