[RFC PATCH] nvme: allowing transition from NVME_CTRL_NEW to NVME_CTRL_DELETING

[RFC PATCH] nvme: allowing transition from NVME_CTRL_NEW to NVME_CTRL_DELETING

[Li Zhong]

It is possible that nvme_remove() being ran concurrently with
nvme_reset_work(), with following sequence:

nvme_probe()
  nvme_init_ctrl()
    //set to NEW
  nvme_async_probe()
                                                      nvme_remove()
                                                        //can not change to
                                                        //DELETING from NEW
    nvme_reset_ctrl_sync()
        nvme_reset_ctrl()
          //change from NEW
          //to RESETTING
                                                       flush reset_work()
                                                       //not yet queued
          queue reset_work
            nvme_reset_work()
              ....                                     ....

With the above running concurrently, then it is possible to cause some
strange issues, like kernel crash with illegal memory accessing
or something like:
kernel: pci 0000:00:1f.0: can't enable device: BAR 0
 [mem 0xc0000000-0xc0003fff] not claimed

This patch fixes it by allowing NVME_CTRL_DELETING state to be set from
NVME_CTRL_NEW state.

Signed-off-by: Li Zhong <lizhongfs at gmail.com>
---
 drivers/nvme/host/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1b7c2afd84cb..035aa086b26b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -347,6 +347,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		break;
 	case NVME_CTRL_DELETING:
 		switch (old_state) {
+		case NVME_CTRL_NEW:
 		case NVME_CTRL_LIVE:
 		case NVME_CTRL_ADMIN_ONLY:
 		case NVME_CTRL_RESETTING:
-- 
2.20.1

[RFC PATCH] nvme: allowing transition from NVME_CTRL_NEW to NVME_CTRL_DELETING

[Sagi Grimberg]

> It is possible that nvme_remove() being ran concurrently with
> nvme_reset_work(), with following sequence:
> 
> nvme_probe()
>    nvme_init_ctrl()
>      //set to NEW
>    nvme_async_probe()
>                                                        nvme_remove()
>                                                          //can not change to
>                                                          //DELETING from NEW
>      nvme_reset_ctrl_sync()
>          nvme_reset_ctrl()
>            //change from NEW
>            //to RESETTING
>                                                         flush reset_work()
>                                                         //not yet queued
>            queue reset_work
>              nvme_reset_work()
>                ....                                     ....
> 
> With the above running concurrently, then it is possible to cause some
> strange issues, like kernel crash with illegal memory accessing
> or something like:
> kernel: pci 0000:00:1f.0: can't enable device: BAR 0
>   [mem 0xc0000000-0xc0003fff] not claimed
> 

How about making nvme_remove syncing the async probe?
Something like this:
--
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 2a8708c9ac18..33408ffa4ba3 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -131,6 +131,7 @@ struct nvme_dev {
         dma_addr_t host_mem_descs_dma;
         struct nvme_host_mem_buf_desc *host_mem_descs;
         void **host_mem_desc_bufs;
+       async_cookie_t async_probe;
  };

  static int io_queue_depth_set(const char *val, const struct 
kernel_param *kp)
@@ -2759,7 +2760,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);
+       dev->async_probe = async_schedule(nvme_async_probe, dev);

         return 0;

@@ -2804,6 +2805,7 @@ static void nvme_remove(struct pci_dev *pdev)
  {
         struct nvme_dev *dev = pci_get_drvdata(pdev);

+       async_synchronize_cookie(dev->async_probe);
         nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
         pci_set_drvdata(pdev, NULL);
--

[RFC PATCH] nvme: allowing transition from NVME_CTRL_NEW to NVME_CTRL_DELETING

[Keith Busch]

On Tue, May 28, 2019@2:01 PM Sagi Grimberg <sagi@grimberg.me> wrote:
> @@ -2759,7 +2760,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);
> +       dev->async_probe = async_schedule(nvme_async_probe, dev);
>
>          return 0;
>
> @@ -2804,6 +2805,7 @@ static void nvme_remove(struct pci_dev *pdev)
>   {
>          struct nvme_dev *dev = pci_get_drvdata(pdev);
>
> +       async_synchronize_cookie(dev->async_probe);
>          nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
>          pci_set_drvdata(pdev, NULL);
> --

This would take extra long to recover from a hot-remove during the
initial probe reset. That path would normally unblock any lost IO
immediately, but would now be blocked on the async work. That may be
acceptable though, since it's just the initial reset that needs to
sync, and it will eventually unblock itself through the timeout
handler later.