[PATCH v3] NVMe: Add controller state for scheduling resets
Guilherme G. Piccoli
gpiccoli at linux.vnet.ibm.com
Tue May 31 15:10:45 PDT 2016
On 05/31/2016 06:25 PM, 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.
Keith, I'm working on a patch right now that explores the capacity of
differentiate between resets. What I'm doing is re-working a quirk
already sent to the list, in which we need to wait some seconds before
read the NVME_REG_CSTS register in a specific adapter after fw-activate
event.
The improved quirk was counting on ctrl->state to differentiate between
resets; whenever reset_controller was issued from userspace, this state
was set as NVME_CTRL_LIVE and in any other case, the state was
NVME_CTRL_RESETTING.
Now, I tested the quirk after adding this patch and, as expected, the
state is always NVME_CTRL_RESETTING. So, is there any other way to
differentiate between resets?
Thanks in advance,
Guilherme
>
> 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>
> ---
> v2 -> v3:
> Fixed the warning message print on failed controller.
>
> 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;
> }
>
More information about the Linux-nvme
mailing list