[PATCH v2] NVMe: Add controller state for scheduling resets

[PATCH v2] NVMe: Add controller state for scheduling resets

[Keith Busch]

All resets now must be submitted through the new state. This makes it
easier to coordinate controller tasks and simplifies the reset logic.

Since all resets go through the same state machine, and most of these
don't want to block for the reset to complete, this patch makes user
initiated resets asynchronous as well. Making user resets block until
complete wasn't necessary in the first place.

Signed-off-by: Keith Busch <keith.busch at intel.com>
Reviewed-by: Christoph Hellwig <hch at lst.de>
---
v1 -> v2:
  Fix export symbol to use GPL.

 drivers/nvme/host/core.c | 26 ++++++++++++++++++++++----
 drivers/nvme/host/nvme.h |  2 ++
 drivers/nvme/host/pci.c  | 40 ++++++++++++++++------------------------
 3 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 6290477..c822977 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -66,6 +66,15 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 
 	spin_lock_irq(&ctrl->lock);
 	switch (new_state) {
+	case NVME_CTRL_SCHED_RESET:
+		switch (old_state) {
+		case NVME_CTRL_NEW:
+		case NVME_CTRL_LIVE:
+			changed = true;
+			/* FALLTHRU */
+		default:
+			break;
+		}
 	case NVME_CTRL_LIVE:
 		switch (old_state) {
 		case NVME_CTRL_RESETTING:
@@ -77,8 +86,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		break;
 	case NVME_CTRL_RESETTING:
 		switch (old_state) {
-		case NVME_CTRL_NEW:
-		case NVME_CTRL_LIVE:
+		case NVME_CTRL_SCHED_RESET:
 			changed = true;
 			/* FALLTHRU */
 		default:
@@ -89,6 +97,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		switch (old_state) {
 		case NVME_CTRL_LIVE:
 		case NVME_CTRL_RESETTING:
+		case NVME_CTRL_SCHED_RESET:
 			changed = true;
 			/* FALLTHRU */
 		default:
@@ -1209,6 +1218,15 @@ out_unlock:
 	return ret;
 }
 
+int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
+{
+	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_SCHED_RESET))
+		return -EBUSY;
+
+	return ctrl->ops->reset_ctrl(ctrl);
+}
+EXPORT_SYMBOL_GPL(nvme_reset_ctrl);
+
 static long nvme_dev_ioctl(struct file *file, unsigned int cmd,
 		unsigned long arg)
 {
@@ -1222,7 +1240,7 @@ static long nvme_dev_ioctl(struct file *file, unsigned int cmd,
 		return nvme_dev_user_cmd(ctrl, argp);
 	case NVME_IOCTL_RESET:
 		dev_warn(ctrl->device, "resetting controller\n");
-		return ctrl->ops->reset_ctrl(ctrl);
+		return nvme_reset_ctrl(ctrl);
 	case NVME_IOCTL_SUBSYS_RESET:
 		return nvme_reset_subsystem(ctrl);
 	case NVME_IOCTL_RESCAN:
@@ -1248,7 +1266,7 @@ static ssize_t nvme_sysfs_reset(struct device *dev,
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
 	int ret;
 
-	ret = ctrl->ops->reset_ctrl(ctrl);
+	ret = nvme_reset_ctrl(ctrl);
 	if (ret < 0)
 		return ret;
 	return count;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1daa048..b948f26 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -73,6 +73,7 @@ enum nvme_ctrl_state {
 	NVME_CTRL_RESETTING,
 	NVME_CTRL_DELETING,
 	NVME_CTRL_DEAD,
+	NVME_CTRL_SCHED_RESET,
 };
 
 struct nvme_ctrl {
@@ -207,6 +208,7 @@ static inline bool nvme_req_needs_retry(struct request *req, u16 status)
 		(jiffies - req->start_time) < req->timeout;
 }
 
+int nvme_reset_ctrl(struct nvme_ctrl *ctrl);
 bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		enum nvme_ctrl_state new_state);
 int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index cd5f445..dfb8f2a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -68,7 +68,6 @@ static struct workqueue_struct *nvme_workq;
 struct nvme_dev;
 struct nvme_queue;
 
-static int nvme_reset(struct nvme_dev *dev);
 static void nvme_process_cq(struct nvme_queue *nvmeq);
 static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown);
 
@@ -868,7 +867,7 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 			 "I/O %d QID %d timeout, reset controller\n",
 			 req->tag, nvmeq->qid);
 		nvme_dev_disable(dev, false);
-		queue_work(nvme_workq, &dev->reset_work);
+		nvme_reset_ctrl(&dev->ctrl);
 
 		/*
 		 * Mark the request as handled, since the inline shutdown
@@ -1285,7 +1284,7 @@ static void nvme_watchdog_timer(unsigned long data)
 
 	/* Skip controllers under certain specific conditions. */
 	if (nvme_should_reset(dev, csts)) {
-		if (queue_work(nvme_workq, &dev->reset_work))
+		if (nvme_reset_ctrl(&dev->ctrl))
 			dev_warn(dev->dev,
 				"Failed status: 0x%x, reset controller.\n",
 				csts);
@@ -1773,7 +1772,11 @@ static void nvme_reset_work(struct work_struct *work)
 	struct nvme_dev *dev = container_of(work, struct nvme_dev, reset_work);
 	int result = -ENODEV;
 
-	if (WARN_ON(dev->ctrl.state == NVME_CTRL_RESETTING))
+	/*
+	 * The controller is being removed if its state machine can't
+	 * transition to reset.
+	 */
+	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING))
 		goto out;
 
 	/*
@@ -1783,9 +1786,6 @@ static void nvme_reset_work(struct work_struct *work)
 	if (dev->ctrl.ctrl_config & NVME_CC_ENABLE)
 		nvme_dev_disable(dev, false);
 
-	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING))
-		goto out;
-
 	result = nvme_pci_enable(dev);
 	if (result)
 		goto out;
@@ -1855,18 +1855,6 @@ static void nvme_remove_dead_ctrl_work(struct work_struct *work)
 	nvme_put_ctrl(&dev->ctrl);
 }
 
-static int nvme_reset(struct nvme_dev *dev)
-{
-	if (!dev->ctrl.admin_q || blk_queue_dying(dev->ctrl.admin_q))
-		return -ENODEV;
-
-	if (!queue_work(nvme_workq, &dev->reset_work))
-		return -EBUSY;
-
-	flush_work(&dev->reset_work);
-	return 0;
-}
-
 static int nvme_pci_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
 {
 	*val = readl(to_nvme_dev(ctrl)->bar + off);
@@ -1887,7 +1875,11 @@ static int nvme_pci_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
 
 static int nvme_pci_reset_ctrl(struct nvme_ctrl *ctrl)
 {
-	return nvme_reset(to_nvme_dev(ctrl));
+	if (WARN_ON(ctrl->state != NVME_CTRL_SCHED_RESET))
+		return -EBUSY;
+	if (!queue_work(nvme_workq, &(to_nvme_dev(ctrl)->reset_work)))
+		return -EBUSY;
+	return 0;
 }
 
 static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
@@ -1968,7 +1960,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	dev_info(dev->ctrl.device, "pci function %s\n", dev_name(&pdev->dev));
 
-	queue_work(nvme_workq, &dev->reset_work);
+	nvme_reset_ctrl(&dev->ctrl);
 	return 0;
 
  release_pools:
@@ -1990,7 +1982,7 @@ static void nvme_reset_notify(struct pci_dev *pdev, bool prepare)
 	if (prepare)
 		nvme_dev_disable(dev, false);
 	else
-		queue_work(nvme_workq, &dev->reset_work);
+		nvme_reset_ctrl(&dev->ctrl);
 }
 
 static void nvme_shutdown(struct pci_dev *pdev)
@@ -2041,7 +2033,7 @@ static int nvme_resume(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct nvme_dev *ndev = pci_get_drvdata(pdev);
 
-	queue_work(nvme_workq, &ndev->reset_work);
+	nvme_reset_ctrl(&ndev->ctrl);
 	return 0;
 }
 #endif
@@ -2080,7 +2072,7 @@ static pci_ers_result_t nvme_slot_reset(struct pci_dev *pdev)
 
 	dev_info(dev->ctrl.device, "restart after slot reset\n");
 	pci_restore_state(pdev);
-	queue_work(nvme_workq, &dev->reset_work);
+	nvme_reset_ctrl(&dev->ctrl);
 	return PCI_ERS_RESULT_RECOVERED;
 }
 
-- 
2.7.2

[PATCH v2] NVMe: Add controller state for scheduling resets

[Keith Busch]

On Tue, May 31, 2016@03:18:04PM -0600, Keith Busch wrote:
>  	/* Skip controllers under certain specific conditions. */
>  	if (nvme_should_reset(dev, csts)) {
> -		if (queue_work(nvme_workq, &dev->reset_work))
> +		if (nvme_reset_ctrl(&dev->ctrl))
>  			dev_warn(dev->dev,
>  				"Failed status: 0x%x, reset controller.\n",
>  				csts);

err, that condition should be !nvme_reset_ctrl since it returns 0 on
success rather than the status of queue_work. It still does correctly
resets the controller, but we miss the very useful warning message. Let
me send a v3...

[PATCH v2] NVMe: Add controller state for scheduling resets

[Sagi Grimberg]

On 01/06/16 00:18, Keith Busch wrote:
> All resets now must be submitted through the new state. This makes it
> easier to coordinate controller tasks and simplifies the reset logic.
>
> Since all resets go through the same state machine, and most of these
> don't want to block for the reset to complete, this patch makes user
> initiated resets asynchronous as well. Making user resets block until
> complete wasn't necessary in the first place.
>
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> Reviewed-by: Christoph Hellwig <hch at lst.de>
> ---
> v1 -> v2:
>    Fix export symbol to use GPL.
>
>   drivers/nvme/host/core.c | 26 ++++++++++++++++++++++----
>   drivers/nvme/host/nvme.h |  2 ++
>   drivers/nvme/host/pci.c  | 40 ++++++++++++++++------------------------
>   3 files changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 6290477..c822977 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -66,6 +66,15 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
>
>   	spin_lock_irq(&ctrl->lock);
>   	switch (new_state) {
> +	case NVME_CTRL_SCHED_RESET:
> +		switch (old_state) {
> +		case NVME_CTRL_NEW:
> +		case NVME_CTRL_LIVE:
> +			changed = true;
> +			/* FALLTHRU */
> +		default:
> +			break;
> +		}
>   	case NVME_CTRL_LIVE:
>   		switch (old_state) {
>   		case NVME_CTRL_RESETTING:
> @@ -77,8 +86,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
>   		break;
>   	case NVME_CTRL_RESETTING:
>   		switch (old_state) {
> -		case NVME_CTRL_NEW:
> -		case NVME_CTRL_LIVE:
> +		case NVME_CTRL_SCHED_RESET:
>   			changed = true;
>   			/* FALLTHRU */
>   		default:
> @@ -89,6 +97,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
>   		switch (old_state) {
>   		case NVME_CTRL_LIVE:
>   		case NVME_CTRL_RESETTING:
> +		case NVME_CTRL_SCHED_RESET:
>   			changed = true;
>   			/* FALLTHRU */
>   		default:
> @@ -1209,6 +1218,15 @@ out_unlock:
>   	return ret;
>   }
>
> +int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
> +{
> +	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_SCHED_RESET))
> +		return -EBUSY;
> +
> +	return ctrl->ops->reset_ctrl(ctrl);
> +}
> +EXPORT_SYMBOL_GPL(nvme_reset_ctrl);
> +
>   static long nvme_dev_ioctl(struct file *file, unsigned int cmd,
>   		unsigned long arg)
>   {
> @@ -1222,7 +1240,7 @@ static long nvme_dev_ioctl(struct file *file, unsigned int cmd,
>   		return nvme_dev_user_cmd(ctrl, argp);
>   	case NVME_IOCTL_RESET:
>   		dev_warn(ctrl->device, "resetting controller\n");
> -		return ctrl->ops->reset_ctrl(ctrl);
> +		return nvme_reset_ctrl(ctrl);

Hey Keith,

So now that pci doesn't live alone in the subsystem (or won't in the
near future) this sort of change will affect the loop and rdma logic.

My understanding of this patch is that ctrl->ops->reset_ctrl is still
allowed to be synchronous, it just makes an extra state transition
(SCHED_RESET -> RESETTING instead of RESETTING immediately).

Is my understanding correct? Do you think that we should modify the
loop and rdma drivers to do async reset as well?

This also makes the different transport behave differently at the moment
which is something we should strive to avoid...

[PATCH v2] NVMe: Add controller state for scheduling resets

[Christoph Hellwig]

On Wed, Jun 08, 2016@02:59:40PM +0300, Sagi Grimberg wrote:
> Is my understanding correct? Do you think that we should modify the
> loop and rdma drivers to do async reset as well?
> 
> This also makes the different transport behave differently at the moment
> which is something we should strive to avoid...

Don't worry, I have an almost ready patch series to pull this change
it in, it actually helps for fabrics as well.

[PATCH v2] NVMe: Add controller state for scheduling resets

[Sagi Grimberg]

>> Is my understanding correct? Do you think that we should modify the
>> loop and rdma drivers to do async reset as well?
>>
>> This also makes the different transport behave differently at the moment
>> which is something we should strive to avoid...
>
> Don't worry, I have an almost ready patch series to pull this change
> it in, it actually helps for fabrics as well.

OK, I'll be waiting to see it then :)

[PATCH v2] NVMe: Add controller state for scheduling resets

[Christoph Hellwig]

On Wed, Jun 08, 2016@03:11:52PM +0300, Sagi Grimberg wrote:
> >Don't worry, I have an almost ready patch series to pull this change
> >it in, it actually helps for fabrics as well.
> 
> OK, I'll be waiting to see it then :)

The changes are fairly big and in hairy code, so my life would be a lot
simpler if the whole series could go in after the initial fabrics merge.