Skip to contentSkip to conversation

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

PATCH New Sam Bradshaw 3 participants 4 messages Nov 5, 2014 – Nov 17, 2014

Subsystems (2)
Block layer NVMe
lore
Share & tools
lore

Sam Bradshaw

author
#0 lore raw

PATCH 1/1 NVMe: Defer namespace add_disk() until after char device creation

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>

---

drivers/block/nvme-core.c +13 -2
index 00fa5d2..1d14378 100644
@@ -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:
maintainer
#1 lore raw

PATCH 1/1 NVMe: Defer namespace add_disk() until after char device creation

Quoted 6 lines · 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.

Quoted 7 lines

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>
Signature · 1 line
Jens Axboe
#2 lore raw

PATCH 1/1 NVMe: Defer namespace add_disk() until after char device creation

Quoted 10 lines · Jens Axboe
Quoted 9 lines · 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.

Quoted 8 lines
Quoted 4 lines

(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.

maintainer
#3 lore raw

PATCH 1/1 NVMe: Defer namespace add_disk() until after char device creation

Quoted 16 lines · Keith Busch
Quoted 15 lines · Jens Axboe
Quoted 13 lines · 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)

Quoted 8 lines
Quoted 3 lines

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.

Signature · 1 line
Jens Axboe