Patches for 4.7

Patches for 4.7

[Keith Busch]

Just a collection of small and unrelated patches.

Keith Busch (3):
  NVMe: Return ENODEV error on invalid namespace
  NVMe: Short-cut removal on surprise hot-unplug
  NVMe: Set capacity to 0 on all unusable namespaces

 drivers/nvme/host/core.c | 36 ++++++++++++++++++++++++------------
 drivers/nvme/host/nvme.h |  3 +++
 drivers/nvme/host/pci.c  |  8 ++++++--
 3 files changed, 33 insertions(+), 14 deletions(-)

-- 
2.7.2

[PATCH-4.7 1/3] NVMe: Return ENODEV error on invalid namespace

[Keith Busch]

If there is no namespace associated with the request, return an error
code that notifies the application the device isn't there.

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

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 114b928..48c7f3c 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -193,6 +193,8 @@ static inline int nvme_error_status(u16 status)
 	switch (status & 0x7ff) {
 	case NVME_SC_SUCCESS:
 		return 0;
+	case NVME_SC_INVALID_NS:
+		return -ENODEV;
 	case NVME_SC_CAP_EXCEEDED:
 		return -ENOSPC;
 	default:
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 783845c..f99833b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -600,7 +600,7 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 		if (ns && !test_bit(NVME_NS_DEAD, &ns->flags))
 			ret = BLK_MQ_RQ_QUEUE_BUSY;
 		else
-			ret = BLK_MQ_RQ_QUEUE_ERROR;
+			blk_mq_end_request(req, -ENODEV);
 		spin_unlock_irq(&nvmeq->q_lock);
 		goto out;
 	}
@@ -931,7 +931,7 @@ static void nvme_cancel_io(struct request *req, void *data, bool reserved)
 
 	status = NVME_SC_ABORT_REQ;
 	if (blk_queue_dying(req->q))
-		status |= NVME_SC_DNR;
+		status = NVME_SC_INVALID_NS | NVME_SC_DNR;
 	blk_mq_complete_request(req, status);
 }
 
-- 
2.7.2

[PATCH-4.7 1/3] NVMe: Return ENODEV error on invalid namespace

[Christoph Hellwig]

On Mon, May 02, 2016@11:12:06AM -0600, Keith Busch wrote:
> If there is no namespace associated with the request, return an error
> code that notifies the application the device isn't there.

I think we're heading in the wrong direction here.  I actually had some
discussion with Jens and Marting on how we should get rid of overloading
random errnos in the block layer and instead have block layer error
codes.

What applications care?  And where are the meanings of these error
codes documented?

[PATCH-4.7 1/3] NVMe: Return ENODEV error on invalid namespace

[Busch, Keith]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch at infradead.org]
> Sent: Tuesday, May 03, 2016 1:58 AM
> To: Busch, Keith
> Cc: Jens Axboe; Christoph Hellwig; linux-nvme at lists.infradead.org
> Subject: Re: [PATCH-4.7 1/3] NVMe: Return ENODEV error on invalid namespace
> 
> On Mon, May 02, 2016@11:12:06AM -0600, Keith Busch wrote:
> > If there is no namespace associated with the request, return an error
> > code that notifies the application the device isn't there.
> 
> I think we're heading in the wrong direction here.  I actually had some
> discussion with Jens and Marting on how we should get rid of overloading
> random errnos in the block layer and instead have block layer error
> codes.
> 
> What applications care?  And where are the meanings of these error
> codes documented?

This was added at the joint request from one OSV and IHV. I've no idea what they're software is doing with this, but they say it doesn't do the right thing with 'EIO'. ENODEV for invalid namespaces sounded like a reasonable error.

[PATCH-4.7 1/3] NVMe: Return ENODEV error on invalid namespace

[Christoph Hellwig]

On Tue, May 03, 2016@02:19:28PM +0000, Busch, Keith wrote:
> This was added at the joint request from one OSV and IHV. I've no idea
> what they're software is doing with this, but they say it doesn't do the
> right thing with 'EIO'. ENODEV for invalid namespaces sounded like a
> reasonable error.

Well, I'll have to say "no" to this until said OSV and IHV can explain
why they need this in detail on linux-nvme.  And even then we'll
probably need a different solution for them.

[PATCH-4.7 2/3] NVMe: Short-cut removal on surprise hot-unplug

[Keith Busch]

This patch adds a new state that when set has the core automatically
kill request queues prior to removing namespaces. If PCI device is not
present at the time the nvme driver's remove is called, we can kill all
IO queues immediately instead of waiting for the watchdog thread to do
that at its polling interval. This improves scenarios where multiple hot
plug events occur at the same time since it doesn't block pci enumeration.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 12 ++++++++++++
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/pci.c  |  4 ++++
 3 files changed, 17 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e7fbe1f..9f1f318 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -95,6 +95,15 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 			break;
 		}
 		break;
+	case NVME_CTRL_DEAD:
+		switch (old_state) {
+		case NVME_CTRL_DELETING:
+			changed = true;
+			/* FALLTHRU */
+		default:
+			break;
+		}
+		break;
 	default:
 		break;
 	}
@@ -1584,6 +1593,9 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns, *next;
 
+	if (ctrl->state == NVME_CTRL_DEAD)
+		nvme_kill_queues(ctrl);
+
 	mutex_lock(&ctrl->namespaces_mutex);
 	list_for_each_entry_safe(ns, next, &ctrl->namespaces, list)
 		nvme_ns_remove(ns);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 48c7f3c..78135cd 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -72,6 +72,7 @@ enum nvme_ctrl_state {
 	NVME_CTRL_LIVE,
 	NVME_CTRL_RESETTING,
 	NVME_CTRL_DELETING,
+	NVME_CTRL_DEAD,
 };
 
 struct nvme_ctrl {
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index f99833b..0881b54 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2019,6 +2019,10 @@ static void nvme_remove(struct pci_dev *pdev)
 	nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
 
 	pci_set_drvdata(pdev, NULL);
+
+	if (!pci_device_is_present(pdev))
+		nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DEAD);
+
 	nvme_uninit_ctrl(&dev->ctrl);
 	nvme_dev_disable(dev, true);
 	flush_work(&dev->reset_work);
-- 
2.7.2

[PATCH-4.7 2/3] NVMe: Short-cut removal on surprise hot-unplug

[Christoph Hellwig]

