[PATCH] nvmet-tcp: avoid circular locking dependency on install_queue()

[Hannes Reinecke]

On 8/17/23 13:20, Sagi Grimberg wrote:
> 
>>> On 8/10/23 16:20, Hannes Reinecke wrote:
>>>> nvmet_tcp_install_queue() is driven from the ->io_work workqueue
>>>> function, but will call flush_workqueue() which might trigger
>>>> ->release_work() which in itself calls flush_work on ->io_work.
>>>>
>>>> To avoid that check for pending queue in disconnecting status,
>>>> and return 'controller busy' until all disconnects are completed.
>>>>
>>>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>>>> ---
>>>>   drivers/nvme/target/tcp.c | 14 ++++++++++++--
>>>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
>>>> index f131b504aa01..97d07488072d 100644
>>>> --- a/drivers/nvme/target/tcp.c
>>>> +++ b/drivers/nvme/target/tcp.c
>>>> @@ -1845,8 +1845,18 @@ static u16 nvmet_tcp_install_queue(struct 
>>>> nvmet_sq *sq)
>>>>           container_of(sq, struct nvmet_tcp_queue, nvme_sq);
>>>>       if (sq->qid == 0) {
>>>> -        /* Let inflight controller teardown complete */
>>>> -        flush_workqueue(nvmet_wq);
>>>> +        struct nvmet_tcp_queue *q;
>>>> +        int pending = 0;
>>>> +
>>>> +        mutex_lock(&nvmet_tcp_queue_mutex);
>>>> +        list_for_each_entry(q, &nvmet_tcp_queue_list, queue_list) {
>>>> +            if (q->state == NVMET_TCP_Q_DISCONNECTING)
>>>> +                pending++;
>>>> +        }
>>>> +        mutex_unlock(&nvmet_tcp_queue_mutex);
>>>> +        /* Retry for pending controller teardown */
>>>> +        if (pending)
>>>> +            return NVME_SC_CONNECT_CTRL_BUSY;
>>>
>>> Overall I'm fine with the approach.
>>>
>>> That will most likely be the case for a controller reset or a "reset
>>> storm". Maybe it is better to keep it at some reasonably allowed pending
>>> to say 10 or something... The whole point is to not let the removals
>>> accumulate and continue creating new ones...
>>>
>>> Can you please test this with a host that does a loop like:
>>> while true; do nvme reset /dev/nvme0; done
>>>
>>> And let it run for some time...
>>> Or even better create a test that does this for 100 iterations
>>> or a log/short variants of this?
>>>
>>> The same would be needed by nvmet-rdma btw.
>>
>> Retested with the reset loop; no issues found after 327 iterations.
> 
> Interesting, did the host see a NVME_SC_CONNECT_CTRL_BUSY  during the
> test?

Nope.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare at suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman