[Devel] [PATCH vz10] fs/fuse kio: fix lost rpc response over rdma io

[Konstantin Khorenko]

> Call ->done() when the peer's RDMA_READ_ACK arrives rather than waiting
> for our own TX completion as well, so that a large message is reported
> as sent before its response can come back. The tx itself is still
> released only once both events are in, as the HCA may still be using
> its MR.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-141673

You have inserted "---" into the commit message, please note, the automation
drops all below first seen "---" at the beginning of the line.

>
> diff --git a/fs/fuse/kio/pcs/pcs_rdma_io.c b/fs/fuse/kio/pcs/pcs_rdma_io.c
> index 1c464e0e60f3e..171e0bf3098bb 100644
> --- a/fs/fuse/kio/pcs/pcs_rdma_io.c
> +++ b/fs/fuse/kio/pcs/pcs_rdma_io.c
> @@ -60,8 +60,9 @@ struct rio_rx {
>  
>  enum {
>  	TX_FREE,                   /* free tx request available for use */
> -	TX_WAIT_FOR_TX_COMPL,      /* tx request sent, wait for TX completion */
> -	TX_WAIT_FOR_READ_ACK,      /* wait for peer to ack RDMA read */
> +	TX_WAIT_FOR_TX_COMPL,      /* wait for TX completion and RDMA read ack */
> +	TX_WAIT_FOR_READ_ACK,      /* TX completed, wait for RDMA read ack */
> +	TX_READ_ACKED,             /* msg completed on ack, wait for TX completion */
>  	TX_MSG_DONE,               /* default: call msg->done() */
>  	TX_SUBMIT_RDMA_READ_ACK,   /* let our peer know that our RDMA_READ is done */
>  };
> @@ -720,10 +721,9 @@ static int rio_submit(struct pcs_rdmaio *rio, struct pcs_msg *msg, int type, u64
>  		 * lose an error.
>  		 *
>  		 * But, if the message is big enough to trigger RDMA READ
> -		 * transfer, we are going to call ->done() callback after we
> -		 * receive RDMA_READ_ACK message from our peer. Since messages
> -		 * in a single RX queue are guaranteed to come in order, there
> -		 * is no race in this case.
> +		 * transfer, ->done() cannot be called here: the peer has not
> +		 * pulled the payload yet. It is called from rio_handle_tx()
> +		 * when RDMA_READ_ACK arrives, see the comment there.
>  		 */
>  		rio_msg_sent(rio, tx, msg, rb == NULL);
>  	}
> @@ -795,7 +795,21 @@ static inline void rio_kick_write_queue(struct pcs_rdmaio *rio)
>  	rio_enable_kick(rio);
>  }
>  
> -static void rio_handle_tx(struct pcs_rdmaio *rio, struct rio_tx *tx, int ok)
> +/* Events a tx waits for, see rio_handle_tx() */
> +enum rio_tx_event {
> +	TX_EV_SEND_CQE,            /* TX completion from our HCA */
> +	TX_EV_READ_ACK,            /* RDMA_READ_ACK from peer */
> +};
> +
> +/*
> + * Handle one of the two events @tx waits for. They are not ordered: the peer
> + * may ack the RDMA read, and send its response, before our HCA reports the
> + * send. Hence msg->done() is called on TX_EV_READ_ACK, which the response
> + * cannot overtake as both come from the same RX queue, while the tx is
> + * released only after both events, as the HCA may still be using its MR.
> + */
> +static void rio_handle_tx(struct pcs_rdmaio *rio, struct rio_tx *tx, int ok,
> +			  enum rio_tx_event ev)
>  {
>  	struct pcs_msg *msg = tx->msg;
>  	u64 xid = tx->xid;
> @@ -814,9 +828,25 @@ static void rio_handle_tx(struct pcs_rdmaio *rio, struct rio_tx *tx, int ok)
>  			rio->n_rdma_read_ongoing--;
>  			break;
>  		case TX_WAIT_FOR_TX_COMPL:
> -		case TX_WAIT_FOR_READ_ACK:
> -			if (++tx->tx_state != TX_MSG_DONE)
> +			if (ev == TX_EV_SEND_CQE) {
> +				/* Still waiting for the peer to ack RDMA read */
> +				tx->tx_state = TX_WAIT_FOR_READ_ACK;
>  				return;
> +			}
> +			/*
> +			 * The peer is done with the payload, complete msg now
> +			 * and keep the tx until our own TX completion arrives.
> +			 * The tx must keep no reference to msg, which may be
> +			 * freed by the completion below.
> +			 */
> +			tx->tx_state = TX_READ_ACKED;
> +			tx->msg = NULL;
> +			pcs_rdma_msg_detach(&tx->send.msg);
> +			break;
> +		case TX_WAIT_FOR_READ_ACK:
> +			rio_put_tx(rio->dev, tx);
> +			break;
> +		case TX_READ_ACKED:
>  			rio_put_tx(rio->dev, tx);
>  			break;
>  		case TX_MSG_DONE:

3 identical cases here, may be to merged them?

i will apply the patch as is but think about it and rework if you consider that.

-- 
Konstantin Khorenko <khorenko at virtuozzo.com>