[Devel] [PATCH VZ10] fs/fuse kio: fix RPC response drop for large messages sent over RDMA

[Devel] [PATCH VZ10] fs/fuse kio: fix RPC response drop for large messages sent over RDMA

[Liu Kui]

When a large message is sent via RDMA READ, it is completed with
rio_msg_sent() - which moves the request onto the RPC pending queue
where the response lookup can find it - only after both the
RDMA_READ_REQ TX completion and the peer's RDMA_READ_ACK have been
observed. However these two events are independent and not ordered:
the ACK, and even the response to the request, can arrive before the
local TX completion is polled. When that happens, the response finds
no matching request on the RPC pending queue and is dropped, stalling
the request until the RPC timeout logic kicks in.

Fix it by completing the message in rio_handle_rx_read_ack() as soon
as the RDMA_READ_ACK arrives. Messages in a single RX queue are
guaranteed to be delivered in order, so the ACK is always processed
before the response, and the request is therefore on the RPC pending
queue by the time its response arrives. This is also what the comment
in rio_submit() has promised all along.

The tx buffer is still recycled only after both events have been seen.
A TX send failure is handled by the connection abort logic as before.
The same applies to an error status carried in the RDMA_READ_ACK:
since the request msg has already been added to the rpc's pending
queue, the rio abort triggers an rpc abort, which completes the msg
with the same PCS_ERR_NET_ABORT error as before.

https://virtuozzo.atlassian.net/browse/VSTOR-141673

Signed-off-by: Liu Kui <kui.liu@virtuozzo.com>
---
 fs/fuse/kio/pcs/pcs_rdma_io.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/fs/fuse/kio/pcs/pcs_rdma_io.c b/fs/fuse/kio/pcs/pcs_rdma_io.c
index 1c464e0e60f3..14beab8fda8c 100644
--- a/fs/fuse/kio/pcs/pcs_rdma_io.c
+++ b/fs/fuse/kio/pcs/pcs_rdma_io.c
@@ -797,7 +797,6 @@ static inline void rio_kick_write_queue(struct pcs_rdmaio *rio)
 
 static void rio_handle_tx(struct pcs_rdmaio *rio, struct rio_tx *tx, int ok)
 {
-	struct pcs_msg *msg = tx->msg;
 	u64 xid = tx->xid;
 
 	/* override remote success if we already aborted */
@@ -825,13 +824,6 @@ static void rio_handle_tx(struct pcs_rdmaio *rio, struct rio_tx *tx, int ok)
 		default:
 			BUG();
 	}
-
-	if (msg) {
-		if (!ok)
-			pcs_set_local_error(&msg->error, PCS_ERR_NET_ABORT);
-
-		rio_msg_sent(rio, NULL, msg, 1);
-	}
 }
 
 /*
@@ -931,6 +923,7 @@ static int rio_handle_rx_read_ack(struct pcs_rdmaio *rio,
 
 	list_for_each_entry(tx, &rio->active_txs, list)
 		if (tx->xid == rack->xid) {
+			rio_msg_sent(rio, NULL, tx->msg, 1);
 			list_del(&tx->list);
 			rio_handle_tx(rio, tx, !rack->status);
 			return 0;
-- 
2.50.1 (Apple Git-155)

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH VZ10] fs/fuse kio: fix RPC response drop for large messages sent over RDMA

[Konstantin Khorenko]

fs/fuse kio: fix RPC response drop for large messages sent over RDMA

The patch moves the ->done() completion of a large outgoing message from
rio_handle_tx(), where it waited for both the local TX completion and the
peer's RDMA_READ_ACK, into rio_handle_rx_read_ack(), so the request reaches
the rpc pending queue before its response can arrive. The tx itself is still
recycled only after both events.

> diff --git a/fs/fuse/kio/pcs/pcs_rdma_io.c b/fs/fuse/kio/pcs/pcs_rdma_io.c
> index 1c464e0e60f3e..14beab8fda8c9 100644
> --- a/fs/fuse/kio/pcs/pcs_rdma_io.c
> +++ b/fs/fuse/kio/pcs/pcs_rdma_io.c
> @@ -797,7 +797,6 @@ static inline void rio_kick_write_queue(struct pcs_rdmaio *rio)
>
>  static void rio_handle_tx(struct pcs_rdmaio *rio, struct rio_tx *tx, int ok)
>  {
> -	struct pcs_msg *msg = tx->msg;
>  	u64 xid = tx->xid;
>
>  	/* override remote success if we already aborted */
> @@ -825,13 +824,6 @@ static void rio_handle_tx(struct pcs_rdmaio *rio, struct rio_tx *tx, int ok)
>  		default:
>  			BUG();
>  	}
> -
> -	if (msg) {
> -		if (!ok)
> -			pcs_set_local_error(&msg->error, PCS_ERR_NET_ABORT);
> -
> -		rio_msg_sent(rio, NULL, msg, 1);
> -	}
>  }

