NVMe: Surprise removal fixes

First on the 'dd' experiements, I did not find kernel that "worked" as expected on a surprise removal. The IO process runs until SIGKILL is recieved. I don't think that's right, and it causes noticable system performance issues for unrelated tasks.

This series is just focusing on getting the driver to cleanup its part so it can unbind from a controller.

Earlier feedback suggested this functionality be in the block layer. I don't have the devices to test what happens with other drivers, and the desired sequence seems unique to NVMe. Maybe that's an indication that the driver's flow could benefit from some redesign, or maybe it's an artifact from having the controller and storage being the same device.

In any case, I would like to isolate this fix to the NVMe driver in the interest of time, and flush out the block layer before the next merge window.

Keith Busch (4):
NVMe: Fix io incapable return values
NVMe: Sync stopped queues with block layer
NVMe: Surprise removal fixes
blk-mq: End unstarted requests on dying queue

 block/blk-mq.c           |  6 ++++--
 drivers/nvme/host/core.c | 14 ++++++++------
 drivers/nvme/host/nvme.h |  4 ++--
 drivers/nvme/host/pci.c  | 14 ++++++++++++++
 4 files changed, 28 insertions(+), 10 deletions(-)

2.6.2.307.g37023ba

The function returns true when the controller can't handle IO.

---
 drivers/nvme/host/nvme.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 4fb5bb7..9664d07 100644
@@ -139,9 +139,9 @@ static inline bool nvme_io_incapable(struct nvme_ctrl *ctrl)
 	u32 val = 0;
 
 	if (ctrl->ops->io_incapable(ctrl))
-		return false;
+		return true;
 	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &val))
-		return false;
+		return true;
 	return val & NVME_CSTS_CFS;
 }
 

2.6.2.307.g37023ba

Looks good,

Looks good,

Make sure no work will submit requests to a stopped h/w queue.

---
 drivers/nvme/host/core.c | 1 +
 1 file changed, 1 insertion(+)

index c5bf001..9d415489 100644
@@ -1415,6 +1415,7 @@ void nvme_stop_queues(struct nvme_ctrl *ctrl)
 
 		blk_mq_cancel_requeue_work(ns->queue);
 		blk_mq_stop_hw_queues(ns->queue);
+		blk_sync_queue(ns->queue);
 	}
 	mutex_unlock(&ctrl->namespaces_mutex);
 }

2.6.2.307.g37023ba

Not sure what that buys us, and it's not documented. But I don't mind having it. Can you provide a better explanation why it's added?

This was trying to close that gap with requests queueing on stopped h/w queues and writing the queue doorbell. It works for requests through the kblock_workqueue, but that's not always the case, so it's only a partial solution.

I don't think a driver call to blk_sync_queue is a good idea. It might be useful as part of a bigger hammer, but then it should be hidden behind that API.

Temporarily halts IO queues on surprise removal prior to flushing pending requests to an error completion.

---
 drivers/nvme/host/core.c | 13 +++++++------
 drivers/nvme/host/pci.c  | 14 ++++++++++++++
 2 files changed, 21 insertions(+), 6 deletions(-)

index 9d415489..f11cb5a 100644
@@ -1186,15 +1186,16 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 	lockdep_assert_held(&ns->ctrl->namespaces_mutex);
 
 	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.
+		 * The request queue is currently stoppped if we got here.
+		 * Prevent new requests from entering before flushing the rest.
+		 * New requests will fail to enter a dying frozen queue, which
+		 * allows blk_cleanup_queue to complete.
 		 */
+		blk_mq_freeze_queue_start(ns->queue);
+		blk_set_queue_dying(ns->queue);
 		blk_mq_abort_requeue_list(ns->queue);
+		blk_mq_start_hw_queues(ns->queue);
 	}
 	if (ns->disk->flags & GENHD_FL_UP) {
 		if (blk_get_integrity(ns->disk))

index 72ef832..4cc7398 100644
@@ -640,6 +640,11 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	struct nvme_command cmnd;
 	int ret = BLK_MQ_RQ_QUEUE_OK;
 
+	if (unlikely(blk_queue_dying(req->q))) {
+		blk_mq_end_request(req, -EFAULT);
+		return BLK_MQ_RQ_QUEUE_OK;
+	}
+
 	/*
 	 * If formated with metadata, require the block layer provide a buffer
 	 * unless this namespace is formated such that the metadata can be

@@ -2118,6 +2123,15 @@ static void nvme_remove(struct pci_dev *pdev)
 	pci_set_drvdata(pdev, NULL);
 	flush_work(&dev->reset_work);
 	flush_work(&dev->scan_work);
+
+	/*
+	 * If the controller can't do IO (surprise removal, for example), we
+	 * need to halt queues prior to deleting namespaces. This ends
+	 * outstanding requests and prevents attempts to sync dirty data.
+	 */
+	if (nvme_io_incapable(&dev->ctrl))
+		nvme_dev_disable(dev, true);
+
 	nvme_remove_namespaces(&dev->ctrl);
 	nvme_uninit_ctrl(&dev->ctrl);
 	nvme_dev_disable(dev, true);

2.6.2.307.g37023ba

This is far from perfect. Let me try explaining what's happening, then I hope to abandon this patch and do it correctly. :)

The test does buffered IO writes with 'dd'. If I yank the drive, 'dd' continues until writing the device's capacity. The device is gone, so there is no place to write dirty pages. The capcity exceeds available memory, so 'dd' will wait even though everything it tries to write fails. It consumes all available memory and the system becomes noticably slower.

Ending that process is not what this patch accomplishes. It just lets del_gendisk and blk_cleanup_queue complete by not allowing processes to continue entering the queue by freezing first. Driver unbind doesn't complete without this.

But there is a possible deadlock here. If you do an orderly removal and surprise removal immediatly after, the device will initially be considered capable of io and let dirty data sync. The hot removal will make that impossible, but the orderly removal is holding the namespace mutex, so no one can cleanup the namespace's queue.

Anyway, I wish to withdraw this patch. I will send an alternate this week using state machine driven logic that should fix the deadlock unless there is a different proposal sooner.

I've a new patch that passes all my tests, including the removal deadlock using device states.

I have namespace capacity set to 0 and revalidate to get buffered writers to end. The error handling is done in the nvme_workq, which currently has WQ_MEM_RECLAIM set. That trigger a warning when revalidate_disk attempts to sync with a non-MEM_RECLAIM work queue, so I removed the flag from NVMe's to suppress the warning, but I don't see what having that flag gained us in the first place. It looks to me that it just spawns a rescuer, but do we need that?

Go directly to ending a request if it wasn't started. Previously, completing a request may invoke a driver callback for a request it didn't initialize.

---
 block/blk-mq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 4c0622f..56c0a72 100644
@@ -599,8 +599,10 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 		 * If a request wasn't started before the queue was
 		 * marked dying, kill it here or it'll go unnoticed.
 		 */
-		if (unlikely(blk_queue_dying(rq->q)))
-			blk_mq_complete_request(rq, -EIO);
+		if (unlikely(blk_queue_dying(rq->q))) {
+			rq->errors = -EIO;
+			blk_mq_end_request(rq, rq->errors);
+		}
 		return;
 	}
 

2.6.2.307.g37023ba

Looks good,

Looks good,

Isn't the dd behavior correct? For non-direct dd, even if a surprise remove happened, it simply keeps dirtying the page cache and is not aware of any errors below.

-----Original Message-----
From: Keith Busch [mailto:[email protected]]
Sent: Thursday, February 4, 2016 12:06 AM
To: linux-nvme at lists.infradead.org; Jens Axboe
Cc: Christoph Hellwig; Sagi Grimberg; Wenbo Wang; Keith Busch
Subject: [PATCH 0/4] NVMe: Surprise removal fixes

First on the 'dd' experiements, I did not find kernel that "worked"
as expected on a surprise removal. The IO process runs until SIGKILL is recieved. I don't think that's right, and it causes noticable system performance issues for unrelated tasks.

This series is just focusing on getting the driver to cleanup its part so it can unbind from a controller.

Earlier feedback suggested this functionality be in the block layer. I don't have the devices to test what happens with other drivers, and the desired sequence seems unique to NVMe. Maybe that's an indication that the driver's flow could benefit from some redesign, or maybe it's an artifact from having the controller and storage being the same device.

In any case, I would like to isolate this fix to the NVMe driver in the interest of time, and flush out the block layer before the next merge window.

Keith Busch (4):
NVMe: Fix io incapable return values
NVMe: Sync stopped queues with block layer
NVMe: Surprise removal fixes
blk-mq: End unstarted requests on dying queue

 block/blk-mq.c           |  6 ++++--
 drivers/nvme/host/core.c | 14 ++++++++------  drivers/nvme/host/nvme.h |  4 ++--  drivers/nvme/host/pci.c  | 14 ++++++++++++++
 4 files changed, 28 insertions(+), 10 deletions(-)

--
2.6.2.307.g37023ba

Try it with a usb drive. It ends with ENOSPC error.

Hi Keith,

Umm, it actually isn't. I think this is what all the block drivers want.

I'm fine with that. But we definitely want to avoid the queue_rq conditional in the long run...