Re: [PATCH v2 1/2] SUNRPC: add request-scoped disconnects when cancelling tasks

[Trond Myklebust <trondmy@kernel.org>]

On Wed, 2026-11-04 at 22:14 -0500, Trond Myklebust wrote:
> On Thu, 2026-11-05 at 02:02 +0000, tmenninger@purestorage.com wrote:
> > From: Tim Menninger <tmenninger@everpuredata.com>
> > 
> > rpc_cancel_tasks() allows callers to cancel a selected set of RPC
> > tasks,
> > but callers that also need to tear down the connection currently
> > have
> > to
> > disconnect at the rpc_clnt level.
> > 
> > That is broader than necessary for clients with multiple
> > transports.
> > A
> > cancelled request is associated with a specific rpc_xprt and
> > records
> > the
> > connection generation on which it was transmitted in
> > rq_connect_cookie.
> > 
> > Add rpc_cancel_tasks_and_disconnect() to cancel matching tasks and
> > mark
> > matching requests that remain on the transport transmit or receive
> > queues
> > for conditional transport disconnect when the request is released.
> > 
> > Store the disconnect indication in struct rpc_rqst rather than
> > struct
> > rpc_task. An RPC task can release one request and later acquire
> > another,
> > so task-scoped state could otherwise be consumed by a successor
> > request
> > and disconnect the wrong transport generation.
> > 
> > After cancelling matching tasks, walk the client's transports and
> > mark
> > matching requests belonging to that client that remain on the
> > transmit
> > or receive queues. An rpc_xprt may be shared by multiple rpc_clnt
> > instances, so requests belonging to other clients are left
> > untouched.
> > 
> > When a marked request is released, use
> > xprt_conditional_disconnect()
> > with
> > its saved rq_connect_cookie. This disconnects the transport only if
> > it is
> > still using the same connection generation on which the request was
> > sent,
> > and leaves a replacement connection untouched.
> > 
> > Requests that were never transmitted do not require a disconnect.
> > 
> 
> I'm all for optimising where it makes sense, however this is adding
> another spinlock protected section in xprt_release() for something
> that
> is clearly a rare corner case.
> 
> So how about something like the following change instead?
> 
Sorry, I oversimplified. Something like this:

8<------------------------------------------------------
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index 7fe8b91fa47c..42b0b04de659 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -2459,6 +2459,12 @@ static bool ff_layout_match_io(const struct rpc_task *task, const void *data)
 	return false;
 }
 
+static bool ff_layout_match_untransmitted_io(const struct rpc_task *task,
+					     const void *data)
+{
+	return !RPC_WAS_SENT(task) && ff_layout_match_io(task, data);
+}
+
 static void ff_layout_cancel_io(struct pnfs_layout_segment *lseg)
 {
 	struct nfs4_ff_layout_segment *flseg = FF_LAYOUT_LSEG(lseg);
@@ -2484,6 +2490,9 @@ static void ff_layout_cancel_io(struct pnfs_layout_segment *lseg)
 			clnt = ds_clp->cl_rpcclient;
 			if (!clnt)
 				continue;
+			rpc_cancel_tasks(clnt, -ECANCELED,
+					 ff_layout_match_untransmitted_io,
+					 lseg);
 			if (!rpc_cancel_tasks(clnt, -ECANCELED,
 					      ff_layout_match_io, lseg))
 				continue;
diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index 0dbdf3722537..28f5bacb59f9 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -219,8 +219,8 @@ struct rpc_task *rpc_run_bc_task(struct rpc_rqst *req,
 		struct rpc_timeout *timeout);
 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);
+bool 		rpc_task_set_rpc_status(struct rpc_task *task, int rpc_status);
+bool		rpc_task_try_cancel(struct rpc_task *task, int error);
 void		rpc_signal_task(struct rpc_task *);
 void		rpc_exit_task(struct rpc_task *);
 void		rpc_exit(struct rpc_task *, int);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 6cedc824cf82..503e95647270 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -930,7 +930,8 @@ unsigned long rpc_cancel_tasks(struct rpc_clnt *clnt, int error,
 			continue;
 		if (!fnmatch(task, data))
 			continue;
-		rpc_task_try_cancel(task, error);
+		if (!rpc_task_try_cancel(task, error))
+			continue;
 		count++;
 	}
 	spin_unlock(&clnt->cl_lock);
diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index 016f16ca5779..97727732400b 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -869,15 +869,16 @@ void rpc_signal_task(struct rpc_task *task)
 		rpc_wake_up_queued_task(queue, task);
 }
 
-void rpc_task_try_cancel(struct rpc_task *task, int error)
+bool rpc_task_try_cancel(struct rpc_task *task, int error)
 {
 	struct rpc_wait_queue *queue;
 
 	if (!rpc_task_set_rpc_status(task, error))
-		return;
+		return false;
 	queue = READ_ONCE(task->tk_waitqueue);
 	if (queue)
 		rpc_wake_up_queued_task(queue, task);
+	return true;
 }
 
 void rpc_exit(struct rpc_task *task, int status)

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