Re: [PATCH 0/2] SUNRPC: let cancelled RPC/RDMA requests drain before release

[Chuck Lever]

On 11/5/26 3:07 AM, Tim Menninger wrote:
> Cancelling an RPC task does not establish that the server has finished
> accessing its advertised Read or Write chunks. Releasing the request can
> invalidate those registrations before the server finishes using them. A
> subsequent remote access can fault the QP, disrupting other RPCs and
> driving further layout cancellation and transport recovery.
>
> This series lets eligible cancelled RPC/RDMA requests remain available
> for normal Reply-driven cleanup instead of immediately releasing them.

Apologies in advance for the length of this response.

My first concern on reading the cover was the client's memory
protection model. An RPC/RDMA Requester invalidates the MRs a request
registered before it releases the request, so that the Responder can
never see the contents of memory that has moved on to another use. I
wanted to be sure the series avoids the connection drops without
weakening that model.

RFC 8166 attaches the protection obligation to reuse, not to the
upper layer's authorization state. Section 4.4.1 requires that the
Requester protect a region from Responder access before the upper
layer may touch the data. Section 8.1.3 requires invalidation before
a terminated RPC's regions are released for reuse. Section 8.1.5
permits a Requester to invalidate on cancel, and gives the reason as
stopping a late Reply from altering memory after it has been reused.
That section then describes cancel-then-invalidate churn as a
denial-of-service exposure, because the Responder's replies terminate
the connection. That churn is the storm this series is addressing.

One might ask whether a layout revocation obliges the NFS client to
invalidate at once to fence the DS. The pNFS layouts were not written
with RPC/RDMA in mind, but the SCSI and NVMe layouts are the closest
analogue, since the host registers its buffers for the storage
controller much as a Requester advertises Write chunks. RFC 8154
Section 2.4.9 and RFC 9561 Section 2.2.3 put the fence on the MDS,
which preempts the client's reservation at the device, and that fence
covers in-flight I/O. The client learns it was fenced from a
Reservation Conflict on its outstanding commands. No host driver
unmaps a buffer under an outstanding command on a live connection.
The flexfiles layout over RPC/RDMA has no reservation, so the MDS
has no way to abort in-flight DS access, and the only fence the NFS
client has is a disconnect.

When these patches are applied, cancel now means "wait for the Reply
anyway, up to the request timeout". The task holds its slot, its MRs,
and its ULP references, including lsegs, for that whole window. I
expect the held lsegs are why the lseg patch is needed to avoid the
livelock. It looks like the request's RPC/RDMA credit is held for
that period as well. That might introduce a deadlock opportunity.

My other concern is complexity. __rpc_execute() already juggles
tk_rpc_status, the signalled state, queued callbacks, and the wakeup
of tasks that acquired an exit status while asleep, and that loop is
hard enough to reason about as it stands. This series adds a third
exit path, a drain, with its own exemption from the wakeup check, a
drain function that re-derives the request state under queue_lock
through three nested conditions, a special case in xprt_timer(), a new
-ECANCELED result from xprt_request_transmit(), and a condition on the
conditional disconnect in xprt_release(). Each of those is a new place
where a cancellation, a signal, a timeout, and a Reply can interleave,
and the interactions are spread across sched.c, xprt.c, and
transport.c.

A handful of specific complaints:

- xprt_request_release_needs_wait() takes the RPC_TASK_NEED_XMIT
  branch before consulting the transport. It dequeues the transmit
  and returns false, so a request with MRs still on rl_registered from
  an earlier transmission is released without draining. On RDMA a
  retransmit normally follows a disconnect, so the connect cookie
  would not match anyway, but the branch order means the transport is
  never asked. The commit message or a comment should carry that
  argument.

- A request whose marshal registered MRs but whose Send was never
  posted has RPC_TASK_NEED_RECV set and a live connection. It drains
  for a full timeout waiting for a Reply that cannot arrive, then
  xprt_rdma_timer forces a disconnect. The drain turns a cheap local
  failure into a connection drop.

- Widening every rpc_task by an atomic64 and converting the hot-path
  reads of tk_rpc_status is a lot of surface for one abort bit. Is
  there a reason the signalled state cannot live in tk_runstate with
  a documented ordering against tk_rpc_status? The race that
  5bbd6e863b15 fixed was the flag and the errno disagreeing, so the
  argument for the combined word needs to be made against that
  specific race.


-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)