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

Sagi Grimberg
2021-08-06T19:57:39Z

> From: Hannes Reinecke <hare@suse.de>
>
> When the number of hardware queues changes during resetting we should
> update the tagset first before using it.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
> drivers/nvme/host/tcp.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 0a97ba02f61e..32268f24f62a 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1789,6 +1789,7 @@ static void nvme_tcp_destroy_io_queues(struct nvme_ctrl *ctrl, bool remove)
> static int nvme_tcp_configure_io_queues(struct nvme_ctrl *ctrl, bool new)
> {
> int ret;
> + u32 prior_q_cnt = ctrl->queue_count;
>
> ret = nvme_tcp_alloc_io_queues(ctrl);
> if (ret)
> @@ -1806,14 +1807,7 @@ static int nvme_tcp_configure_io_queues(struct nvme_ctrl *ctrl, bool new)
> ret = PTR_ERR(ctrl->connect_q);
> goto out_free_tag_set;
> }
> - }
> -
> - 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?

> 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?

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.

> return 0;
>