Hi Keith
Can I have the honor of getting your comment on this patch?
Thanks in advance
Jianchao
On 03/08/2018 02:19 PM, Jianchao Wang wrote:
> nvme_dev_disable will issue command on adminq to clear HMB and
> delete io cq/sqs, maybe more in the future. When adminq no response,
> it has to depends on timeout path. However, nvme_timeout has to
> invoke nvme_dev_disable before return, so that the DMA mappings
> could be released safely. This will introduce dangerous circular
> dependence. Moreover, the whole nvme_dev_disable is under
> shutdown_lock even waiting for the command, this makes things
> worse.
>
> To avoid this, this patch refactors the nvme_timeout. The basic
> principle is:
> - When need to schedule reset_work, hand over expired requests
> to nvme_dev_disable. They will be completed after the controller
> is disabled/shtudown.
> - When requests from nvme_dev_disable and nvme_reset_work expires,
> disable the controller directly then the request could be completed
> to wakeup the waiter. nvme_pci_disable_ctrl_directly is introduced
> for this, it doesn't send commands on adminq and the shutdown_lock
> is not needed here, because the nvme_abort_requests_sync in
> nvme_dev_disable could synchronize with nvme_timeout.
>
> Signed-off-by: Jianchao Wang <jianchao.w.w...@oracle.com>
> ---
> drivers/nvme/host/pci.c | 199
> ++++++++++++++++++++++++++++++++----------------
> 1 file changed, 134 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index e186158..ce09057 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -70,6 +70,7 @@ struct nvme_queue;
>
> static void nvme_process_cq(struct nvme_queue *nvmeq);
> static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown);
> +static void nvme_pci_disable_ctrl_directly(struct nvme_dev *dev);
>
> /*
> * Represents an NVM Express device. Each nvme_dev is a PCI function.
> @@ -98,6 +99,7 @@ struct nvme_dev {
> u32 cmbloc;
> struct nvme_ctrl ctrl;
> struct completion ioq_wait;
> + bool inflight_flushed;
>
> /* shadow doorbell buffer support: */
> u32 *dbbuf_dbs;
> @@ -1180,73 +1182,13 @@ static void nvme_warn_reset(struct nvme_dev *dev, u32
> csts)
> csts, result);
> }
>
> -static enum blk_eh_timer_return nvme_timeout(struct request *req, bool
> reserved)
> +static enum blk_eh_timer_return nvme_pci_abort_io_req(struct request *req)
> {
> struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> struct nvme_queue *nvmeq = iod->nvmeq;
> struct nvme_dev *dev = nvmeq->dev;
> - struct request *abort_req;
> struct nvme_command cmd;
> - u32 csts = readl(dev->bar + NVME_REG_CSTS);
> -
> - /*
> - * Reset immediately if the controller is failed
> - */
> - if (nvme_should_reset(dev, csts)) {
> - nvme_warn_reset(dev, csts);
> - nvme_dev_disable(dev, false);
> - nvme_reset_ctrl(&dev->ctrl);
> - return BLK_EH_HANDLED;
> - }
> -
> - /*
> - * Did we miss an interrupt?
> - */
> - if (__nvme_poll(nvmeq, req->tag)) {
> - dev_warn(dev->ctrl.device,
> - "I/O %d QID %d timeout, completion polled\n",
> - req->tag, nvmeq->qid);
> - return BLK_EH_HANDLED;
> - }
> -
> - /*
> - * Shutdown immediately if controller times out while starting. The
> - * reset work will see the pci device disabled when it gets the forced
> - * cancellation error. All outstanding requests are completed on
> - * shutdown, so we return BLK_EH_HANDLED.
> - */
> - switch (dev->ctrl.state) {
> - case NVME_CTRL_CONNECTING:
> - case NVME_CTRL_RESETTING:
> - dev_warn(dev->ctrl.device,
> - "I/O %d QID %d timeout, disable controller\n",
> - req->tag, nvmeq->qid);
> - nvme_dev_disable(dev, false);
> - set_bit(NVME_REQ_CANCELLED, &nvme_req(req)->flags);
> - return BLK_EH_HANDLED;
> - default:
> - break;
> - }
> -
> - /*
> - * Shutdown the controller immediately and schedule a reset if the
> - * command was already aborted once before and still hasn't been
> - * returned to the driver, or if this is the admin queue.
> - */
> - if (!nvmeq->qid || iod->aborted) {
> - dev_warn(dev->ctrl.device,
> - "I/O %d QID %d timeout, reset controller\n",
> - req->tag, nvmeq->qid);
> - nvme_dev_disable(dev, false);
> - nvme_reset_ctrl(&dev->ctrl);
> -
> - /*
> - * Mark the request as handled, since the inline shutdown
> - * forces all outstanding requests to complete.
> - */
> - set_bit(NVME_REQ_CANCELLED, &nvme_req(req)->flags);
> - return BLK_EH_HANDLED;
> - }
> + struct request *abort_req;
>
> if (atomic_dec_return(&dev->ctrl.abort_limit) < 0) {
> atomic_inc(&dev->ctrl.abort_limit);
> @@ -1282,6 +1224,105 @@ static enum blk_eh_timer_return nvme_timeout(struct
> request *req, bool reserved)
> return BLK_EH_RESET_TIMER;
> }
>
> +static enum blk_eh_timer_return nvme_timeout(struct request *req, bool
> reserved)
> +{
> + struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> + struct nvme_queue *nvmeq = iod->nvmeq;
> + struct nvme_dev *dev = nvmeq->dev;
> + u32 csts = readl(dev->bar + NVME_REG_CSTS);
> + enum {ABORT, RESET, DISABLE} action;
> + enum blk_eh_timer_return ret;
> + /*
> + * This is for nvme_abort_req. This request will be completed
> + * by nvme_flush_aborted_requests after the controller is
> + * disabled/shutdown
> + */
> + if (test_bit(NVME_REQ_ABORTED, &nvme_req(req)->flags))
> + return BLK_EH_NOT_HANDLED;
> +
> + /*
> + * Reset immediately if the controller is failed.
> + * Defer the completion to nvme_flush_aborted_requests.
> + */
> + if (nvme_should_reset(dev, csts)) {
> + nvme_warn_reset(dev, csts);
> + nvme_reset_ctrl(&dev->ctrl);
> + return BLK_EH_RESET_TIMER;
> + }
> +
> + /*
> + * Did we miss an interrupt?
> + */
> + if (__nvme_poll(nvmeq, req->tag)) {
> + dev_warn(dev->ctrl.device,
> + "I/O %d QID %d timeout, completion polled\n",
> + req->tag, nvmeq->qid);
> + return BLK_EH_HANDLED;
> + }
> +
> + if (nvmeq->qid) {
> + if (dev->ctrl.state == NVME_CTRL_RESETTING ||
> + iod->aborted)
> + action = RESET;
> + else
> + action = ABORT;
> + } else {
> + /*
> + * Disable immediately if controller times out while disabling/
> + * shuting down/starting. The nvme_dev_disable/nvme_reset_work
> + * will see the error.
> + * Note: inflight_flushed is set in nvme_dev_disable when it
> + * abort all the inflight requests. Introducing this flag is due
> + * to there is no state to represent the shutdown procedure.
> + */
> + if (dev->ctrl.state == NVME_CTRL_CONNECTING ||
> + dev->inflight_flushed)
> + action = DISABLE;
> + else
> + action = RESET;
> + }
> +
> + switch (action) {
> + case ABORT:
> + ret = nvme_pci_abort_io_req(req);
> + break;
> + case RESET:
> + dev_warn(dev->ctrl.device,
> + "I/O %d QID %d timeout, reset controller\n",
> + req->tag, nvmeq->qid);
> + set_bit(NVME_REQ_CANCELLED, &nvme_req(req)->flags);
> + nvme_reset_ctrl(&dev->ctrl);
> + /*
> + * The reset work will take over this request. nvme_abort_req
> + * employs blk_abort_request to force the request to be timed
> + * out. So we need to return BLK_EH_RESET_TIMER then the
> + * RQF_MQ_TIMEOUT_EXPIRED could be cleared.
> + */
> + ret = BLK_EH_RESET_TIMER;
> + break;
> + case DISABLE:
> + /*
> + * We disable the controller directly here so that we could
> + * complete the request safely to wake up the nvme_dev_disable/
> + * nvme_reset_work who is waiting on adminq. We cannot return
> + * BLK_EH_RESET_TIMER to depend on error recovery procedure
> + * because it is waiting for timeout path.
> + */
> + dev_warn(dev->ctrl.device,
> + "I/O %d QID %d timeout, disable controller\n",
> + req->tag, nvmeq->qid);
> + nvme_pci_disable_ctrl_directly(dev);
> + set_bit(NVME_REQ_CANCELLED, &nvme_req(req)->flags);
> + ret = BLK_EH_HANDLED;
> + break;
> + default:
> + WARN_ON(1);
> + break;
> + }
> +
> + return ret;
> +}
> +
> static void nvme_free_queue(struct nvme_queue *nvmeq)
> {
> dma_free_coherent(nvmeq->q_dmadev, CQ_SIZE(nvmeq->q_depth),
> @@ -2169,6 +2210,33 @@ static void nvme_pci_disable(struct nvme_dev *dev)
> }
> }
>
> +/*
> + * This is only invoked by nvme_timeout. shutdown_lock is not needed
> + * here because nvme_abort_requests_sync in nvme_dev_disable will
> + * synchronize with the timeout path.
> + */
> +static void nvme_pci_disable_ctrl_directly(struct nvme_dev *dev)
> +{
> + int i;
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> + bool dead = true;
> +
> + if (pci_is_enabled(pdev)) {
> + u32 csts = readl(dev->bar + NVME_REG_CSTS);
> +
> + dead = !!((csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY) ||
> + pdev->error_state != pci_channel_io_normal);
> +
> + if (!dead)
> + nvme_disable_ctrl(&dev->ctrl, dev->ctrl.cap);
> + }
> +
> + for (i = dev->ctrl.queue_count - 1; i >= 0; i--)
> + nvme_suspend_queue(&dev->queues[i]);
> +
> + nvme_pci_disable(dev);
> +}
> +
> static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
> {
> int i;
> @@ -2205,6 +2273,8 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool
> shutdown)
>
> }
> nvme_stop_queues(&dev->ctrl);
> + nvme_abort_requests_sync(&dev->ctrl);
> + dev->inflight_flushed = true;
>
> if (!dead) {
> nvme_disable_io_queues(dev);
> @@ -2215,9 +2285,8 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool
> shutdown)
>
> nvme_pci_disable(dev);
>
> - blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
> - blk_mq_tagset_busy_iter(&dev->admin_tagset, nvme_cancel_request,
> &dev->ctrl);
> -
> + nvme_flush_aborted_requests(&dev->ctrl);
> + dev->inflight_flushed = false;
> /*
> * The driver will not be starting up queues again if shutting down so
> * must flush all entered requests to their failed completion to avoid
>
