linux-kernel - Re: [PATCH v2] nvme_core: scan namespaces asynchronously

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 24 Jan 2024 13:32:47 -0600
From: stuart hayes <stuart.w.hayes@...il.com>
To: Chaitanya Kulkarni <chaitanyak@...dia.com>,
 Keith Busch <kbusch@...nel.org>, Sagi Grimberg <sagi@...mberg.me>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 Jens Axboe <axboe@...nel.dk>, Christoph Hellwig <hch@....de>,
 "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>
Subject: Re: [PATCH v2] nvme_core: scan namespaces asynchronously



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.