[PATCH] NVMe: Defer namespace add_disk() until after char device creation

[PATCH] NVMe: Defer namespace add_disk() until after char device creation

[Sam Bradshaw]

In the current probe flow, each namespace gets an add_disk() then the 
char device for the controller is registered.  For misbehaving devices 
or namespace(s) that are not yet ready when add_disk() is called (eg. 
namespace accesses that return NVME_SC_NS_NOT_READY and are requeued), 
it can take time to disposition all the accesses.

This change moves add_disk() after the char device is created to give 
manageability stacks an interface to query as IO flushes out.

(I also considered deferring the nvme_ns_add() to an async context but
that requires some sort of mutex between probe and remove to handle the
surprise remove during add_disk condition and was messier than this 
patch)

Signed-off-by: Sam Bradshaw <sbradshaw at micron.com>
---
diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 00fa5d2..1d14378 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -2336,6 +2336,16 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	return result;
 }
 
+static int nvme_ns_add(struct nvme_dev *dev)
+{
+	struct nvme_ns *ns;
+
+	list_for_each_entry(ns, &dev->namespaces, list)
+		add_disk(ns->disk);
+
+	return 0;
+}
+
 /*
  * Return: error value if an error occurred setting up the queues or calling
  * Identify Device.  0 if these succeeded, even if adding some of the
@@ -2398,8 +2408,6 @@ static int nvme_dev_add(struct nvme_dev *dev)
 		if (ns)
 			list_add_tail(&ns->list, &dev->namespaces);
 	}
-	list_for_each_entry(ns, &dev->namespaces, list)
-		add_disk(ns->disk);
 	res = 0;
 
  out:
@@ -2941,10 +2949,13 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (result)
 		goto remove;
 
+	nvme_ns_add(dev);
+
 	dev->initialized = 1;
 	return 0;
 
  remove:
+	INIT_LIST_HEAD(&dev->namespaces);
 	nvme_dev_remove(dev);
 	nvme_free_namespaces(dev);
  shutdown:

[PATCH] NVMe: Defer namespace add_disk() until after char device creation

[Jens Axboe]

On 2014-11-04 17:18, Sam Bradshaw wrote:
> In the current probe flow, each namespace gets an add_disk() then the
> char device for the controller is registered.  For misbehaving devices
> or namespace(s) that are not yet ready when add_disk() is called (eg.
> namespace accesses that return NVME_SC_NS_NOT_READY and are requeued),
> it can take time to disposition all the accesses.
>
> This change moves add_disk() after the char device is created to give
> manageability stacks an interface to query as IO flushes out.
>
> (I also considered deferring the nvme_ns_add() to an async context but
> that requires some sort of mutex between probe and remove to handle the
> surprise remove during add_disk condition and was messier than this
> patch)

I'd love to see this get added. You need the control char dev to fixup 
such bad situations, and it may never get created if we get stuck on 
trying to setup and probe the block parts.

Acked-by: Jens Axboe <axboe at fb.com>

-- 
Jens Axboe

[PATCH] NVMe: Defer namespace add_disk() until after char device creation

[Keith Busch]

On Fri, 14 Nov 2014, Jens Axboe wrote:
> On 2014-11-04 17:18, Sam Bradshaw wrote:
>> In the current probe flow, each namespace gets an add_disk() then the
>> char device for the controller is registered.  For misbehaving devices
>> or namespace(s) that are not yet ready when add_disk() is called (eg.
>> namespace accesses that return NVME_SC_NS_NOT_READY and are requeued),
>> it can take time to disposition all the accesses.
>> 
>> This change moves add_disk() after the char device is created to give
>> manageability stacks an interface to query as IO flushes out.
>> 
>> (I also considered deferring the nvme_ns_add() to an async context but
>> that requires some sort of mutex between probe and remove to handle the
>> surprise remove during add_disk condition and was messier than this
>> patch)
>
> I'd love to see this get added. You need the control char dev to fixup such 
> bad situations, and it may never get created if we get stuck on trying to 
> setup and probe the block parts.

Would it be much trouble to make "add_disk" non-blocking? If it's not
too bad, then all storage drivers might benefit from that and we don't
have to do things like this in a driver. I haven't really looked into it,
so asking out of curiosity and not a serious suggestion yet.

[PATCH] NVMe: Defer namespace add_disk() until after char device creation

[Jens Axboe]

On 11/17/2014 08:19 AM, Keith Busch wrote:
> On Fri, 14 Nov 2014, Jens Axboe wrote:
>> On 2014-11-04 17:18, Sam Bradshaw wrote:
>>> In the current probe flow, each namespace gets an add_disk() then the
>>> char device for the controller is registered.  For misbehaving devices
>>> or namespace(s) that are not yet ready when add_disk() is called (eg.
>>> namespace accesses that return NVME_SC_NS_NOT_READY and are requeued),
>>> it can take time to disposition all the accesses.
>>>
>>> This change moves add_disk() after the char device is created to give
>>> manageability stacks an interface to query as IO flushes out.
>>>
>>> (I also considered deferring the nvme_ns_add() to an async context but
>>> that requires some sort of mutex between probe and remove to handle the
>>> surprise remove during add_disk condition and was messier than this
>>> patch)
>>
>> I'd love to see this get added. You need the control char dev to fixup
>> such bad situations, and it may never get created if we get stuck on
>> trying to setup and probe the block parts.
>
> Would it be much trouble to make "add_disk" non-blocking? If it's not
> too bad, then all storage drivers might benefit from that and we don't
> have to do things like this in a driver. I haven't really looked into it,
> so asking out of curiosity and not a serious suggestion yet.

It would certainly be possible, and could potentially speedup device 
probing and creation if lots of devices are present. It's also one of 
those things where the devil is in the details, on ensuring we don't 
introduce removal races and userspace issues. See the mess on the SCSI 
async scanning, for instance...

For now, I'd prefer if we just created the char device first, it's the 
sensible thing to do imho. Making add_disk async is definitely doable, 
and I would not mind doing it, it's just a lot more work.

-- 
Jens Axboe