Received-SPF: pass (google.com: domain of linux-kernel+bounces-32605-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <189cde89-9750-476f-8fbb-1c95dc056efb@grimberg.me>
Date: Mon, 22 Jan 2024 11:13:15 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] nvme_core: scan namespaces asynchronously
Content-Language: en-US
To: Stuart Hayes <stuart.w.hayes@gmail.com>, linux-kernel@vger.kernel.org,
 Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@kernel.dk>,
 Christoph Hellwig <hch@lst.de>, linux-nvme@lists.infradead.org
References: <20240118210303.10484-1-stuart.w.hayes@gmail.com>
From: Sagi Grimberg <sagi@grimberg.me>
In-Reply-To: <20240118210303.10484-1-stuart.w.hayes@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 1/18/24 23:03, Stuart Hayes wrote:
> Use async function calls to make namespace scanning happen in parallel.
> 
> Without the patch, NVME namespaces are scanned serially, so it can take a
> long time for all of a controller's namespaces to become available,
> especially with a slower (TCP) interface with large number of namespaces.
> 
> The time it took for all namespaces to show up after connecting (via TCP)
> to a controller with 1002 namespaces was measured:
> 
> network latency   without patch   with patch
>       0                 6s            1s
>      50ms             210s           10s
>     100ms             417s           18s
> 

Impressive speedup. Not a very common use-case though...

> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> 
> --
> V2: remove module param to enable/disable async scanning
>      add scan time measurements to commit message
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 0af612387083..069350f85b83 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4,6 +4,7 @@
>    * Copyright (c) 2011-2014, Intel Corporation.
>    */
>   
> +#include <linux/async.h>
>   #include <linux/blkdev.h>
>   #include <linux/blk-mq.h>
>   #include <linux/blk-integrity.h>
> @@ -3812,12 +3813,38 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_info *info)
>   		nvme_ns_remove(ns);
>   }
>   
> -static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> +/*
> + * struct nvme_scan_state - keeps track of controller & NSIDs to scan
> + * @ctrl:	Controller on which namespaces are being scanned
> + * @count:	Next NSID to scan (for sequential scan), or
> + *		Index of next NSID to scan in ns_list (for list scan)
> + * @ns_list:	pointer to list of NSIDs to scan (NULL if sequential scan)
> + */
> +struct nvme_scan_state {
> +	struct nvme_ctrl *ctrl;
> +	atomic_t count;
> +	__le32 *ns_list;
> +};
> +
> +static void nvme_scan_ns(void *data, async_cookie_t cookie)

I think its better to call it nvme_scan_ns_async to indicate what
it is.

>   {
> -	struct nvme_ns_info info = { .nsid = nsid };
> +	struct nvme_ns_info info = {};
> +	struct nvme_scan_state *scan_state;
> +	struct nvme_ctrl *ctrl;
> +	u32 nsid;
>   	struct nvme_ns *ns;
>   	int ret;
>   
> +	scan_state = data;
> +	ctrl = scan_state->ctrl;

I think these assignments can be done on the declaration.

> +	nsid = (u32)atomic_fetch_add(1, &scan_state->count);
> +	/*
> +	 * get NSID from list (if scanning from a list, not sequentially)
> +	 */
> +	if (scan_state->ns_list)
> +		nsid = le32_to_cpu(scan_state->ns_list[nsid]);
> +

This is awkward. ns_list passed in optionally.
How about we limit this change to only operate on nvme_scan_ns_list?
If the controller is old or quirked to support only a sequential scan
it does not benefit from a parallel scan. I doubt that these controllers
are likely to expose a large number of namespaces anyways.

> +	info.nsid = nsid;
>   	if (nvme_identify_ns_descs(ctrl, &info))
>   		return;
>   
> @@ -3881,11 +3908,15 @@ static int nvme_scan_ns_list(struct nvme_ctrl *ctrl)
>   	__le32 *ns_list;
>   	u32 prev = 0;
>   	int ret = 0, i;
> +	ASYNC_DOMAIN(domain);
> +	struct nvme_scan_state scan_state;
>   
>   	ns_list = kzalloc(NVME_IDENTIFY_DATA_SIZE, GFP_KERNEL);
>   	if (!ns_list)
>   		return -ENOMEM;
>   
> +	scan_state.ctrl = ctrl;
> +	scan_state.ns_list = ns_list;

Is there a need to have a local ns_list variable here?

>   	for (;;) {
>   		struct nvme_command cmd = {
>   			.identify.opcode	= nvme_admin_identify,
> @@ -3901,19 +3932,25 @@ static int nvme_scan_ns_list(struct nvme_ctrl *ctrl)
>   			goto free;
>   		}
>   
> +		/*
> +		 * scan list starting at list offset 0
> +		 */
> +		atomic_set(&scan_state.count, 0);
>   		for (i = 0; i < nr_entries; i++) {
>   			u32 nsid = le32_to_cpu(ns_list[i]);
>   
>   			if (!nsid)	/* end of the list? */
>   				goto out;
> -			nvme_scan_ns(ctrl, nsid);
> +			async_schedule_domain(nvme_scan_ns, &scan_state, &domain);
>   			while (++prev < nsid)
>   				nvme_ns_remove_by_nsid(ctrl, prev);
>   		}
> +		async_synchronize_full_domain(&domain);
>   	}
>    out:
>   	nvme_remove_invalid_namespaces(ctrl, prev);

Is it a good idea to remove the invalid namespaces before synchronizing
the async scans?

>    free:
> +	async_synchronize_full_domain(&domain);
>   	kfree(ns_list);
>   	return ret;
>   }
> @@ -3922,14 +3959,23 @@ static void nvme_scan_ns_sequential(struct nvme_ctrl *ctrl)
>   {
>   	struct nvme_id_ctrl *id;
>   	u32 nn, i;
> +	ASYNC_DOMAIN(domain);
> +	struct nvme_scan_state scan_state;
>   
>   	if (nvme_identify_ctrl(ctrl, &id))
>   		return;
>   	nn = le32_to_cpu(id->nn);
>   	kfree(id);
>   
> +	scan_state.ctrl = ctrl;
> +	/*
> +	 * scan sequentially starting at NSID 1
> +	 */
> +	atomic_set(&scan_state.count, 1);
> +	scan_state.ns_list = NULL;
>   	for (i = 1; i <= nn; i++)
> -		nvme_scan_ns(ctrl, i);
> +		async_schedule_domain(nvme_scan_ns, &scan_state, &domain);
> +	async_synchronize_full_domain(&domain);
>   
>   	nvme_remove_invalid_namespaces(ctrl, nn);
>   }

I think we need a blktest for this. ns scanning has been notorious when
running simultaneously with controller reset/reconnect/remove
sequences... Ideally a test with a larger number of namespaces to
exercise the code.

Also, make sure that blktest suite does not complain about anything
else.