From 4fb135ad154060bf289ccf35734e3d3b6e5029a7 Mon Sep 17 00:00:00 2001 From: Johannes Thumshirn Date: Thu, 19 Apr 2018 19:43:42 +0200 Subject: [PATCH 01/15] nvme: fc: provide a descriptive error Provide a descriptive error in case an lport to rport association isn't found when creating the FC-NVME controller. Currently it's very hard to debug the reason for a failed connect attempt without a look at the source. Signed-off-by: Johannes Thumshirn Reviewed-by: James Smart Reviewed-by: Hannes Reinecke Signed-off-by: Keith Busch --- drivers/nvme/host/fc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c index 6cb26bcf6ec0..8b66879b4ebf 100644 --- a/drivers/nvme/host/fc.c +++ b/drivers/nvme/host/fc.c @@ -3284,6 +3284,8 @@ nvme_fc_create_ctrl(struct device *dev, struct nvmf_ctrl_options *opts) } spin_unlock_irqrestore(&nvme_fc_lock, flags); + pr_warn("%s: %s - %s combination not found\n", + __func__, opts->traddr, opts->host_traddr); return ERR_PTR(-ENOENT); } From cde6bf4f440631b2735be83c39426f4f469b0b21 Mon Sep 17 00:00:00 2001 From: Johannes Thumshirn Date: Thu, 26 Apr 2018 14:25:15 -0600 Subject: [PATCH 02/15] nvme: change order of qid and cmdid in completion trace Keith reported that command submission and command completion tracepoints have the order of the cmdid and qid fields swapped. While it isn't easily possible to change the command submission tracepoint, as there is a regression test parsing it in blktests we can swap the command completion tracepoint to have the fields aligned. Signed-off-by: Johannes Thumshirn Reported-by: Keith Busch Reviewed-by: Jens Axboe Signed-off-by: Keith Busch --- drivers/nvme/host/trace.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h index ea91fccd1bc0..01390f0e1671 100644 --- a/drivers/nvme/host/trace.h +++ b/drivers/nvme/host/trace.h @@ -148,8 +148,8 @@ TRACE_EVENT(nvme_complete_rq, __entry->flags = nvme_req(req)->flags; __entry->status = nvme_req(req)->status; ), - TP_printk("cmdid=%u, qid=%d, res=%llu, retries=%u, flags=0x%x, status=%u", - __entry->cid, __entry->qid, __entry->result, + TP_printk("qid=%d, cmdid=%u, res=%llu, retries=%u, flags=0x%x, status=%u", + __entry->qid, __entry->cid, __entry->result, __entry->retries, __entry->flags, __entry->status) ); From 0302ae60fc2368de5c561be49e2d9ea26dd462de Mon Sep 17 00:00:00 2001 From: Micah Parrish Date: Thu, 12 Apr 2018 13:25:25 -0600 Subject: [PATCH 03/15] NVMe: Add Quirk Delay before CHK RDY for Seagate Nytro Flash Storage Add Seagate Nytro Flash Storage nvme drive to quirk list for NVME_QUIRK_DELAY_BEFORE_CHK_RDY, which solves a bug where the drive is probed on hot-add before the firmare is ready, I/O errors are generated while reading sector 0, and linux is "unable to read partition table". Signed-off-by: Micah Parrish Reviewed-by: Martin K. Petersen Signed-off-by: Keith Busch --- drivers/nvme/host/pci.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index fbc71fac6f1e..af88f3fe2cc0 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2704,6 +2704,8 @@ static const struct pci_device_id nvme_id_table[] = { .driver_data = NVME_QUIRK_NO_DEEPEST_PS }, { PCI_VDEVICE(INTEL, 0x5845), /* Qemu emulated controller */ .driver_data = NVME_QUIRK_IDENTIFY_CNS, }, + { PCI_DEVICE(0x1bb1, 0x0100), /* Seagate Nytro Flash Storage */ + .driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, }, { PCI_DEVICE(0x1c58, 0x0003), /* HGST adapter */ .driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, }, { PCI_DEVICE(0x1c58, 0x0023), /* WDC SN200 adapter */ From ea48e877994f086af481427bac110aa63686c3ce Mon Sep 17 00:00:00 2001 From: Wei Xu Date: Thu, 26 Apr 2018 14:59:19 -0600 Subject: [PATCH 04/15] nvme: lightnvm: add granby support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a new lightnvm quirk to identify CNEX’s Granby controller. Signed-off-by: Wei Xu Reviewed-by: Javier González Reviewed-by: Matias Bjørling Signed-off-by: Keith Busch --- drivers/nvme/host/pci.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index af88f3fe2cc0..8a86dbb0583d 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2720,6 +2720,8 @@ static const struct pci_device_id nvme_id_table[] = { .driver_data = NVME_QUIRK_LIGHTNVM, }, { PCI_DEVICE(0x1d1d, 0x2807), /* CNEX WL */ .driver_data = NVME_QUIRK_LIGHTNVM, }, + { PCI_DEVICE(0x1d1d, 0x2601), /* CNEX Granby */ + .driver_data = NVME_QUIRK_LIGHTNVM, }, { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) }, { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) }, { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) }, From 1811977568e0f59d145da087e45f3dca09dab1c3 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Fri, 27 Apr 2018 13:42:52 -0600 Subject: [PATCH 05/15] nvme/pci: Use async_schedule for initial reset work This patch schedules the initial controller reset in an async_domain so that it can be synchronized from wait_for_device_probe(). This way the kernel waits for the initial nvme controller scan to complete for all devices before proceeding with the boot sequence, which may have nvme dependencies. Reported-by: Mikulas Patocka Tested-by: Mikulas Patocka Signed-off-by: Keith Busch --- drivers/nvme/host/pci.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 8a86dbb0583d..dcd1be005eef 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -13,6 +13,7 @@ */ #include +#include #include #include #include @@ -2488,6 +2489,13 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev) return 0; } +static void nvme_async_probe(void *data, async_cookie_t cookie) +{ + struct nvme_dev *dev = data; + nvme_reset_ctrl_sync(&dev->ctrl); + flush_work(&dev->ctrl.scan_work); +} + static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id) { int node, result = -ENOMEM; @@ -2532,7 +2540,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)); - nvme_reset_ctrl(&dev->ctrl); + async_schedule(nvme_async_probe, dev); return 0; From 3831761eb859f5599a522f064007b31100510c3a Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Wed, 2 May 2018 11:06:54 -0600 Subject: [PATCH 06/15] nvme: only reconfigure discard if necessary Currently nvme reconfigures discard for every disk revalidation. This is problematic because any O_WRONLY or O_RDWR open will trigger a partition scan through udev/systemd, and we will reconfigure discard. This blows away any user settings, like discard_max_bytes. Only re-configure the user settable settings if we need to. Signed-off-by: Jens Axboe [removed redundant queue flag setting] Signed-off-by: Keith Busch --- drivers/nvme/host/core.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 9df4f71e58ca..62262fac7a5d 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1347,13 +1347,19 @@ static void nvme_set_chunk_size(struct nvme_ns *ns) blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(chunk_size)); } -static void nvme_config_discard(struct nvme_ctrl *ctrl, - unsigned stream_alignment, struct request_queue *queue) +static void nvme_config_discard(struct nvme_ns *ns) { + struct nvme_ctrl *ctrl = ns->ctrl; + struct request_queue *queue = ns->queue; u32 size = queue_logical_block_size(queue); - if (stream_alignment) - size *= stream_alignment; + if (!(ctrl->oncs & NVME_CTRL_ONCS_DSM)) { + blk_queue_flag_clear(QUEUE_FLAG_DISCARD, queue); + return; + } + + if (ctrl->nr_streams && ns->sws && ns->sgs) + size *= ns->sws * ns->sgs; BUILD_BUG_ON(PAGE_SIZE / sizeof(struct nvme_dsm_range) < NVME_DSM_MAX_RANGES); @@ -1361,9 +1367,12 @@ static void nvme_config_discard(struct nvme_ctrl *ctrl, queue->limits.discard_alignment = 0; queue->limits.discard_granularity = size; + /* If discard is already enabled, don't reset queue limits */ + if (blk_queue_flag_test_and_set(QUEUE_FLAG_DISCARD, queue)) + return; + blk_queue_max_discard_sectors(queue, UINT_MAX); blk_queue_max_discard_segments(queue, NVME_DSM_MAX_RANGES); - blk_queue_flag_set(QUEUE_FLAG_DISCARD, queue); if (ctrl->quirks & NVME_QUIRK_DEALLOCATE_ZEROES) blk_queue_max_write_zeroes_sectors(queue, UINT_MAX); @@ -1407,10 +1416,6 @@ static void nvme_update_disk_info(struct gendisk *disk, { sector_t capacity = le64_to_cpup(&id->nsze) << (ns->lba_shift - 9); unsigned short bs = 1 << ns->lba_shift; - unsigned stream_alignment = 0; - - if (ns->ctrl->nr_streams && ns->sws && ns->sgs) - stream_alignment = ns->sws * ns->sgs; blk_mq_freeze_queue(disk->queue); blk_integrity_unregister(disk); @@ -1424,10 +1429,9 @@ static void nvme_update_disk_info(struct gendisk *disk, nvme_init_integrity(disk, ns->ms, ns->pi_type); if (ns->ms && !nvme_ns_has_pi(ns) && !blk_get_integrity(disk)) capacity = 0; - set_capacity(disk, capacity); - if (ns->ctrl->oncs & NVME_CTRL_ONCS_DSM) - nvme_config_discard(ns->ctrl, stream_alignment, disk->queue); + set_capacity(disk, capacity); + nvme_config_discard(ns); blk_mq_unfreeze_queue(disk->queue); } From 80f513b5056d0bf127653d2327b7b24e322dc7e3 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Mon, 7 May 2018 08:30:24 -0600 Subject: [PATCH 07/15] nvme/pci: Hold controller reference during async probe It is possible the driver's remove may have freed the controller if the remove callback is invoked prior to the async_schedule starting the reset_work. This patch fixes that by holding a reference on the controller. Reported-by: Mikulas Patocka Signed-off-by: Keith Busch --- drivers/nvme/host/pci.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index dcd1be005eef..7acecdf25621 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2492,8 +2492,10 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev) static void nvme_async_probe(void *data, async_cookie_t cookie) { struct nvme_dev *dev = data; + nvme_reset_ctrl_sync(&dev->ctrl); flush_work(&dev->ctrl.scan_work); + nvme_put_ctrl(&dev->ctrl); } static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id) @@ -2540,6 +2542,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)); + nvme_get_ctrl(&dev->ctrl); async_schedule(nvme_async_probe, dev); return 0; From cc1d5e749a2e1cf59fa940b976181e631d6985e1 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Thu, 10 May 2018 08:34:20 -0600 Subject: [PATCH 08/15] nvme/pci: Sync controller reset for AER slot_reset AER handling expects a successful return from slot_reset means the driver made the device functional again. The nvme driver had been using an asynchronous reset to recover the device, so the device may still be initializing after control is returned to the AER handler. This creates problems for subsequent event handling, causing the initializion to fail. This patch fixes that by syncing the controller reset before returning to the AER driver, and reporting the true state of the reset. Link: https://bugzilla.kernel.org/show_bug.cgi?id=199657 Reported-by: Alex Gagniuc Cc: Sinan Kaya Cc: Bjorn Helgaas Cc: stable@vger.kernel.org Tested-by: Alex Gagniuc Reviewed-by: Christoph Hellwig Reviewed-by: Martin K. Petersen Signed-off-by: Keith Busch --- drivers/nvme/host/pci.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 7acecdf25621..b58aebb347a0 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2681,8 +2681,15 @@ 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); - nvme_reset_ctrl(&dev->ctrl); - return PCI_ERS_RESULT_RECOVERED; + nvme_reset_ctrl_sync(&dev->ctrl); + + switch (dev->ctrl.state) { + case NVME_CTRL_LIVE: + case NVME_CTRL_ADMIN_ONLY: + return PCI_ERS_RESULT_RECOVERED; + default: + return PCI_ERS_RESULT_DISCONNECT; + } } static void nvme_error_resume(struct pci_dev *pdev) From 287a63ebbe7c0c1e1c16f7462b4129edf5cd08df Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Thu, 17 May 2018 18:31:46 +0200 Subject: [PATCH 09/15] nvme: mark the result argument to nvme_complete_async_event volatile We'll need that in the PCIe driver soon as we'll read it straight off the CQ. Signed-off-by: Christoph Hellwig --- drivers/nvme/host/core.c | 2 +- drivers/nvme/host/nvme.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 62262fac7a5d..b070c659391f 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -3344,7 +3344,7 @@ static void nvme_fw_act_work(struct work_struct *work) } void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, - union nvme_result *res) + volatile union nvme_result *res) { u32 result = le32_to_cpu(res->u32); diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 061fecfd44f5..7a872b0d53a7 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -400,7 +400,7 @@ int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len, bool send); void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, - union nvme_result *res); + volatile union nvme_result *res); void nvme_stop_queues(struct nvme_ctrl *ctrl); void nvme_start_queues(struct nvme_ctrl *ctrl); From 750dde4472e48eadf28221b7eb02d493db1bcfd0 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 18 May 2018 08:37:04 -0600 Subject: [PATCH 10/15] nvme-pci: simplify nvme_cqe_valid We always look at the current CQ head and phase, so don't pass these as separate arguments, and rename the function to nvme_cqe_pending. Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/pci.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index b58aebb347a0..cf36dd39f2a5 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -915,10 +915,10 @@ static void nvme_pci_complete_rq(struct request *req) } /* We read the CQE phase first to check if the rest of the entry is valid */ -static inline bool nvme_cqe_valid(struct nvme_queue *nvmeq, u16 head, - u16 phase) +static inline bool nvme_cqe_pending(struct nvme_queue *nvmeq) { - return (le16_to_cpu(nvmeq->cqes[head].status) & 1) == phase; + return (le16_to_cpu(nvmeq->cqes[nvmeq->cq_head].status) & 1) == + nvmeq->cq_phase; } static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq) @@ -965,7 +965,7 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, static inline bool nvme_read_cqe(struct nvme_queue *nvmeq, struct nvme_completion *cqe) { - if (nvme_cqe_valid(nvmeq, nvmeq->cq_head, nvmeq->cq_phase)) { + if (nvme_cqe_pending(nvmeq)) { *cqe = nvmeq->cqes[nvmeq->cq_head]; if (++nvmeq->cq_head == nvmeq->q_depth) { @@ -1006,7 +1006,7 @@ static irqreturn_t nvme_irq(int irq, void *data) static irqreturn_t nvme_irq_check(int irq, void *data) { struct nvme_queue *nvmeq = data; - if (nvme_cqe_valid(nvmeq, nvmeq->cq_head, nvmeq->cq_phase)) + if (nvme_cqe_pending(nvmeq)) return IRQ_WAKE_THREAD; return IRQ_NONE; } @@ -1016,7 +1016,7 @@ static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag) struct nvme_completion cqe; int found = 0, consumed = 0; - if (!nvme_cqe_valid(nvmeq, nvmeq->cq_head, nvmeq->cq_phase)) + if (!nvme_cqe_pending(nvmeq)) return 0; spin_lock_irq(&nvmeq->q_lock); From f9dde187fa921c12a8680089a77595b866e65455 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Thu, 17 May 2018 18:31:48 +0200 Subject: [PATCH 11/15] nvme-pci: remove cq check after submission We always check the completion queue after submitting, but in my testing this isn't a win even on DRAM/xpoint devices. In some cases it's actually worse. Kill it. Signed-off-by: Jens Axboe Signed-off-by: Keith Busch Signed-off-by: Christoph Hellwig --- drivers/nvme/host/pci.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index cf36dd39f2a5..577570d3e1f4 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -69,7 +69,6 @@ MODULE_PARM_DESC(io_queue_depth, "set io queue depth, should >= 2"); struct nvme_dev; struct nvme_queue; -static void nvme_process_cq(struct nvme_queue *nvmeq); static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown); /* @@ -896,7 +895,6 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, goto out_cleanup_iod; } __nvme_submit_cmd(nvmeq, &cmnd); - nvme_process_cq(nvmeq); spin_unlock_irq(&nvmeq->q_lock); return BLK_STS_OK; out_cleanup_iod: From d1f06f4ae0410f8e5025f3c9129a52b86579e174 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Thu, 17 May 2018 18:31:49 +0200 Subject: [PATCH 12/15] nvme-pci: move ->cq_vector == -1 check outside of ->q_lock We only clear it dynamically in nvme_suspend_queue(). When we do, ensure to do a full flush so that any nvme_queue_rq() invocation will see it. Ideally we'd kill this check completely, but we're using it to flush requests on a dying queue. Signed-off-by: Jens Axboe Signed-off-by: Christoph Hellwig --- drivers/nvme/host/pci.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 577570d3e1f4..3dfedc84a921 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -872,6 +872,13 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, struct nvme_command cmnd; blk_status_t ret; + /* + * We should not need to do this, but we're still using this to + * ensure we can drain requests on a dying queue. + */ + if (unlikely(nvmeq->cq_vector < 0)) + return BLK_STS_IOERR; + ret = nvme_setup_cmd(ns, req, &cmnd); if (ret) return ret; @@ -889,11 +896,6 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, blk_mq_start_request(req); spin_lock_irq(&nvmeq->q_lock); - if (unlikely(nvmeq->cq_vector < 0)) { - ret = BLK_STS_IOERR; - spin_unlock_irq(&nvmeq->q_lock); - goto out_cleanup_iod; - } __nvme_submit_cmd(nvmeq, &cmnd); spin_unlock_irq(&nvmeq->q_lock); return BLK_STS_OK; @@ -1321,6 +1323,12 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq) nvmeq->cq_vector = -1; spin_unlock_irq(&nvmeq->q_lock); + /* + * Ensure that nvme_queue_rq() sees it ->cq_vector == -1 without + * having to grab the lock. + */ + mb(); + if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q) blk_mq_quiesce_queue(nvmeq->dev->ctrl.admin_q); From 5cb525c8315f1dd9232b59cd1cf1e0f19ff1a5df Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Thu, 17 May 2018 18:31:50 +0200 Subject: [PATCH 13/15] nvme-pci: handle completions outside of the queue lock Split the completion of events into a two part process: 1) Reap the events inside the queue lock 2) Complete the events outside the queue lock Since we never wrap the queue, we can access it locklessly after we've updated the completion queue head. This patch started off with batching events on the stack, but with this trick we don't have to. Keith Busch came up with that idea. Note that this kills the ->cqe_seen as well. I haven't been able to trigger any ill effects of this. If we do race with polling every so often, it should be rare enough NOT to trigger any issues. Signed-off-by: Jens Axboe Signed-off-by: Keith Busch [hch: refactored, restored poll early exit optimization] Signed-off-by: Christoph Hellwig --- drivers/nvme/host/pci.c | 91 +++++++++++++++++++++-------------------- 1 file changed, 47 insertions(+), 44 deletions(-) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 3dfedc84a921..7fbb6f94b561 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -161,7 +161,6 @@ struct nvme_queue { u16 cq_head; u16 qid; u8 cq_phase; - u8 cqe_seen; u32 *dbbuf_sq_db; u32 *dbbuf_cq_db; u32 *dbbuf_sq_ei; @@ -932,9 +931,9 @@ static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq) } } -static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, - struct nvme_completion *cqe) +static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx) { + volatile struct nvme_completion *cqe = &nvmeq->cqes[idx]; struct request *req; if (unlikely(cqe->command_id >= nvmeq->q_depth)) { @@ -957,50 +956,58 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, return; } - nvmeq->cqe_seen = 1; req = blk_mq_tag_to_rq(*nvmeq->tags, cqe->command_id); nvme_end_request(req, cqe->status, cqe->result); } -static inline bool nvme_read_cqe(struct nvme_queue *nvmeq, - struct nvme_completion *cqe) +static void nvme_complete_cqes(struct nvme_queue *nvmeq, u16 start, u16 end) { - if (nvme_cqe_pending(nvmeq)) { - *cqe = nvmeq->cqes[nvmeq->cq_head]; - - if (++nvmeq->cq_head == nvmeq->q_depth) { - nvmeq->cq_head = 0; - nvmeq->cq_phase = !nvmeq->cq_phase; - } - return true; + while (start != end) { + nvme_handle_cqe(nvmeq, start); + if (++start == nvmeq->q_depth) + start = 0; } - return false; } -static void nvme_process_cq(struct nvme_queue *nvmeq) +static inline void nvme_update_cq_head(struct nvme_queue *nvmeq) { - struct nvme_completion cqe; - int consumed = 0; - - while (nvme_read_cqe(nvmeq, &cqe)) { - nvme_handle_cqe(nvmeq, &cqe); - consumed++; + if (++nvmeq->cq_head == nvmeq->q_depth) { + nvmeq->cq_head = 0; + nvmeq->cq_phase = !nvmeq->cq_phase; } +} - if (consumed) +static inline bool nvme_process_cq(struct nvme_queue *nvmeq, u16 *start, + u16 *end, int tag) +{ + bool found = false; + + *start = nvmeq->cq_head; + while (!found && nvme_cqe_pending(nvmeq)) { + if (nvmeq->cqes[nvmeq->cq_head].command_id == tag) + found = true; + nvme_update_cq_head(nvmeq); + } + *end = nvmeq->cq_head; + + if (*start != *end) nvme_ring_cq_doorbell(nvmeq); + return found; } static irqreturn_t nvme_irq(int irq, void *data) { - irqreturn_t result; struct nvme_queue *nvmeq = data; + u16 start, end; + spin_lock(&nvmeq->q_lock); - nvme_process_cq(nvmeq); - result = nvmeq->cqe_seen ? IRQ_HANDLED : IRQ_NONE; - nvmeq->cqe_seen = 0; + nvme_process_cq(nvmeq, &start, &end, -1); spin_unlock(&nvmeq->q_lock); - return result; + + if (start == end) + return IRQ_NONE; + nvme_complete_cqes(nvmeq, start, end); + return IRQ_HANDLED; } static irqreturn_t nvme_irq_check(int irq, void *data) @@ -1013,27 +1020,17 @@ static irqreturn_t nvme_irq_check(int irq, void *data) static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag) { - struct nvme_completion cqe; - int found = 0, consumed = 0; + u16 start, end; + bool found; if (!nvme_cqe_pending(nvmeq)) return 0; spin_lock_irq(&nvmeq->q_lock); - while (nvme_read_cqe(nvmeq, &cqe)) { - nvme_handle_cqe(nvmeq, &cqe); - consumed++; - - if (tag == cqe.command_id) { - found = 1; - break; - } - } - - if (consumed) - nvme_ring_cq_doorbell(nvmeq); + found = nvme_process_cq(nvmeq, &start, &end, tag); spin_unlock_irq(&nvmeq->q_lock); + nvme_complete_cqes(nvmeq, start, end); return found; } @@ -1340,6 +1337,7 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq) static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown) { struct nvme_queue *nvmeq = &dev->queues[0]; + u16 start, end; if (shutdown) nvme_shutdown_ctrl(&dev->ctrl); @@ -1347,8 +1345,10 @@ static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown) nvme_disable_ctrl(&dev->ctrl, dev->ctrl.cap); spin_lock_irq(&nvmeq->q_lock); - nvme_process_cq(nvmeq); + nvme_process_cq(nvmeq, &start, &end, -1); spin_unlock_irq(&nvmeq->q_lock); + + nvme_complete_cqes(nvmeq, start, end); } static int nvme_cmb_qdepth(struct nvme_dev *dev, int nr_io_queues, @@ -1995,6 +1995,7 @@ static void nvme_del_queue_end(struct request *req, blk_status_t error) static void nvme_del_cq_end(struct request *req, blk_status_t error) { struct nvme_queue *nvmeq = req->end_io_data; + u16 start, end; if (!error) { unsigned long flags; @@ -2006,8 +2007,10 @@ static void nvme_del_cq_end(struct request *req, blk_status_t error) */ spin_lock_irqsave_nested(&nvmeq->q_lock, flags, SINGLE_DEPTH_NESTING); - nvme_process_cq(nvmeq); + nvme_process_cq(nvmeq, &start, &end, -1); spin_unlock_irqrestore(&nvmeq->q_lock, flags); + + nvme_complete_cqes(nvmeq, start, end); } nvme_del_queue_end(req, error); From 1ab0cd6966fc4a7e9dfbd7c6eda917ae9c977f42 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Thu, 17 May 2018 18:31:51 +0200 Subject: [PATCH 14/15] nvme-pci: split the nvme queue lock into submission and completion locks This is now feasible. We protect the submission queue ring with ->sq_lock, and the completion side with ->cq_lock. Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe Signed-off-by: Christoph Hellwig --- drivers/nvme/host/pci.c | 44 +++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 7fbb6f94b561..1b49b694a57a 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -147,9 +147,10 @@ static inline struct nvme_dev *to_nvme_dev(struct nvme_ctrl *ctrl) struct nvme_queue { struct device *q_dmadev; struct nvme_dev *dev; - spinlock_t q_lock; + spinlock_t sq_lock; struct nvme_command *sq_cmds; struct nvme_command __iomem *sq_cmds_io; + spinlock_t cq_lock ____cacheline_aligned_in_smp; volatile struct nvme_completion *cqes; struct blk_mq_tags **tags; dma_addr_t sq_dma_addr; @@ -894,9 +895,9 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, blk_mq_start_request(req); - spin_lock_irq(&nvmeq->q_lock); + spin_lock_irq(&nvmeq->sq_lock); __nvme_submit_cmd(nvmeq, &cmnd); - spin_unlock_irq(&nvmeq->q_lock); + spin_unlock_irq(&nvmeq->sq_lock); return BLK_STS_OK; out_cleanup_iod: nvme_free_iod(dev, req); @@ -1000,9 +1001,9 @@ static irqreturn_t nvme_irq(int irq, void *data) struct nvme_queue *nvmeq = data; u16 start, end; - spin_lock(&nvmeq->q_lock); + spin_lock(&nvmeq->cq_lock); nvme_process_cq(nvmeq, &start, &end, -1); - spin_unlock(&nvmeq->q_lock); + spin_unlock(&nvmeq->cq_lock); if (start == end) return IRQ_NONE; @@ -1026,9 +1027,9 @@ static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag) if (!nvme_cqe_pending(nvmeq)) return 0; - spin_lock_irq(&nvmeq->q_lock); + spin_lock_irq(&nvmeq->cq_lock); found = nvme_process_cq(nvmeq, &start, &end, tag); - spin_unlock_irq(&nvmeq->q_lock); + spin_unlock_irq(&nvmeq->cq_lock); nvme_complete_cqes(nvmeq, start, end); return found; @@ -1051,9 +1052,9 @@ static void nvme_pci_submit_async_event(struct nvme_ctrl *ctrl) c.common.opcode = nvme_admin_async_event; c.common.command_id = NVME_AQ_BLK_MQ_DEPTH; - spin_lock_irq(&nvmeq->q_lock); + spin_lock_irq(&nvmeq->sq_lock); __nvme_submit_cmd(nvmeq, &c); - spin_unlock_irq(&nvmeq->q_lock); + spin_unlock_irq(&nvmeq->sq_lock); } static int adapter_delete_queue(struct nvme_dev *dev, u8 opcode, u16 id) @@ -1310,15 +1311,15 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq) { int vector; - spin_lock_irq(&nvmeq->q_lock); + spin_lock_irq(&nvmeq->cq_lock); if (nvmeq->cq_vector == -1) { - spin_unlock_irq(&nvmeq->q_lock); + spin_unlock_irq(&nvmeq->cq_lock); return 1; } vector = nvmeq->cq_vector; nvmeq->dev->online_queues--; nvmeq->cq_vector = -1; - spin_unlock_irq(&nvmeq->q_lock); + spin_unlock_irq(&nvmeq->cq_lock); /* * Ensure that nvme_queue_rq() sees it ->cq_vector == -1 without @@ -1344,9 +1345,9 @@ static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown) else nvme_disable_ctrl(&dev->ctrl, dev->ctrl.cap); - spin_lock_irq(&nvmeq->q_lock); + spin_lock_irq(&nvmeq->cq_lock); nvme_process_cq(nvmeq, &start, &end, -1); - spin_unlock_irq(&nvmeq->q_lock); + spin_unlock_irq(&nvmeq->cq_lock); nvme_complete_cqes(nvmeq, start, end); } @@ -1406,7 +1407,8 @@ static int nvme_alloc_queue(struct nvme_dev *dev, int qid, int depth) nvmeq->q_dmadev = dev->dev; nvmeq->dev = dev; - spin_lock_init(&nvmeq->q_lock); + spin_lock_init(&nvmeq->sq_lock); + spin_lock_init(&nvmeq->cq_lock); nvmeq->cq_head = 0; nvmeq->cq_phase = 1; nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride]; @@ -1442,7 +1444,7 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid) { struct nvme_dev *dev = nvmeq->dev; - spin_lock_irq(&nvmeq->q_lock); + spin_lock_irq(&nvmeq->cq_lock); nvmeq->sq_tail = 0; nvmeq->cq_head = 0; nvmeq->cq_phase = 1; @@ -1450,7 +1452,7 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid) memset((void *)nvmeq->cqes, 0, CQ_SIZE(nvmeq->q_depth)); nvme_dbbuf_init(dev, nvmeq, qid); dev->online_queues++; - spin_unlock_irq(&nvmeq->q_lock); + spin_unlock_irq(&nvmeq->cq_lock); } static int nvme_create_queue(struct nvme_queue *nvmeq, int qid) @@ -2001,14 +2003,14 @@ static void nvme_del_cq_end(struct request *req, blk_status_t error) unsigned long flags; /* - * We might be called with the AQ q_lock held - * and the I/O queue q_lock should always + * We might be called with the AQ cq_lock held + * and the I/O queue cq_lock should always * nest inside the AQ one. */ - spin_lock_irqsave_nested(&nvmeq->q_lock, flags, + spin_lock_irqsave_nested(&nvmeq->cq_lock, flags, SINGLE_DEPTH_NESTING); nvme_process_cq(nvmeq, &start, &end, -1); - spin_unlock_irqrestore(&nvmeq->q_lock, flags); + spin_unlock_irqrestore(&nvmeq->cq_lock, flags); nvme_complete_cqes(nvmeq, start, end); } From 1eae349d18fc7b8e7d88673f7194d7a542fdb25c Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Thu, 17 May 2018 18:31:52 +0200 Subject: [PATCH 15/15] nvme-pci: drop IRQ disabling on submission queue lock Since we aren't sharing the lock for completions now, we don't have to make it IRQ safe. Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe Signed-off-by: Christoph Hellwig --- drivers/nvme/host/pci.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 1b49b694a57a..133fc063d10c 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -895,9 +895,9 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, blk_mq_start_request(req); - spin_lock_irq(&nvmeq->sq_lock); + spin_lock(&nvmeq->sq_lock); __nvme_submit_cmd(nvmeq, &cmnd); - spin_unlock_irq(&nvmeq->sq_lock); + spin_unlock(&nvmeq->sq_lock); return BLK_STS_OK; out_cleanup_iod: nvme_free_iod(dev, req); @@ -1052,9 +1052,9 @@ static void nvme_pci_submit_async_event(struct nvme_ctrl *ctrl) c.common.opcode = nvme_admin_async_event; c.common.command_id = NVME_AQ_BLK_MQ_DEPTH; - spin_lock_irq(&nvmeq->sq_lock); + spin_lock(&nvmeq->sq_lock); __nvme_submit_cmd(nvmeq, &c); - spin_unlock_irq(&nvmeq->sq_lock); + spin_unlock(&nvmeq->sq_lock); } static int adapter_delete_queue(struct nvme_dev *dev, u8 opcode, u16 id)