[Severity: Critical]
Does this also drop the completion of messages received over RDMA READ?

The removed block ran for every tx_state, not only for the two states of an
outgoing large message. A tx in state TX_SUBMIT_RDMA_READ_ACK carries the
*incoming* message that the local side pulls from the peer, and it is set up
in rio_submit_rdma_read():

fs/fuse/kio/pcs/pcs_rdma_io.c:rio_submit_rdma_read() {
	...
	tx->tx_state = TX_SUBMIT_RDMA_READ_ACK;
	tx->msg = msg;
	tx->xid = rb->xid;
	...
}

The msg there comes from rio_handle_rx_immediate(), which calls
rio->netio.getmsg() and then completes the message itself only when the whole
body arrived inline:

fs/fuse/kio/pcs/pcs_rdma_io.c:rio_handle_rx_immediate() {
	...
	if (len == msg->size) {
		msg->done(msg);
	} else if (rio_submit_rdma_read(rio, msg, offset, rb, true) == -EAGAIN) {
	...
}

So for a message that needs an RDMA READ, the delivery to the rpc layer used
to happen when the read completed:

rio_tx_done() -> rio_handle_tx() -> case TX_SUBMIT_RDMA_READ_ACK
	rio_put_tx()			/* clears tx->msg */
	rio_submit(SUBMIT_RDMA_READ_ACK)
	...
	if (msg)
		rio_msg_sent(rio, NULL, msg, 1);	/* msg->done(msg) */

After this patch nothing calls ->done() on that path. rio_handle_rx_read_ack()
does not cover it: it walks rio->active_txs, and a TX_SUBMIT_RDMA_READ_ACK tx
is never put on that list - only rio_msg_sent(..., done == 0) adds a tx there,
and that is reached only from the rio_submit() send path.

Does this mean every received message larger than RDMA_THRESHOLD is now never
handed to rpc_work_input()/the response handler, so the msg allocated by
rpc_get_hdr() leaks and the corresponding request only completes when the kill
calendar fires?

The same block was also the only place that reported a failed RDMA READ to
that message:

	if (!ok)
		pcs_set_local_error(&msg->error, PCS_ERR_NET_ABORT);

Would keeping the msg handling for the TX_SUBMIT_RDMA_READ_ACK case, and
clearing tx->msg on the ack path instead, cover both message directions?

> @@ -931,6 +923,7 @@ static int rio_handle_rx_read_ack(struct pcs_rdmaio *rio,
>
>  	list_for_each_entry(tx, &rio->active_txs, list)
>  		if (tx->xid == rack->xid) {
> +			rio_msg_sent(rio, NULL, tx->msg, 1);
>  			list_del(&tx->list);
>  			rio_handle_tx(rio, tx, !rack->status);
>  			return 0;

[Severity: Low]
This isn't a bug today, but is there a reason to complete the message before
unlinking the tx?

rio_msg_sent() calls msg->done(msg), after which the msg may be freed, while
the tx is still on rio->active_txs with tx->msg pointing at it. Both users of
that list, rio_cleanup() and pcs_rdma_next_timeout(), dereference tx->msg:

fs/fuse/kio/pcs/pcs_rdma_io.c:pcs_rdma_next_timeout() {
	...
	if (!list_empty(&rio->active_txs)) {
		tx = list_first_entry(&rio->active_txs, struct rio_tx, list);
		return tx->msg->start_time + rio->send_timeout;
	}
	...
}

Doing the list_del() first would keep the completed msg unreachable from the
list. tx->msg is also left pointing at the completed message until the TX
completion reaches rio_put_tx().

[Severity: Low]
The commit fixes a bug that came in with the original RDMA transport support.
Should it carry

Fixes: bcf01a42ff8c ("fs/fuse kio: implement support RDMA transport")

The Feature: vStorage tag used by the neighbouring fs/fuse kio commits, and
present in the reverted 21b2e4b35dfa ("fs/fuse kio: fix lost rpc response over
rdma io"), is missing as well.

-- 
Konstantin Khorenko <khorenko@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH VZ10] fs/fuse kio: fix RPC response drop for large messages sent over RDMA

[Konstantin Khorenko]

Liu, also please let me know if this patch replacement (ok, replacement with some next fixed version you send soon)
is a must for HCI 8 release or we can release the HCI 8 kernel as is and change the patch later in next releases.

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 9/14/26 12:40, Liu Kui wrote:
> When a large message is sent via RDMA READ, it is completed with
> rio_msg_sent() - which moves the request onto the RPC pending queue
> where the response lookup can find it - only after both the
> RDMA_READ_REQ TX completion and the peer's RDMA_READ_ACK have been
> observed. However these two events are independent and not ordered:
> the ACK, and even the response to the request, can arrive before the
> local TX completion is polled. When that happens, the response finds
> no matching request on the RPC pending queue and is dropped, stalling
> the request until the RPC timeout logic kicks in.
> 
> Fix it by completing the message in rio_handle_rx_read_ack() as soon
> as the RDMA_READ_ACK arrives. Messages in a single RX queue are
> guaranteed to be delivered in order, so the ACK is always processed
> before the response, and the request is therefore on the RPC pending
> queue by the time its response arrives. This is also what the comment
> in rio_submit() has promised all along.
> 
> The tx buffer is still recycled only after both events have been seen.
> A TX send failure is handled by the connection abort logic as before.
> The same applies to an error status carried in the RDMA_READ_ACK:
> since the request msg has already been added to the rpc's pending
> queue, the rio abort triggers an rpc abort, which completes the msg
> with the same PCS_ERR_NET_ABORT error as before.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-141673
> 
> Signed-off-by: Liu Kui <kui.liu@virtuozzo.com>
> ---
>  fs/fuse/kio/pcs/pcs_rdma_io.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/fs/fuse/kio/pcs/pcs_rdma_io.c b/fs/fuse/kio/pcs/pcs_rdma_io.c
> index 1c464e0e60f3..14beab8fda8c 100644
> --- a/fs/fuse/kio/pcs/pcs_rdma_io.c
> +++ b/fs/fuse/kio/pcs/pcs_rdma_io.c
> @@ -797,7 +797,6 @@ static inline void rio_kick_write_queue(struct pcs_rdmaio *rio)
>  
>  static void rio_handle_tx(struct pcs_rdmaio *rio, struct rio_tx *tx, int ok)
>  {
> -	struct pcs_msg *msg = tx->msg;
>  	u64 xid = tx->xid;
>  
>  	/* override remote success if we already aborted */
> @@ -825,13 +824,6 @@ static void rio_handle_tx(struct pcs_rdmaio *rio, struct rio_tx *tx, int ok)
>  		default:
>  			BUG();
>  	}
> -
> -	if (msg) {
> -		if (!ok)
> -			pcs_set_local_error(&msg->error, PCS_ERR_NET_ABORT);
> -
> -		rio_msg_sent(rio, NULL, msg, 1);
> -	}
>  }
>  
>  /*
> @@ -931,6 +923,7 @@ static int rio_handle_rx_read_ack(struct pcs_rdmaio *rio,
>  
>  	list_for_each_entry(tx, &rio->active_txs, list)
>  		if (tx->xid == rack->xid) {
> +			rio_msg_sent(rio, NULL, tx->msg, 1);
>  			list_del(&tx->list);
>  			rio_handle_tx(rio, tx, !rack->status);
>  			return 0;

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel