[PATCH 0/4] NVMe: Surprise removal fixes

[PATCH 0/4] NVMe: Surprise removal fixes

[Keith Busch]

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

[PATCH 1/4] NVMe: Fix io incapable return values

[Keith Busch]

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

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/nvme.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 4fb5bb7..9664d07 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -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

[PATCH 1/4] NVMe: Fix io incapable return values

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagig at mellanox.com>

[PATCH 1/4] NVMe: Fix io incapable return values

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 2/4] NVMe: Sync stopped queues with block layer

[Keith Busch]

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

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c5bf001..9d415489 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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

[PATCH 2/4] NVMe: Sync stopped queues with block layer

[Sagi Grimberg]

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

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?

[PATCH 2/4] NVMe: Sync stopped queues with block layer

[Keith Busch]

On Thu, Feb 04, 2016@05:57:07PM +0200, Sagi Grimberg wrote:
> 
> >Make sure no work will submit requests to a stopped h/w queue.
> 
> 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.

[PATCH 2/4] NVMe: Sync stopped queues with block layer

[Christoph Hellwig]

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.

[PATCH 3/4] NVMe: Surprise removal fixes

[Keith Busch]

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

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 13 +++++++------
 drivers/nvme/host/pci.c  | 14 ++++++++++++++
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9d415489..f11cb5a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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))
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 72ef832..4cc7398 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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

[PATCH 3/4] NVMe: Surprise removal fixes

[Christoph Hellwig]

On Wed, Feb 03, 2016@09:05:42AM -0700, Keith Busch wrote:
> Temporarily halts IO queues on surprise removal prior to flushing
> pending requests to an error completion.
> 
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
>  drivers/nvme/host/core.c | 13 +++++++------
>  drivers/nvme/host/pci.c  | 14 ++++++++++++++
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 9d415489..f11cb5a 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -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);
>  	}

Do we really still need all this magic if ->queue_rq returns a failure
if the queue is dying?

> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 72ef832..4cc7398 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -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;
> +	}

"Bad address" is a really odd error for a dying block device.

[PATCH 3/4] NVMe: Surprise removal fixes

[Keith Busch]

On Mon, Feb 08, 2016@10:16:40AM -0800, Christoph Hellwig wrote:
> On Wed, Feb 03, 2016@09:05:42AM -0700, Keith Busch wrote:
> > +		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);
> >  	}
> 
> Do we really still need all this magic if ->queue_rq returns a failure
> if the queue is dying?

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.

[PATCH 3/4] NVMe: Surprise removal fixes

[Keith Busch]

On Mon, Feb 08, 2016@06:38:23PM +0000, Keith Busch wrote:
> On Mon, Feb 08, 2016@10:16:40AM -0800, Christoph Hellwig wrote:
> > Do we really still need all this magic if ->queue_rq returns a failure
> > if the queue is dying?
> 
> This is far from perfect. Let me try explaining what's happening, then
> I hope to abandon this patch and do it correctly. :)

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?

[PATCH 4/4] blk-mq: End unstarted requests on dying queue

[Keith Busch]

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.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 block/blk-mq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4c0622f..56c0a72 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -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

[PATCH 4/4] blk-mq: End unstarted requests on dying queue

[Sagi Grimberg]

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

Looks good,

Reviewed-by: Sagi Grimberg <sagig at mellanox.com>

[PATCH 4/4] blk-mq: End unstarted requests on dying queue

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 0/4] NVMe: Surprise removal fixes

[Wenbo Wang]

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:keith.busch@intel.com] 
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

[PATCH 0/4] NVMe: Surprise removal fixes

[Keith Busch]

On Thu, Feb 04, 2016@03:25:50AM +0000, Wenbo Wang wrote:
> 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.

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

[PATCH 0/4] NVMe: Surprise removal fixes

[Sagi Grimberg]

Hi Keith,

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

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

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

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