Re: [PATCH v4 2/8] nvme-tcp: Update number of hardware queues before using them

Sagi Grimberg
2021-08-11T01:01:19Z

On 8/9/21 1:52 AM, Daniel Wagner wrote:
> Hi Sagi,
>
> On Fri, Aug 06, 2021 at 12:57:17PM -0700, Sagi Grimberg wrote:
>>> - ret = nvme_tcp_start_io_queues(ctrl);
>>> - if (ret)
>>> - goto out_cleanup_connect_q;
>>> -
>>> - if (!new) {
>>> - nvme_start_queues(ctrl);
>>> + } else if (prior_q_cnt != ctrl->queue_count) {
>>
>> So if the queue count did not change we don't wait to make sure
>> the queue g_usage_counter ref made it to zero? What guarantees that it
>> did?
>
> Hmm, good point. we should always call nvme_wait_freeze_timeout()
> for !new queues. Is this what you are implying?

I think we should always wait for the freeze to complete.

>
>
>>> if (!nvme_wait_freeze_timeout(ctrl, NVME_IO_TIMEOUT)) {
>>> /*
>>> * If we timed out waiting for freeze we are likely to
>>> @@ -1828,6 +1822,10 @@ static int nvme_tcp_configure_io_queues(struct nvme_ctrl *ctrl, bool new)
>>> nvme_unfreeze(ctrl);
>>> }
>>> + ret = nvme_tcp_start_io_queues(ctrl);
>>> + if (ret)
>>> + goto out_cleanup_connect_q;
>>> +
>>
>> Did you test this with both heavy I/O, reset loop and ifdown/ifup
>> loop?
>
> Not sure if this classifies as heavy I/O (on 80 CPU machine)
>
> fio --rw=readwrite --name=test --filename=/dev/nvme16n1 --size=50M \
> --direct=1 --bs=4k --numjobs=40 --group_reporting --runtime=4h \
> --time_based
>
> and then I installed iptables rules to block the traffic on the
> controller side. With this test it is pretty easily to get
> the host hanging. Let me know what test you would like to see
> from me. I am glad to try to get them running.

Lets add iodepth=128

>> If we unquiesce and unfreeze before we start the queues the pending I/Os
>> may resume before the connect and not allow the connect to make forward
>> progress.
>
> So the unfreeze should happen after the connect call? What about the
> newly created queues? Do they not suffer from the same problem? Isn't
> the NVME_TCP_Q_LIVE flag not enough?

Q_LIVE will protect against the transport itself from queueing, however
when multipath is not used, the transport will return BLK_STS_RESOURCE
which will immediately trigger re-submission, in an endless loop, and
that can prevent forward progress. It is also consistent with what
nvme-pci does.