Re: [PATCH] xprtrdma: serialize unmap_sync with xprt_disconnect

Hi Tim -

Reviewer hat on.

I went over this patch with several LLM reviewers and my own
recollection of the code. The combined findings are below.

Summary: I agree there is a bug. The proposed fix introduces
technical debt that I would rather not see in-tree. Importantly,
there are areas where you can reuse existing code instead of
open-coding, the Fixes: tag might be wrong, and a few code
comments need updating as part of the fix.


On Tue, Aug 18, 2026, at 8:28 AM, [email protected] wrote:
>     xprtrdma: serialize unmap_sync with xprt_disconnect
>
>     frwr_unmap_sync() posts LOCAL_INV Work Requests to the QP and waits
>     for the matching CQE. This can race with rpcrdma_xprt_disconnect():
>
>       1. frwr_unmap_sync() snapshots r_xprt->rx_ep
>       2. rpcrdma_xprt_disconnect() drains the QP
>       3. frwr_unmap_sync() posts LOCAL_INV WRs after the drain
>       4. No CQE arrives and wait_for_completion() blocks indefinitely
>
>     The hung task appears in the rpciod workqueue:
>
>       Workqueue: rpciod rpc_async_schedule [sunrpc]
>       wait_for_completion()
>       frwr_unmap_sync()
>       xprt_rdma_free()
>       xprt_release()
>
>     Serialize synchronous MR invalidation against QP teardown with
>     rx_unmap_rwsem. frwr_unmap_sync() holds the read side through
>     ib_post_send(), then drops it before waiting for completion so that
>     disconnect can acquire the write side and drain the QP.
>
>     Track admitted synchronous unmaps with rx_unmap_active. Disconnect
>     waits for the count to reach zero after draining the QP and before
>     destroying transport resources. This ensures that no synchronous
>     unmap is still accessing the endpoint or MR completion state when
>     those resources are freed.
>
>     Tested on v7.2-rc6 under pNFS load with repeated data server restarts
>     to provoke rapid QP disconnect/reconnect cycling. Without the fix, a
>     hung task in frwr_unmap_sync() reproduced twice in six attempts. With
>     the fix, no hang occurred across 300 cycles.
>
>     The issue has also been observed on v6.8 and v6.18.34. The race has
>     been present since synchronous FRWR invalidation was introduced.
>
>     Fixes: c9918ff56dfb ("xprtrdma: Add ro_unmap_sync method for FRWR")
>     Signed-off-by: Tim Menninger <[email protected]>
>

Nice find, and the read side around the rx_ep sample and ib_post_send()
does close the window.

On the tag: c9918ff56dfb landed in v4.5.  There rx_ep was an embedded
struct rpcrdma_ep rather than a pointer, frwr_op_unmap_sync() posted to
ia->ri_id->qp, and rpcrdma_ep_disconnect() called rpcrdma_flush_cqs()
without draining the QP.  ib_drain_qp() reached that path in v4.7 with
550d7502cf66 ("xprtrdma: Use core ib_drain_qp() API"), and the sync unmap
moved to xprt_rdma_free() in v5.3 with 0ab115237025 ("xprtrdma: Wake RPCs
directly in rpcrdma_wc_send path").

The race as described needs a freeable rx_ep and the "r_xprt->rx_ep =
NULL" store, and both arrive in v5.7.  Should the tag be:

  Fixes: e28ce90083f0 ("xprtrdma: kmalloc rpcrdma_ep separate from rpcrdma_xprt")

That would also make this sentence inaccurate:

>     The race has been present since synchronous FRWR invalidation was
>     introduced.

The changelog does not say why only the sync path needs this.
xprt_rdma_free() runs from xprt_release() after ->release_xprt has
dropped the transport send lock, so it is the one LOCAL_INV poster that
disconnect is not serialized against.  frwr_unmap_async() runs inline in
rpcrdma_wc_receive()->rpcrdma_reply_handler(), which ib_drain_rq() orders
ahead of ib_drain_sq().  Worth a sentence?

> diff --git a/net/sunrpc/xprtrdma/frwr_ops.c b/net/sunrpc/xprtrdma/frwr_ops.c
> index e5c71cf705a3..a4733d75664f 100644
> --- a/net/sunrpc/xprtrdma/frwr_ops.c
> +++ b/net/sunrpc/xprtrdma/frwr_ops.c
> @@ -567,11 +567,20 @@ static void frwr_wc_localinv_wake(struct ib_cq *cq, struct ib_wc *wc)
>  void frwr_unmap_sync(struct rpcrdma_xprt *r_xprt, struct rpcrdma_req *req)
>  {
>  	struct ib_send_wr *first, **prev, *last;
> -	struct rpcrdma_ep *ep = r_xprt->rx_ep;
>  	const struct ib_send_wr *bad_wr;
> +	struct rpcrdma_ep *ep;
>  	struct rpcrdma_mr *mr;
>  	int rc;
>  
> +	/* serialize against rpcrdma_xprt_disconnect() */
> +	down_read(&r_xprt->rx_unmap_rwsem);
> +
> +	ep = r_xprt->rx_ep;
> +	if (!ep)
> +		goto out_unlock;

The kernel-doc above frwr_unmap_sync() says it "Sleeps until it is safe
for the host CPU to access the previously mapped memory regions."  With
this early return that is now conditional.  Would it help to note there
that rpcrdma_reqs_reset() has already released this req's MRs whenever
rx_ep is NULL?  The correctness of returning here depends on that, and it
happens in another file.

> +
> +	atomic_inc(&r_xprt->rx_unmap_active);
> +
>  	/* ORDER: Invalidate all of the MRs first
>  	 *
>  	 * Chain the LOCAL_INV Work Requests and post them with
> @@ -614,6 +623,8 @@ void frwr_unmap_sync(struct rpcrdma_xprt *r_xprt, struct rpcrdma_req *req)
>  	bad_wr = NULL;
>  	rc = ib_post_send(ep->re_id->qp, first, &bad_wr);

The comment just above this hunk still reads:

	/* Transport disconnect drains the receive CQ before it
	 * replaces the QP. The RPC reply handler won't call us
	 * unless re_id->qp is a valid pointer.
	 */

Is that still the rationale here?  frwr_unmap_sync()'s only caller is
xprt_rdma_free(), not the reply handler, and the new down_read() is there
because the receive CQ drain does not cover this path.  The same comment
sits above frwr_unmap_async(), where it does still hold.

>  
> +	up_read(&r_xprt->rx_unmap_rwsem);
> +
>  	/* The final LOCAL_INV WR in the chain is supposed to
>  	 * do the wake. If it was never posted, the wake will
>  	 * not happen, so don't wait in that case.
> @@ -621,7 +632,7 @@ void frwr_unmap_sync(struct rpcrdma_xprt *r_xprt, struct rpcrdma_req *req)
>  	if (bad_wr != first)
>  		wait_for_completion(&mr->mr_linv_done);
>  	if (!rc)
> -		return;
> +		goto out_atomic_dec;
>  
>  	/* On error, the MRs get destroyed once the QP has drained. */
>  	trace_xprtrdma_post_linv_err(req, rc);
> @@ -629,6 +640,14 @@ void frwr_unmap_sync(struct rpcrdma_xprt *r_xprt, struct rpcrdma_req *req)
>  	/* Force a connection loss to ensure complete recovery.
>  	 */
>  	rpcrdma_force_disconnect(ep);
> +
> +out_atomic_dec:
> +	if (atomic_dec_and_test(&r_xprt->rx_unmap_active))
> +		wake_up_var(&r_xprt->rx_unmap_active);
> +	return;
> +
> +out_unlock:
> +	up_read(&r_xprt->rx_unmap_rwsem);
>  }
>  
>  /**

Could rx_unmap_active come out entirely?

It is needed because the waiter sleeps on mr->mr_linv_done, and
rpcrdma_mrs_destroy() frwr_mr_release()s that mr once the count reaches
zero.  Would moving that completion into struct rpcrdma_req decouple the
two?  A req is freed only by rpcrdma_req_destroy() from
rpcrdma_buffer_destroy(), and xprt_rdma_free() is running on this one, so
it cannot be freed under the wait.  rpcrdma_req_reset() leaves it alone
as well.

frwr_wc_localinv_wake() can reach it without a new field.  mr->mr_req is
already there, and frwr_wc_localinv_done() dereferences it the same way
for rl_reply.

The other use is keeping ep alive for the rpcrdma_force_disconnect(ep)
above, after up_read().  Would rpcrdma_ep_get() and rpcrdma_ep_put()
across the read section cover that one?

Together those would drop the atomic_t, the wait_var_event() in
rpcrdma_xprt_disconnect(), and the atomic_set() in xprt_setup_rdma(),
and leave the rwsem with one job.

This isn't a bug, but if the counter stays, is this
atomic_dec_and_wake_up() from linux/wait_bit.h?  nfsd41_cb_inflight_end()
uses it for the same inflight-counter pattern.

> diff --git a/net/sunrpc/xprtrdma/transport.c b/net/sunrpc/xprtrdma/transport.c
> index d4e6746d8ecd..e2b6287511a4 100644
> --- a/net/sunrpc/xprtrdma/transport.c
> +++ b/net/sunrpc/xprtrdma/transport.c
> @@ -364,6 +364,9 @@ xprt_setup_rdma(struct xprt_create *args)
>  	INIT_DELAYED_WORK(&new_xprt->rx_connect_worker,
>  			  xprt_rdma_connect_worker);
>  
> +	init_rwsem(&new_xprt->rx_unmap_rwsem);
> +	atomic_set(&new_xprt->rx_unmap_active, 0);
> +
>  	xprt->max_payload = RPCRDMA_MAX_DATA_SEGS << PAGE_SHIFT;
>  
>  	return xprt;

This isn't a bug, but is the atomic_set() needed?  xprt_alloc() kzallocs
the whole struct rpcrdma_xprt.

> diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
> index 04b286223b24..ae8d06d1cf29 100644
> --- a/net/sunrpc/xprtrdma/verbs.c
> +++ b/net/sunrpc/xprtrdma/verbs.c
> @@ -580,18 +580,24 @@ int rpcrdma_xprt_connect(struct rpcrdma_xprt *r_xprt)
>   */
>  void rpcrdma_xprt_disconnect(struct rpcrdma_xprt *r_xprt)
>  {
> -	struct rpcrdma_ep *ep = r_xprt->rx_ep;
> +	struct rpcrdma_ep *ep;
>  	struct rdma_cm_id *id;
>  	int rc;
>  
> +	down_write(&r_xprt->rx_unmap_rwsem);
> +
> +	ep = r_xprt->rx_ep;
>  	if (!ep)
> -		return;
> +		goto out_unlock;
>  
>  	id = ep->re_id;
>  	rc = rdma_disconnect(id);
>  	trace_xprtrdma_disconnect(r_xprt, rc);
>  
>  	rpcrdma_xprt_drain(r_xprt);
> +	wait_var_event(&r_xprt->rx_unmap_active,
> +		       !atomic_read(&r_xprt->rx_unmap_active));
> +
>  	rpcrdma_reps_unmap(r_xprt);
>  	rpcrdma_sendctxs_destroy(r_xprt);
>  	rpcrdma_reqs_reset(r_xprt);
> @@ -601,6 +607,9 @@ void rpcrdma_xprt_disconnect(struct rpcrdma_xprt *r_xprt)
>  		rdma_destroy_id(id);
>  
>  	r_xprt->rx_ep = NULL;
> +
> +out_unlock:
> +	up_write(&r_xprt->rx_unmap_rwsem);
>  }
>  
>  /* Fixed-size circular FIFO queue. This implementation is wait-free and

This wait is the half of rx_unmap_active that a req-resident completion
would remove, leaving drain-then-teardown as before.

> diff --git a/net/sunrpc/xprtrdma/xprt_rdma.h b/net/sunrpc/xprtrdma/xprt_rdma.h
> index 4cbc941e4a3e..419d5952150a 100644
> --- a/net/sunrpc/xprtrdma/xprt_rdma.h
> +++ b/net/sunrpc/xprtrdma/xprt_rdma.h

[ ... ]

> @@ -449,6 +450,10 @@ struct rpcrdma_xprt {
>  	struct delayed_work	rx_connect_worker;
>  	struct rpc_timeout	rx_timeout;
>  	struct rpcrdma_stats	rx_stats;
> +
> +	/* for serializing xprt disconnect and sync MR unmap */
> +	struct rw_semaphore	rx_unmap_rwsem;
> +	atomic_t                rx_unmap_active;
>  };
>  
>  #define rpcx_to_rdmax(x) container_of(x, struct rpcrdma_xprt, rx_xprt)

Have you considered putting these on struct rpcrdma_ep instead?
rpcrdma_xprt_drain() already fences the receive side against the drain
with ep->re_receiving and ep->re_done, and an rwsem on the transport
outlives every ep it guards.

Would the comment be more useful naming the rule rather than the two
functions?  Something like: read side held across sampling rx_ep and
posting to its QP.

This isn't a bug, but rx_unmap_active is space-aligned where the rest of
the struct uses a tab.

-- 
Chuck Lever