lkml.org 
[lkml]   [2018]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH V4 3/5] nvme-pci: avoid nvme_dev_disable to be invoked in nvme_timeout
    Date
    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.wang@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
    --
    2.7.4
    \
     
     \ /
      Last update: 2018-03-08 07:21    [W:3.751 / U:0.636 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site