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

[Konstantin Khorenko <khorenko@virtuozzo.com>]

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