Re: [PATCH v2] nvme_core: scan namespaces asynchronously

[stuart hayes]

On 1/23/2024 2:21 PM, Chaitanya Kulkarni wrote:
> On 1/23/2024 8:37 AM, Keith Busch wrote:
> > On Mon, Jan 22, 2024 at 11:13:15AM +0200, Sagi Grimberg wrote:
> > > On 1/18/24 23:03, Stuart Hayes wrote:
> > > > @@ -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);
> >
> > You mentioned async scanning was an improvement if you have 1000
> > namespaces, but wouldn't this be worse if you have very few namespaces?
> > IOW, the decision to use the async schedule should be based on
> > nr_entries, right?
>
> Perhaps it's also helpful to documents the data for small number of
> namespaces, we can think of collecting data something like this:-
>
> NR Namespaces        Seq Scan        Async Scan
> 2
> 4
> 8
> 16
> 32
> 64
> 128
> 256
> 512
> 1024
>
> If we find that difference is not that much then we can go ahead with
> this patch, if it the difference is not acceptable to the point that it
> will regress for common setups then we can make it configurable ?
>
> -ck
I believe the only reason the async scanning should take any longer than
sync scanning is that nvme_scan_ns() has to wait on the workqueue until it
is scheduled.

Testing on my system (with pcie nvme devices with a single namespace), it
looks like it only takes a fraction of a ms (100us or less typically) for
that to happen.  Then it takes 6-10ms or more for the actual namesapce scan.

So scanning asynchronously, even using a local pcie device with a single
namespace, doesn't take significantly longer.  Of course I guess it might
take a bit longer on a busy system, but I wouldn't think that scanning
namespaces is a critical path where a few milliseconds would make much
difference (?).  It wouldn't be too hard to make it scan synchronously if
there aren't more than, say, a couple namespaces, but my opinion is that
the minimal benefit wouldn't be worth the extra code.