respun multiple removal fix

From: Rakesh Pandit <rakesh at tuxera.com>

Commit c5f6ce97c1210 tries to address multiple resets but fails as work_busy doesn't involve any synchronization and can fail. This is reproducible easily as can be seen by WARNING below which is triggered with line:

WARN_ON(dev->ctrl.state == NVME_CTRL_RESETTING)

Allowing multiple resets can result in multiple controller removal as well if different conditions inside nvme_reset_work fail and which might deadlock on device_release_driver.

This patch introduces new state called NVME_CTRL_SCHED_RESET and uses it for synchronizing work queue addition (reset_work) as state change uses controller spinlock.

Did you catch my proposal to Keith and Rakesh?

I proposed to synchronize with ctrl state NVME_CTRL_RESETTING before scheduling the reset_work and on removal flow (which is why the warning exist) simply do a sync cancel of reset_work.

IMO, this is better than adding a new state to the ctrl state machine.

Quoted 2 lines

Did you catch my proposal to Keith and Rakesh?

I saw it.

Quoted 5 lines

I proposed to synchronize with ctrl state NVME_CTRL_RESETTING before scheduling the reset_work and on removal flow (which is why the warning exist) simply do a sync cancel of reset_work.

Quoted 1 line

IMO, this is better than adding a new state to the ctrl state machine.

Why? Knowing from the state machine that we're going to reset is very useful and will safe us from doing pointless work elsewhere. There is a reason we have done it that way in Fabrics from the beginning.

E.g. as the work_busy check in nvme_should_reset can now be replaced with a check for NVME_CTRL_SCHED_RESET and NVME_CTRL_RESETTING and we're much more reliable.

Quoted 10 lines

Quoted 2 lines

Did you catch my proposal to Keith and Rakesh?

I saw it.

Quoted 5 lines

I proposed to synchronize with ctrl state NVME_CTRL_RESETTING before scheduling the reset_work and on removal flow (which is why the warning exist) simply do a sync cancel of reset_work.

Quoted 5 lines

Quoted 1 line

IMO, this is better than adding a new state to the ctrl state machine.

Why? Knowing from the state machine that we're going to reset is very useful and will safe us from doing pointless work elsewhere. There is a reason we have done it that way in Fabrics from the beginning.

I'm not following, we have not done it for fabrics, we are doing in fabrics exactly what I'm proposing.

Quoted 3 lines

E.g. as the work_busy check in nvme_should_reset can now be replaced with a check for NVME_CTRL_SCHED_RESET and NVME_CTRL_RESETTING and we're much more reliable.

I agree on the state check, I think it should check NVME_CTRL_RESETTING and use that state when we schedule reset_work (like we do in fabrics) and drop the redundant WARN_ON in reset_work to protect against the special case of controller concurrent deletion (which we remove altogether by canceling the reset_work there).

I think we're in sync about the mechanics, I just understand why we need another state here.

Can something like the (untested) following work?

--

drivers/nvme/host/pci.c +1 -1

index 2769298e16dc..ca92fa24c6fc 100644
@@ -1384,7 +1384,7 @@ static bool nvme_should_reset(struct nvme_dev *dev,
 u32 csts)
        bool nssro = dev->subsystem && (csts & NVME_CSTS_NSSRO);
 
        /* If there is a reset ongoing, we shouldn't reset again. */
-       if (work_busy(&dev->reset_work))
+       if (dev->ctrl.state == NVME_CTRL_RESETTING)
                return false;
 

        /* We shouldn't reset unless the controller is on fatal error state

drivers/nvme/host/pci.c +2 -2

@@ -2026,8 +2026,8 @@ 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 (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING))
+               return -EBUSY;
        if (!queue_work(nvme_wq, &dev->reset_work))
                return -EBUSY;
        return 0;

cancel_work_sync seems to be a good idea

but I would still consider
new state to be a better alternative as it seems to make overal design simpler.

Anyway, once we can decide which way to go, I would be happy to give this a test spin.

You just forgot to remove the now reduntant WARN_ON in untested one but of course this was just untested and for demonstration only.

index a60926410438..0935dc08f46e 100644

drivers/nvme/host/core.c +3 -1

@@ -161,7 +161,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 			break;
 		}
 		break;
-	case NVME_CTRL_RESETTING:
+	case NVME_CTRL_SCHED_RESET:
 		switch (old_state) {
 		case NVME_CTRL_NEW:
 		case NVME_CTRL_LIVE:

@@ -172,6 +172,15 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 			break;
 		}
 		break;
+	case NVME_CTRL_RESETTING:
+		switch (old_state) {

Quoted 3 lines · Christoph Hellwig

index a60926410438..0935dc08f46e 100644

drivers/nvme/host/core.c +0 -0

Quoted 8 lines

drivers/nvme/host/core.c +7 -0

 		break;
+	case NVME_CTRL_RESETTING:
+		switch (old_state) {
+		case NVME_CTRL_SCHED_RESET:
+			changed = true;
+			/* FALLTHRU */
+		default:
+			break;

drivers/nvme/host/core.c

Quoted 5 lines

drivers/nvme/host/core.c +2 -0

+		}
+		break;
 	case NVME_CTRL_RECONNECTING:
 		switch (old_state) {
 		case NVME_CTRL_LIVE:

We also need to add the new NVME_SCHED_RESET state as a valid old_state transition to NVME_CTRL_DELETING.

drivers/nvme/host/core.c static int nvme_reset(struct nvme_dev *dev) · line 2021

Quoted 11 lines

drivers/nvme/host/core.c +2 -2

@@ -2009,8 +2014,8 @@ 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 (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_SCHED_RESET))
+		return -EBUSY;
 	if (!queue_work(nvme_workq, &dev->reset_work))
 		return -EBUSY;
 	return 0;

Just going through a hypothetical scenario: let's say a user requests an orderly removal of a device through sysfs. The the controller happens to fail after del_gendisk is called, and commands timeout, triggering a reset.

Today, the reset work will consider the controller dead and kill the nvme queues to force all new IO to fail immediately.

With this change, the reset work won't get to run, so nothing will get to kill the queues. The driver will be stuck at del_gendisk.

Something like the following will fix that for pci:

---

drivers/nvme/host/pci.c +0 -0

@@ -993,7 +993,10 @@ 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);