nvme/pci: Fix hot removal during error handling
PATCH v2 1/1 nvme/pci: Fix hot removal during error handling
A removal waits for the reset_work to complete. If a surprise removal occurs around the same time as an error triggered controller reset, and reset work happened to dispatch a command to the removed controller, the command won't be recovered since the timeout work doesn't do anything during error recovery.
This patch fixes this by removing the admin queue prior to syncing reset.
---
v1 -> v2:
This is simpler than the previous version. We only need to move syncing with the work after the admin queue has been successfully removed.
drivers/nvme/host/pci.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/nvme/host/pci.c +2 -3
@@ -2564,16 +2564,15 @@ static void nvme_remove(struct pci_dev *pdev)struct nvme_dev *dev =pci_get_drvdata (pdev);nvme_change_ctrl_state (&dev->ctrl,NVME_CTRL_DELETING ); - -cancel_work_sync (&dev->ctrl.reset_work);pci_set_drvdata (pdev,NULL );if (!pci_device_is_present (pdev)) {nvme_change_ctrl_state (&dev->ctrl,NVME_CTRL_DEAD );nvme_dev_disable (dev, true); +nvme_dev_remove_admin (dev); } -flush_work (&dev->ctrl.reset_work); +cancel_work_sync (&dev->ctrl.reset_work);nvme_stop_ctrl (&dev->ctrl);nvme_remove_namespaces (&dev->ctrl);nvme_dev_disable (dev, true);
Signature · 1 line
2.14.4
Sagi Grimberg
maintainerPATCH v2 1/1 nvme/pci: Fix hot removal during error handling
drivers/nvme/host/pci.c static void nvme_remove(struct pci_dev *pdev) · line 2575
Quoted 3 lines
drivers/nvme/host/pci.c +1
-1-flush_work (&dev->ctrl.reset_work); +cancel_work_sync (&dev->ctrl.reset_work);
Does reset_work requeue itself? if not I don't fully understand this change.
PATCH v2 1/1 nvme/pci: Fix hot removal during error handling
Quoted 6 lines · Sagi Grimberg
drivers/nvme/host/pci.c static void nvme_remove(struct pci_dev *pdev) · line 2575
Quoted 3 lines
drivers/nvme/host/pci.c +1
-1-flush_work (&dev->ctrl.reset_work); +cancel_work_sync (&dev->ctrl.reset_work);Does
reset_workrequeue itself? if not I don't fully understand this change.
You'd have to abuse the state machine if reset_work wanted to requeue itself, so that shouldn't happen.
We could just leave it as a flush work, and that should be okay too. The cancel_work_sync should accomplish the same, but also prevent the work from even starting if it just so happens to have been pending in a work queue, but that seems very unlikely.
Sagi Grimberg
maintainerPATCH v2 1/1 nvme/pci: Fix hot removal during error handling
Quoted 7 lines
Quoted 6 lines
drivers/nvme/host/pci.c static void nvme_remove(struct pci_dev *pdev) · line 2575
Quoted 3 lines
drivers/nvme/host/pci.c +1
-1-flush_work (&dev->ctrl.reset_work); +cancel_work_sync (&dev->ctrl.reset_work);Does
reset_workrequeue itself? if not I don't fully understand this change.
Quoted 7 lines
You'd have to abuse the state machine if
reset_workwanted to requeue itself, so that shouldn't happen.We could just leave it as a flush work, and that should be okay too. The
cancel_work_syncshould accomplish the same, but also prevent the work from even starting if it just so happens to have been pending in a work queue, but that seems very unlikely.
I don't mind, but was looking to understand what it is trying to achieve. If you do change that, would be a good idea to include it in the change log
Christoph Hellwig
maintainerPATCH v2 1/1 nvme/pci: Fix hot removal during error handling
Quoted 10 lines · Sagi Grimberg
Quoted 10 lines
Quoted 6 lines
drivers/nvme/host/pci.c static void nvme_remove(struct pci_dev *pdev) · line 2575
Quoted 3 lines
drivers/nvme/host/pci.c +1
-1-flush_work (&dev->ctrl.reset_work); +cancel_work_sync (&dev->ctrl.reset_work);Does
reset_workrequeue itself? if not I don't fully understand this change.You'd have to abuse the state machine if
reset_workwanted to requeue itself, so that shouldn't happen.
Quoted 8 lines
Quoted 4 lines
We could just leave it as a flush work, and that should be okay too. The
cancel_work_syncshould accomplish the same, but also prevent the work from even starting if it just so happens to have been pending in a work queue, but that seems very unlikely.I don't mind, but was looking to understand what it is trying to achieve. If you do change that, would be a good idea to include it in the change log
Keith, any plans to resend? Or should I take this one after all?