nvme: don't schedule multiple resets
PATCH 1/2 nvme: don't schedule multiple resets
The queue_work only fails if the work is pending, but not yet running. If the work is running, the work item would get requeued, triggering a double reset. If the first reset fails for any reason, the second reset triggers:
WARN_ON(dev->ctrl.state == NVME_CTRL_RESETTING)
Hitting that schedules controller deletion for a second time, which potentially takes a reference on the device that is being deleted. If the reset occurs at the same time as a hot removal event, this causes a double-free.
This patch has the reset helper function check if the work is busy prior to queueing, and changes all places that schedule resets to use this function. Since most users don't want to sync with that work, the "flush_work" is moved to the only caller that wants to sync.
--- drivers/nvme/host/pci.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
drivers/nvme/host/pci.c +13 -9
@@ -892,7 +892,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 (dev);/* * Mark the request as handled, since the inline shutdown
@@ -1290,7 +1290,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 (dev))dev_warn (dev->dev,"Failed status: 0x%x, reset controller.\n" , csts);
@@ -1812,11 +1812,10 @@ static int nvme_reset(struct nvme_dev *dev) {if (!dev->ctrl.admin_q ||blk_queue_dying (dev->ctrl.admin_q))return -ENODEV ; - +if (work_busy (&dev->reset_work)) +return -ENODEV ;if (!queue_work (nvme_workq, &dev->reset_work))return -EBUSY ; - -flush_work (&dev->reset_work);return 0 ; }
@@ -1840,7 +1839,12 @@ 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)); +struct nvme_dev *dev =to_nvme_dev (ctrl); +int ret =nvme_reset (dev); + +if (!ret) +flush_work (&dev->reset_work); +return ret; }static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
@@ -1934,7 +1938,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 (dev); }static void nvme_shutdown (struct pci_dev *pdev)
@@ -2003,7 +2007,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 (ndev); return 0; }#endif
@@ -2042,7 +2046,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 (dev);return PCI_ERS_RESULT_RECOVERED ; }
Signature · 1 line
2.7.2
PATCH 2/2 nvme: Delete created IO queues on reset
Commit c21377f8 (Suspend all queues before deletion) decrements the online queue count prior to our attempt to delete those IO queues, so the driver ended up not having the controller delete any. This patch uses the queue_count instead of online_queues.
--- drivers/nvme/host/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/pci.c +1 -1
@@ -1512,7 +1512,7 @@ static int nvme_delete_queue(struct nvme_queue *nvmeq, u8 opcode)static void nvme_disable_io_queues (struct nvme_dev *dev) { -int pass, queues = dev->online_queues -1 ; +int pass, queues = dev->queue_count -1 ;unsigned long timeout;u8 opcode = nvme_admin_delete_sq;
Signature · 1 line
2.7.2
Sagi Grimberg
maintainerPATCH 2/2 nvme: Delete created IO queues on reset
Quoted 4 lines
Commit
c21377f8(Suspend all queues before deletion) decrements the online queue count prior to our attempt to delete those IO queues, so the driver ended up not having the controller delete any. This patch uses thequeue_countinstead ofonline_queues.
A fixes tag would be useful here.
Other than that looks good,
PATCH 2/2 nvme: Delete created IO queues on reset
In reply to Keith BuschQuoted 4 lines · Keith Busch
Commit
c21377f8(Suspend all queues before deletion) decrements the online queue count prior to our attempt to delete those IO queues, so the driver ended up not having the controller delete any. This patch uses thequeue_countinstead ofonline_queues.
hmm, this is obviously correct. Nice catch. Shame I missed it :(
Quoted 18 lines
Signed-off-by Keith Busch <keith.busch at intel.com>--- drivers/nvme/host/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/nvme/host/pci.c +1
-1@@ -1512,7 +1512,7 @@ static int nvme_delete_queue(struct nvme_queue *nvmeq, u8 opcode)static void nvme_disable_io_queues (struct nvme_dev *dev) { -int pass, queues = dev->online_queues -1 ; +int pass, queues = dev->queue_count -1 ;unsigned long timeout;u8 opcode = nvme_admin_delete_sq;
Signature · 1 line
Gabriel Krisman Bertazi
Christoph Hellwig
maintainerPATCH 2/2 nvme: Delete created IO queues on reset
In reply to Keith BuschQuoted 4 lines · Keith Busch
Commit
c21377f8(Suspend all queues before deletion) decrements the online queue count prior to our attempt to delete those IO queues, so the driver ended up not having the controller delete any. This patch uses thequeue_countinstead ofonline_queues.
What if not all queues were online before? Should we take a a snapshot of ->online_queues before suspending the queues and then use that later?
PATCH 2/2 nvme: Delete created IO queues on reset
Quoted 9 lines · Christoph Hellwig
Quoted 4 lines · Keith Busch
Commit
c21377f8(Suspend all queues before deletion) decrements the online queue count prior to our attempt to delete those IO queues, so the driver ended up not having the controller delete any. This patch uses thequeue_countinstead ofonline_queues.What if not all queues were online before? Should we take a a snapshot of ->online_queues before suspending the queues and then use that later?
That sounds good. In the worst case, using queue_count may attempt to delete a queue the adapter didn't create, which just returns an invalid QID error, and everything continues as normal. But it's easily avoidable, so I'll send a v2 of this one.
Sagi Grimberg
maintainerPATCH 1/2 nvme: don't schedule multiple resets
In reply to Keith BuschLooks good,
Christoph Hellwig
maintainerPATCH 1/2 nvme: don't schedule multiple resets
In reply to Keith BuschQuoted 12 lines · Keith Busch
The
queue_workonly fails if the work is pending, but not yet running. If the work is running, the work item would get requeued, triggering a double reset. If the first reset fails for any reason, the second reset triggers:WARN_ON(dev->ctrl.state == NVME_CTRL_RESETTING)Hitting that schedules controller deletion for a second time, which potentially takes a reference on the device that is being deleted. If the reset occurs at the same time as a hot removal event, this causes a double-free.
Quoted 4 lines
This patch has the reset helper function check if the work is busy prior to queueing, and changes all places that schedule resets to use this function. Since most users don't want to sync with that work, the "
flush_work" is moved to the only caller that wants to sync.
Looks fine. I actually have something very similar in an old branch, except that I also moved nvme_reset to common code and made the fabrics drivers use it. I'll really need to get back to that stuff..