Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v4 2/8] nvme-tcp: Update number of hardware queues before
 using them
To:     Daniel Wagner <dwagner@suse.de>, linux-nvme@lists.infradead.org
Cc:     linux-kernel@vger.kernel.org,
        James Smart <james.smart@broadcom.com>,
        Keith Busch <kbusch@kernel.org>,
        Ming Lei <ming.lei@redhat.com>, Hannes Reinecke <hare@suse.de>,
        Wen Xiong <wenxiong@us.ibm.com>
References: <20210802112658.75875-1-dwagner@suse.de>
 <20210802112658.75875-3-dwagner@suse.de>
From:   Sagi Grimberg <sagi@grimberg.me>
Message-ID: <8373c07f-f5df-1ec6-9fda-d0262fc1b377@grimberg.me>
Date:   Fri, 6 Aug 2021 12:57:17 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210802112658.75875-3-dwagner@suse.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


> 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;
>   
>   out_wait_freeze_timed_out:
>