> @@ -1584,6 +1593,9 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
>  {
>  	struct nvme_ns *ns, *next;
>  
> +	if (ctrl->state == NVME_CTRL_DEAD)
> +		nvme_kill_queues(ctrl);
> +
>  	mutex_lock(&ctrl->namespaces_mutex);
>  	list_for_each_entry_safe(ns, next, &ctrl->namespaces, list)
>  		nvme_ns_remove(ns);

Not happy about so much magic.. At least add comments explaining why
you're doing this, and that it's just an optimization.

[PATCH-4.7 2/3] NVMe: Short-cut removal on surprise hot-unplug

[Keith Busch]

On Tue, May 03, 2016@01:01:40AM -0700, Christoph Hellwig wrote:
> > @@ -1584,6 +1593,9 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
> >  {
> >  	struct nvme_ns *ns, *next;
> >  
> > +	if (ctrl->state == NVME_CTRL_DEAD)
> > +		nvme_kill_queues(ctrl);
> > +
> >  	mutex_lock(&ctrl->namespaces_mutex);
> >  	list_for_each_entry_safe(ns, next, &ctrl->namespaces, list)
> >  		nvme_ns_remove(ns);
> 
> Not happy about so much magic.. At least add comments explaining why
> you're doing this, and that it's just an optimization.

We could just call nvme_kill_queues from the pci driver's nvme_remove
instead of adding the state. I just thought this was universally useful
for a non-graceful disconnect.

[PATCH-4.7 2/3] NVMe: Short-cut removal on surprise hot-unplug

[Christoph Hellwig]

On Tue, May 10, 2016@03:16:27PM -0400, Keith Busch wrote:
> On Tue, May 03, 2016@01:01:40AM -0700, Christoph Hellwig wrote:
> > > @@ -1584,6 +1593,9 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
> > >  {
> > >  	struct nvme_ns *ns, *next;
> > >  
> > > +	if (ctrl->state == NVME_CTRL_DEAD)
> > > +		nvme_kill_queues(ctrl);
> > > +
> > >  	mutex_lock(&ctrl->namespaces_mutex);
> > >  	list_for_each_entry_safe(ns, next, &ctrl->namespaces, list)
> > >  		nvme_ns_remove(ns);
> > 
> > Not happy about so much magic.. At least add comments explaining why
> > you're doing this, and that it's just an optimization.
> 
> We could just call nvme_kill_queues from the pci driver's nvme_remove
> instead of adding the state. I just thought this was universally useful
> for a non-graceful disconnect.

I think it's useful, it's just a little too magic to go without a
comment.  Can you add comments explaining it and resend?

[PATCH-4.7 3/3] NVMe: Set capacity to 0 on all unusable namespaces

[Keith Busch]

Set the disk capacity to 0 if unable to identify a namespace for any
reason. This way revalidating detached namespaces will be deleted quicker
since dirty data can't be sync'ed.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9f1f318..b9de770 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -771,31 +771,27 @@ static void nvme_config_discard(struct nvme_ns *ns)
 static int nvme_revalidate_disk(struct gendisk *disk)
 {
 	struct nvme_ns *ns = disk->private_data;
-	struct nvme_id_ns *id;
+	struct nvme_id_ns *id = NULL;
 	u8 lbaf, pi_type;
 	u16 old_ms;
 	unsigned short bs;
 
-	if (test_bit(NVME_NS_DEAD, &ns->flags)) {
-		set_capacity(disk, 0);
-		return -ENODEV;
-	}
+	if (test_bit(NVME_NS_DEAD, &ns->flags))
+		goto no_dev;
+
 	if (nvme_identify_ns(ns->ctrl, ns->ns_id, &id)) {
 		dev_warn(disk_to_dev(ns->disk), "%s: Identify failure\n",
 				__func__);
-		return -ENODEV;
-	}
-	if (id->ncap == 0) {
-		kfree(id);
-		return -ENODEV;
+		goto no_dev;
 	}
+	if (id->ncap == 0)
+		goto no_dev;
 
 	if (nvme_nvm_ns_supported(ns, id) && ns->type != NVME_NS_LIGHTNVM) {
 		if (nvme_nvm_register(ns->queue, disk->disk_name)) {
 			dev_warn(disk_to_dev(ns->disk),
 				"%s: LightNVM init failure\n", __func__);
-			kfree(id);
-			return -ENODEV;
+			goto no_dev;
 		}
 		ns->type = NVME_NS_LIGHTNVM;
 	}
@@ -845,6 +841,10 @@ static int nvme_revalidate_disk(struct gendisk *disk)
 
 	kfree(id);
 	return 0;
+ no_dev:
+	kfree(id);
+	set_capacity(disk, 0);
+	return -ENODEV;
 }
 
 static char nvme_pr_type(enum pr_type type)
-- 
2.7.2

[PATCH-4.7 3/3] NVMe: Set capacity to 0 on all unusable namespaces

[Christoph Hellwig]

On Mon, May 02, 2016@11:12:08AM -0600, Keith Busch wrote:
> Set the disk capacity to 0 if unable to identify a namespace for any
> reason. This way revalidating detached namespaces will be deleted quicker
> since dirty data can't be sync'ed.
> 
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
>  drivers/nvme/host/core.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 9f1f318..b9de770 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -771,31 +771,27 @@ static void nvme_config_discard(struct nvme_ns *ns)
>  static int nvme_revalidate_disk(struct gendisk *disk)
>  {
>  	struct nvme_ns *ns = disk->private_data;
> -	struct nvme_id_ns *id;
> +	struct nvme_id_ns *id = NULL;
>  	u8 lbaf, pi_type;
>  	u16 old_ms;
>  	unsigned short bs;
>  
> -	if (test_bit(NVME_NS_DEAD, &ns->flags)) {
> -		set_capacity(disk, 0);
> -		return -ENODEV;
> -	}
> +	if (test_bit(NVME_NS_DEAD, &ns->flags))
> +		goto no_dev;
> +
>  	if (nvme_identify_ns(ns->ctrl, ns->ns_id, &id)) {
>  		dev_warn(disk_to_dev(ns->disk), "%s: Identify failure\n",
>  				__func__);
> -		return -ENODEV;
> -	}
> -	if (id->ncap == 0) {
> -		kfree(id);
> -		return -ENODEV;
> +		goto no_dev;
>  	}
> +	if (id->ncap == 0)
> +		goto no_dev;
>  
>  	if (nvme_nvm_ns_supported(ns, id) && ns->type != NVME_NS_LIGHTNVM) {
>  		if (nvme_nvm_register(ns->queue, disk->disk_name)) {
>  			dev_warn(disk_to_dev(ns->disk),
>  				"%s: LightNVM init failure\n", __func__);
> -			kfree(id);
> -			return -ENODEV;
> +			goto no_dev;

Not related to this patch, but: the lightnvm code really needs to go away
here - the NVMe spec is clear that the Identify Namespace structure is
NVM Command set specific, and our whole block device implementation
shouldn't even be called for lightnvm.  

Otherwise this looks fine to me,

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