nvme: do not block connect call on inaccessible devices

PATCH New Hannes Reinecke 3 participants 6 messages Jun 5, 2018 NVMe

Share & tools

Hannes Reinecke

author

2018-06-05 08:29 UTC #0 lore raw

PATCH 1/1 nvme: do not block connect call on inaccessible devices

When a device is in ANA inaccessible state the call to 'nvme connect' will hang as a parition scan is triggered and the I/O will be requeued indefinitely.
Or rather, requeued until the next path is connected, but we cannot issue any nvme ioctls anymore as the nvmf_dev_mutex is blocked by the initial 'connect' call.

This patch inhibits partition scan if no working paths are found, and retriggers the partition scan once the device becomes active.

Signed-off-by Hannes Reinecke <hare at suse.com>

---
 drivers/nvme/host/multipath.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

drivers/nvme/host/multipath.c +28 -0

index 14bcc51016c7..30ba3c43e541 100644
@@ -275,16 +275,44 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
 
 void nvme_mpath_add_disk(struct nvme_ns_head *head)
 {
+	struct nvme_ns *ns;
+	int nr_working = 0;
+
 	if (!head->disk)
 		return;
 
 	mutex_lock(&head->subsys->lock);
+	list_for_each_entry(ns, &head->list, siblings) {
+		if (nvme_ns_ana_state(ns) == NVME_ANA_OPTIMIZED ||
+		    nvme_ns_ana_state(ns) == NVME_ANA_NONOPTIMIZED)
+			nr_working++;
+	}
+	/* Disable partition scan if no working paths are found */
+	if (!nr_working)
+		head->disk->flags |= GENHD_FL_NO_PART_SCAN;
+	else
+		head->disk->flags &= ~GENHD_FL_NO_PART_SCAN;
+
 	if (!(head->disk->flags & GENHD_FL_UP)) {
 		device_add_disk(&head->subsys->dev, head->disk);
 		if (sysfs_create_group(&disk_to_dev(head->disk)->kobj,
 				&nvme_ns_id_attr_group))
 			pr_warn("%s: failed to create sysfs group for identification\n",
 				head->disk->disk_name);
+	} else {
+		struct block_device *bdev;
+
+		/*
+		 * We need to do an explicit blkdev_get() to
+		 * re-trigger partition scan if new paths have been added.
+		 */
+		bdev = bdget_disk(head->disk, 0);
+		if (bdev) {
+			bdev->bd_invalidated = 1;
+			if (!blkdev_get(bdev, FMODE_READ, NULL))
+				blkdev_put(bdev, FMODE_READ);
+			bdput(bdev);
+		}
 	}
 	mutex_unlock(&head->subsys->lock);
 }

Signature · 1 line

2.12.3

Christoph Hellwig

maintainer

2018-06-05 11:17 UTC #1 lore raw

PATCH 1/1 nvme: do not block connect call on inaccessible devices

Quoted 9 lines · Hannes Reinecke

When a device is in ANA inaccessible state the call to 'nvme connect' will hang as a parition scan is triggered and the I/O will be requeued indefinitely.
Or rather, requeued until the next path is connected, but we cannot issue any nvme ioctls anymore as the nvmf_dev_mutex is blocked by the initial 'connect' call.
This patch inhibits partition scan if no working paths are found, and retriggers the partition scan once the device becomes active.

And what updates the state once all your inaccessible or change state groups become optimized?

Also I see no point registering the disk if we can't do I/O to it.

I'd rather prefer something along the lines of the completely untested patch below if we really want to go down this route:

drivers/nvme/host/core.c +1 -1

index 3b55580bb9be..5d90560775e1 100644
@@ -3080,7 +3080,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 		pr_warn("%s: failed to register lightnvm sysfs group for identification\n",
 			ns->disk->disk_name);
 
-	nvme_mpath_add_disk(ns->head);
+	nvme_mpath_add_disk(ns);
 	nvme_fault_inject_init(ns);
 	return;
  out_unlink_ns:

drivers/nvme/host/multipath.c +42 ~~-20~~

108 lines not on this page · raw message

