[PATCH v2 2/3] nvme-tcp: Handle number of queue changes

[Sagi Grimberg]

On 8/23/22 10:44, Daniel Wagner wrote:
> On reconnect, the number of queues might have changed.
> 
> In the case where we have more queues available than previously we try
> to access queues which are not initialized yet.
> 
> The other case where we have less queues than previously, the
> connection attempt will fail because the target doesn't support the
> old number of queues and we end up in a reconnect loop.
> 
> Thus, only start queues which are currently present in the tagset
> limited by the number of available queues. Then we update the tagset
> and we can start any new queue.
> 
> Signed-off-by: Daniel Wagner <dwagner at suse.de>
> ---
>   drivers/nvme/host/tcp.c | 59 ++++++++++++++++++++++++++---------------
>   1 file changed, 38 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 044da18c06f5..93206215e381 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1762,11 +1762,12 @@ static void nvme_tcp_stop_io_queues(struct nvme_ctrl *ctrl)
>   		nvme_tcp_stop_queue(ctrl, i);
>   }
>   
> -static int nvme_tcp_start_io_queues(struct nvme_ctrl *ctrl)
> +static int nvme_tcp_start_io_queues(struct nvme_ctrl *ctrl,
> +				    int first, int last)
>   {
>   	int i, ret;
>   
> -	for (i = 1; i < ctrl->queue_count; i++) {
> +	for (i = first; i < last; i++) {
>   		ret = nvme_tcp_start_queue(ctrl, i);
>   		if (ret)
>   			goto out_stop_queues;
> @@ -1775,7 +1776,7 @@ static int nvme_tcp_start_io_queues(struct nvme_ctrl *ctrl)
>   	return 0;
>   
>   out_stop_queues:
> -	for (i--; i >= 1; i--)
> +	for (i--; i >= first; i--)
>   		nvme_tcp_stop_queue(ctrl, i);
>   	return ret;
>   }
> @@ -1899,31 +1900,38 @@ static void nvme_tcp_destroy_io_queues(struct nvme_ctrl *ctrl, bool remove)
>   	nvme_tcp_free_io_queues(ctrl);
>   }
>   
> -static int nvme_tcp_configure_io_queues(struct nvme_ctrl *ctrl, bool new)
> +static int nvme_tcp_configure_io_queues(struct nvme_ctrl *nctrl, bool new)

Lets not change the name here, it creates some churn...

>   {
> -	int ret;
> +	struct nvme_tcp_ctrl *ctrl = to_tcp_ctrl(nctrl);

Why do you need the tcp ctrl?

> +	int ret, nr_queues;
>   
> -	ret = nvme_tcp_alloc_io_queues(ctrl);
> +	ret = nvme_tcp_alloc_io_queues(nctrl);
>   	if (ret)
>   		return ret;
>   
>   	if (new) {
> -		ret = nvme_tcp_alloc_tag_set(ctrl);
> +		ret = nvme_tcp_alloc_tag_set(nctrl);
>   		if (ret)
>   			goto out_free_io_queues;
>   
> -		ret = nvme_ctrl_init_connect_q(ctrl);
> +		ret = nvme_ctrl_init_connect_q(nctrl);
>   		if (ret)
>   			goto out_free_tag_set;
>   	}
>   
> -	ret = nvme_tcp_start_io_queues(ctrl);
> +	/*
> +	 * Only start IO queues for which we have allocated the tagset
> +	 * and limitted it to the available queues. On reconnects, the
> +	 * queue number might have changed.
> +	 */
> +	nr_queues = min(ctrl->tag_set.nr_hw_queues + 1, nctrl->queue_count);

	nr_queues = min(ctrl->tagset->nr_hw_queues + 1, ctrl->queue_count);

> +	ret = nvme_tcp_start_io_queues(nctrl, 1, nr_queues);
>   	if (ret)
>   		goto out_cleanup_connect_q;
>   
>   	if (!new) {
> -		nvme_start_queues(ctrl);
> -		if (!nvme_wait_freeze_timeout(ctrl, NVME_IO_TIMEOUT)) {
> +		nvme_start_queues(nctrl);
> +		if (!nvme_wait_freeze_timeout(nctrl, NVME_IO_TIMEOUT)) {
>   			/*
>   			 * If we timed out waiting for freeze we are likely to
>   			 * be stuck.  Fail the controller initialization just
> @@ -1932,26 +1940,35 @@ static int nvme_tcp_configure_io_queues(struct nvme_ctrl *ctrl, bool new)
>   			ret = -ENODEV;
>   			goto out_wait_freeze_timed_out;
>   		}
> -		blk_mq_update_nr_hw_queues(ctrl->tagset,
> -			ctrl->queue_count - 1);
> -		nvme_unfreeze(ctrl);
> +		blk_mq_update_nr_hw_queues(nctrl->tagset,
> +			nctrl->queue_count - 1);
> +		nvme_unfreeze(nctrl);
>   	}
>   
> +	/*
> +	 * If the number of queues has increased (reconnect case)
> +	 * start all new queues now.
> +	 */
> +	ret = nvme_tcp_start_io_queues(nctrl, nr_queues,
> +				       ctrl->tag_set.nr_hw_queues + 1);
> +	if (ret)
> +		goto out_cleanup_connect_q;

You need to go to out_wait_freeze_timed_out as IOs pending have
possibly restarted with nvme_unfreeze...

> +
>   	return 0;
>   
>   out_wait_freeze_timed_out:
> -	nvme_stop_queues(ctrl);
> -	nvme_sync_io_queues(ctrl);
> -	nvme_tcp_stop_io_queues(ctrl);
> +	nvme_stop_queues(nctrl);
> +	nvme_sync_io_queues(nctrl);
> +	nvme_tcp_stop_io_queues(nctrl);
>   out_cleanup_connect_q:
> -	nvme_cancel_tagset(ctrl);
> +	nvme_cancel_tagset(nctrl);
>   	if (new)
> -		blk_mq_destroy_queue(ctrl->connect_q);
> +		blk_mq_destroy_queue(nctrl->connect_q);
>   out_free_tag_set:
>   	if (new)
> -		blk_mq_free_tag_set(ctrl->tagset);
> +		blk_mq_free_tag_set(nctrl->tagset);
>   out_free_io_queues:
> -	nvme_tcp_free_io_queues(ctrl);
> +	nvme_tcp_free_io_queues(nctrl);
>   	return ret;
>   }
>