[PATCH] nvme-tcp: Fix possible race of io_work and direct send

[Yi Zhang]

On 1/9/21 4:20 AM, Sagi Grimberg wrote:
>
>>> --- a/drivers/nvme/host/tcp.c
>>> +++ b/drivers/nvme/host/tcp.c
>>
>>> @@ -279,7 +289,7 @@ static inline void nvme_tcp_queue_request(struct 
>>> nvme_tcp_request *req,
>>>          if (queue->io_cpu == smp_processor_id() &&
>>>              sync && empty && mutex_trylock(&queue->send_mutex)) {
>>
>> but we can't rely on the processor id if we are on preemptible state..
>>
>>    373.940336] BUG: using smp_processor_id() in preemptible [00000000]
>> code: kworker/u8:4/457
>> [  373.942037] caller is nvme_tcp_submit_async_event+0x4da/0x690 
>> [nvme_tcp]
>> [  373.942144] CPU: 0 PID: 457 Comm: kworker/u8:4 Not tainted 
>> 5.11.0-rc1+ #116
>> [  373.942171] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>> BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
>> [  373.942196] Workqueue: nvme-wq nvme_async_event_work [nvme_core]
>> [  373.942259] Call Trace:
>> [  373.942286]  dump_stack+0xd0/0x119
>> [  373.942346]  check_preemption_disabled+0xec/0xf0
>> [  373.942397]  nvme_tcp_submit_async_event+0x4da/0x690 [nvme_tcp]
>> [  373.942449]  nvme_async_event_work+0x11d/0x1f0 [nvme_core]
>> [  373.942496]  ? nvme_ctrl_loss_tmo_show+0x140/0x140 [nvme_core]
>> [  373.942576]  process_one_work+0x9f3/0x1630
>> [  373.942649]  ? pwq_dec_nr_in_flight+0x330/0x330
>> [  373.942765]  worker_thread+0x9e/0xf60
>> [  373.942849]  ? process_one_work+0x1630/0x1630
>> [  373.942889]  kthread+0x362/0x480
>> [  373.942918]  ? kthread_create_on_node+0x100/0x100
>
> Well, this is a heuristic anyways, too bad we need to
> disable preemption just for this...
>
> I'm assuming that this makes the issue go away?
> -- 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index cf856103025a..ea07336ca179 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -281,8 +281,11 @@ static inline void nvme_tcp_queue_request(struct 
> nvme_tcp_request *req,
>                 bool sync, bool last)
>  {
>         struct nvme_tcp_queue *queue = req->queue;
> +       int cpu;
>         bool empty;
>
> +       cpu = get_cpu();
> +       put_cpu();
>         empty = llist_add(&req->lentry, &queue->req_list) &&
>                 list_empty(&queue->send_list) && !queue->request;
>
> @@ -291,8 +294,8 @@ static inline void nvme_tcp_queue_request(struct 
> nvme_tcp_request *req,
>          * directly, otherwise queue io_work. Also, only do that if we
>          * are on the same cpu, so we don't introduce contention.
>          */
> -       if (queue->io_cpu == smp_processor_id() &&
> -           sync && empty && mutex_trylock(&queue->send_mutex)) {
> +       if (queue->io_cpu == cpu && sync && empty &&
> +           mutex_trylock(&queue->send_mutex)) {
>                 queue->more_requests = !last;
>                 nvme_tcp_send_all(queue);
>                 queue->more_requests = false;
>
Hi Sagi
Thanks for the quick response, confirmed the issue was fixed, feel free 
to add:

Tested-by: Yi Zhang <yi.zhang at redhat.com>

> -- 
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
>