drivers/nvme/host/nvme.h +2 -2

index a6541b9f1548..ccbeae09599d 100644
@@ -460,7 +460,7 @@ void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
 void nvme_failover_req(struct request *req);
 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl);
 int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl,struct nvme_ns_head *head);
-void nvme_mpath_add_disk(struct nvme_ns_head *head);
+void nvme_mpath_add_disk(struct nvme_ns *ns);
 void nvme_mpath_remove_disk(struct nvme_ns_head *head);
 int nvme_configure_ana(struct nvme_ctrl *ctrl);
 void nvme_deconfigure_ana(struct nvme_ctrl *ctrl);

@@ -507,7 +507,7 @@ static inline int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl,
 {
 	return 0;
 }
-static inline void nvme_mpath_add_disk(struct nvme_ns_head *head)
+static inline void nvme_mpath_add_disk(struct nvme_ns *ns)
 {
 }
 static inline void nvme_mpath_remove_disk(struct nvme_ns_head *head)

Hannes Reinecke

author

2018-06-05 12:23 UTC #2 lore raw

PATCH 1/1 nvme: do not block connect call on inaccessible devices

Quoted 8 lines · On Tue, 5 Jun 2018 13:17:46 +0200 Christoph Hellwig <[email protected]> wrote:

Quoted 7 lines · Hannes Reinecke
When a device is in ANA inaccessible state the call to 'nvme connect' will hang as a parition scan is triggered and the I/O will be requeued indefinitely.
Or rather, requeued until the next path is connected, but we cannot issue any nvme ioctls anymore as the nvmf_dev_mutex is blocked by the initial 'connect' call.

Quoted 6 lines

Quoted 2 lines
This patch inhibits partition scan if no working paths are found, and retriggers the partition scan once the device becomes active.
And what updates the state once all your inaccessible or change state groups become optimized?

ANA AENs?
Someone told me we should rely on ANA AENS or the system is hosed anyway :-)

My idea was to hook into the 'revalidate_disk' call here, but then this approach turned out to be easier.

Quoted 5 lines

Also I see no point registering the disk if we can't do I/O to it.
I'd rather prefer something along the lines of the completely untested patch below if we really want to go down this route:

Okay, I'll give it a go.

Cheers,

Hannes

Bart Van Assche

2018-06-05 12:35 UTC #3 lore raw

PATCH 1/1 nvme: do not block connect call on inaccessible devices

Quoted 8 lines · Hannes Reinecke

Quoted 2 lines · On Tue, 5 Jun 2018 13:17:46 +0200 Christoph Hellwig <[email protected]> wrote:
And what updates the state once all your inaccessible or change state groups become optimized?
ANA AENs?
Someone told me we should rely on ANA AENS or the system is hosed anyway :-)

Is there a standard for AENs over NVMeOF? If so, how many receive buffers should the initiator reserve for AENs? Which credit mechanism should be used for AEN communication?

Thanks,

Bart.

[email protected]

maintainer

2018-06-05 12:50 UTC #4 lore raw

PATCH 1/1 nvme: do not block connect call on inaccessible devices

Quoted 3 lines · Bart Van Assche

Is there a standard for AENs over NVMeOF? If so, how many receive buffers should the initiator reserve for AENs? Which credit mechanism should be used for AEN communication?

Take a look at sections 5.2 and 7.7 of NVMe 1.3. All that text actually goes back to 1.0 without major change.

Christoph Hellwig

maintainer

2018-06-05 12:51 UTC #5 lore raw

PATCH 1/1 nvme: do not block connect call on inaccessible devices

In reply to Hannes Reinecke

Quoted 7 lines · Hannes Reinecke

Quoted 6 lines
Quoted 2 lines
This patch inhibits partition scan if no working paths are found, and retriggers the partition scan once the device becomes active.
And what updates the state once all your inaccessible or change state groups become optimized?
ANA AENs?

Not in your patch.

Quoted 2 lines

Someone told me we should rely on ANA AENS or the system is hosed anyway :-)

Yes, of course. But AENs aren;t going to help you if you don't take any action upon them.