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

[Meir Elisha]

On 17/02/2025 10:23, Sagi Grimberg wrote:
> 
> 
> 
> On 16/02/2025 17:08, 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>
> 
> Hey Meir, thanks for addressing this.
> 
>> ---
>>   drivers/nvme/target/tcp.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
>> index 7c51c2a8c109..b66aa93baaf4 100644
>> --- a/drivers/nvme/target/tcp.c
>> +++ b/drivers/nvme/target/tcp.c
>> @@ -848,7 +848,8 @@ 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;
>> +    // Ensure rcv_state is visible only after queue->cmd is set
> 
> Nit: let's stick to c-style comments.
> 
>> +    smp_store_release(&queue->rcv_state, NVMET_TCP_RECV_PDU);
> 
> Isn't WRITE_ONCE() enough in this case? We don't really need smp barrier here afaict...
AFAIK, WRITE_ONCE() along will not guarantee ordering of those 2 assignments and not
the visibility across other cpus therefore not solving the issue.