[PATCH v3] nvmet-tcp: Enforce update ordering between queue->cmd and rcv_state

[Sagi Grimberg]

On 19/02/2025 17:58, Meir Elisha wrote:
> The order in which queue->cmd and rcv_state are updated is crucial.
> If these assignments are reordered by the compiler, the worker might not
> get queued in nvmet_tcp_queue_response(), hanging the IO. to enforce the
> the correct reordering, set rcv_state using smp_store_release().
>
> Signed-off-by: Meir Elisha <meir.elisha at volumez.com>

I think we need a fixes tag, and also rephrase the patch title to 
describe it is
fixing a bug rather than what it is doing.

Something like:
"nvmet-tcp: Fix a possible sporadic response drops in weakly ordered 
architectures"

> ---
> Changes from v2:
> 	- Read queue_state and queue_cmd locally and use load barrier
> 	- Remove unnecessary barrier in nvmet_tcp_handle_h2c_data_pdu
>
> Changes from v1:
> 	- Change comments to c-style
>
>   drivers/nvme/target/tcp.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index 7c51c2a8c109..20fc1a27b720 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -572,9 +572,15 @@ static void nvmet_tcp_queue_response(struct nvmet_req *req)
>   		container_of(req, struct nvmet_tcp_cmd, req);
>   	struct nvmet_tcp_queue	*queue = cmd->queue;
>   	struct nvme_sgl_desc *sgl;
> +	enum nvmet_tcp_queue_state queue_state = READ_ONCE(queue->state);
> +	/*
> +	 * Use an acquire load to ensure that any updates to queue->state are visible
> +	 * before loading queue->cmd.
> +	 */
> +	struct nvmet_tcp_cmd *queue_cmd = smp_load_acquire(&queue->cmd);

Nit, lets move this above the sgl.

>   	u32 len;
>   
> -	if (unlikely(cmd == queue->cmd)) {
> +	if (unlikely(cmd == queue_cmd)) {
>   		sgl = &cmd->req.cmd->common.dptr.sgl;
>   		len = le32_to_cpu(sgl->length);
>   
> @@ -583,7 +589,7 @@ static void nvmet_tcp_queue_response(struct nvmet_req *req)
>   		 * Avoid using helpers, this might happen before
>   		 * nvmet_req_init is completed.
>   		 */
> -		if (queue->rcv_state == NVMET_TCP_RECV_PDU &&
> +		if (queue_state == NVMET_TCP_RECV_PDU &&
>   		    len && len <= cmd->req.port->inline_data_size &&
>   		    nvme_is_write(cmd->req.cmd))
>   			return;
> @@ -847,8 +853,9 @@ static void nvmet_prepare_receive_pdu(struct nvmet_tcp_queue *queue)
>   {
>   	queue->offset = 0;
>   	queue->left = sizeof(struct nvme_tcp_hdr);
> -	queue->cmd = NULL;
> -	queue->rcv_state = NVMET_TCP_RECV_PDU;
> +	WRITE_ONCE(queue->cmd, NULL);
> +	/* Ensure rcv_state is visible only after queue->cmd is set */
> +	smp_store_release(&queue->rcv_state, NVMET_TCP_RECV_PDU);
>   }
>   
>   static void nvmet_tcp_free_crypto(struct nvmet_tcp_queue *queue)