NVMe: IO ending fixes on surprise removal

PATCH New Keith Busch 3 participants 5 messages Dec 11, 2015 – Dec 18, 2015 NVMe

lore

Share & tools

lore

Keith Busch

author

2015-12-11 20:14 UTC #0 lore raw

PATCH 1/1 NVMe: IO ending fixes on surprise removal

This patch fixes a lost request discovered during IO + hot removal.

The driver's pci removal deletes gendisks prior to shutting down the controller to allow dirty data to sync. Dirty data can not be synced on a surprise removal, though, and would potentially block indefinitely.

The driver previously had marked the queue as dying in this scenario to prevent new requests from attempting, however it will still block for requests that already entered the queue. This patch fixes this by quiescing IO first, then aborting the requeued requests before deleting disks.

Reported-by Sujith Pandel <sujith_pandel at dell.com>

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/pci.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

drivers/nvme/host/pci.c +19 -1

index 9e294ff..0c67b57 100644
@@ -2540,8 +2540,17 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 {
 	bool kill = nvme_io_incapable(ns->dev) && !blk_queue_dying(ns->queue);
 
-	if (kill)
+	if (kill) {
 		blk_set_queue_dying(ns->queue);
+
+		/*
+		 * The controller was shutdown first if we got here through
+		 * device removal. The shutdown may requeue outstanding
+		 * requests. These need to be aborted immediately so
+		 * del_gendisk doesn't block indefinitely for their completion.
+		 */
+		blk_mq_abort_requeue_list(ns->queue);
+	}
 	if (ns->disk->flags & GENHD_FL_UP)
 		del_gendisk(ns->disk);
 	if (kill || !blk_queue_dying(ns->queue)) {

@@ -2977,6 +2986,15 @@ static void nvme_dev_remove(struct nvme_dev *dev)
 {
 	struct nvme_ns *ns, *next;
 
+	if (nvme_io_incapable(dev)) {
+		/*
+		 * If the device is not capable of IO (surprise hot-removal,
+		 * for example), we need to quiesce prior to deleting the
+		 * namespaces. This will end outstanding requests and prevent
+		 * attempts to sync dirty data.
+		 */
+		nvme_dev_shutdown(dev);
+	}
 	list_for_each_entry_safe(ns, next, &dev->namespaces, list)
 		nvme_ns_remove(ns);
 }

Signature · 1 line

2.6.2.307.g37023ba

Sagi Grimberg

2015-12-13 14:59 UTC #1 lore raw

PATCH 1/1 NVMe: IO ending fixes on surprise removal

drivers/nvme/host/pci.c static void nvme_ns_remove(struct nvme_ns *ns) · line 2552

Quoted 12 lines

drivers/nvme/host/pci.c +9 -1

-	if (kill)
+	if (kill) {
  		blk_set_queue_dying(ns->queue);
+
+		/*
+		 * The controller was shutdown first if we got here through
+		 * device removal. The shutdown may requeue outstanding
+		 * requests. These need to be aborted immediately so
+		 * del_gendisk doesn't block indefinitely for their completion.
+		 */
+		blk_mq_abort_requeue_list(ns->queue);

Something looks convoluted a bit here.
We abort_requeue_list here and...

drivers/nvme/host/pci.c static void nvme_ns_remove(struct nvme_ns *ns) · line 2556

Quoted 4 lines

drivers/nvme/host/pci.c +1 -0

+	}
  	if (ns->disk->flags & GENHD_FL_UP)
  		del_gendisk(ns->disk);
  	if (kill || !blk_queue_dying(ns->queue)) {

Here again?

Keith Busch

author

2015-12-14 16:29 UTC #2 lore raw

PATCH 1/1 NVMe: IO ending fixes on surprise removal

Quoted 10 lines · Sagi Grimberg

drivers/nvme/host/pci.c static void nvme_ns_remove(struct nvme_ns *ns) · line 2552
Quoted 6 lines
drivers/nvme/host/pci.c +6 -0
+		 * The controller was shutdown first if we got here through
+		 * device removal. The shutdown may requeue outstanding
+		 * requests. These need to be aborted immediately so
+		 * del_gendisk doesn't block indefinitely for their completion.
+		 */
+		blk_mq_abort_requeue_list(ns->queue);
Something looks convoluted a bit here.
We abort_requeue_list here and...

Quoted 6 lines

drivers/nvme/host/pci.c static void nvme_ns_remove(struct nvme_ns *ns) · line 2556
Quoted 4 lines
drivers/nvme/host/pci.c +1 -0
+	}
 	if (ns->disk->flags & GENHD_FL_UP)
 		del_gendisk(ns->disk);
 	if (kill || !blk_queue_dying(ns->queue)) {
Here again?

Correct. It's solving two different problems. The first is to end requeued commands that del_gendisk would wait for, like what happens on a surprise removal with filesystem data syncing. The second is to end requeued commands that blk_cleanup_queue would wait for, like direct and passthrough.

One might wonder why the first requeue list abort doesn't address the second case: we don't execute the path containing the first abort requeue list on an orderly removal to allow filesystem data to sync first.

The whole shutdown and removal sequence is a little different today than when it was originally implemented, so will need to verify if it is even possible to have commands on the requeue list at the second point anymore.

Sagi Grimberg

2015-12-15 15:45 UTC #3 lore raw

PATCH 1/1 NVMe: IO ending fixes on surprise removal

Quoted 24 lines · Keith Busch

Quoted 10 lines · Sagi Grimberg
drivers/nvme/host/pci.c static void nvme_ns_remove(struct nvme_ns *ns) · line 2552
Quoted 6 lines
drivers/nvme/host/pci.c +6 -0
+		 * The controller was shutdown first if we got here through
+		 * device removal. The shutdown may requeue outstanding
+		 * requests. These need to be aborted immediately so
+		 * del_gendisk doesn't block indefinitely for their completion.
+		 */
+		blk_mq_abort_requeue_list(ns->queue);
Something looks convoluted a bit here.
We abort_requeue_list here and...
Quoted 6 lines
drivers/nvme/host/pci.c static void nvme_ns_remove(struct nvme_ns *ns) · line 2556
Quoted 4 lines
drivers/nvme/host/pci.c +1 -0
+	}
  	if (ns->disk->flags & GENHD_FL_UP)
  		del_gendisk(ns->disk);
  	if (kill || !blk_queue_dying(ns->queue)) {
Here again?
Correct. It's solving two different problems. The first is to end requeued commands that del_gendisk would wait for, like what happens on a surprise removal with filesystem data syncing. The second is to end requeued commands that blk_cleanup_queue would wait for, like direct and passthrough.

Quoted 3 lines

One might wonder why the first requeue list abort doesn't address the second case: we don't execute the path containing the first abort requeue list on an orderly removal to allow filesystem data to sync first.

Makes sense, the arrangement is just a bit confusing here.

Sujith Pandel

2015-12-18 05:19 UTC #4 lore raw

PATCH 1/1 NVMe: IO ending fixes on surprise removal

In reply to Keith Busch

Quoted 6 lines · Keith Busch

This patch fixes a lost request discovered during IO + hot removal.
The driver's pci removal deletes gendisks prior to shutting down the controller to allow dirty data to sync. Dirty data can not be synced on a surprise removal, though, and would potentially block indefinitely.

Quoted 8 lines

The driver previously had marked the queue as dying in this scenario to prevent new requests from attempting, however it will still block for requests that already entered the queue. This patch fixes this by quiescing IO first, then aborting the requeued requests before deleting disks.
Reported-by Sujith Pandel <sujith_pandel at dell.com>
Signed-off-by Keith Busch <keith.busch at intel.com>

Tested this patch against linux-4.4-rc4.
I/O terminated successfully everytime and hotplug-removal is clean i.e no remnants of the removed NVMe disk is present.

Tested-by Sujith Pandel <sujith_pandel at dell.com>

Thanks and Regards,
Sujith