* [PATCH 1/2] nvme: only allow entering LIVE from CONNECTING state
2025-02-14 8:02 [PATCH 0/2] nvme-fc: fix schedule in atomic context Daniel Wagner
@ 2025-02-14 8:02 ` Daniel Wagner
2025-02-20 10:34 ` Sagi Grimberg
2025-02-14 8:02 ` [PATCH 2/2] nvme-fc: rely on state transitions to handle connectivity loss Daniel Wagner
` (3 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Daniel Wagner @ 2025-02-14 8:02 UTC (permalink / raw)
To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
James Smart, Hannes Reinecke, Shinichiro Kawasaki
Cc: linux-nvme, linux-kernel, Daniel Wagner
The fabric transports and also the PCI transport are not entering the
LIVE state from NEW or RESETTING. This makes the state machine more
restrictive and allows to catch not supported state transitions, e.g.
directly switching from RESETTING to LIVE.
Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
drivers/nvme/host/core.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 818d4e49aab51c388af9a48bf9d466fea9cef51b..f028913e2e622ee348e88879c6e6b7e8f8a1cc82 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -564,8 +564,6 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
switch (new_state) {
case NVME_CTRL_LIVE:
switch (old_state) {
- case NVME_CTRL_NEW:
- case NVME_CTRL_RESETTING:
case NVME_CTRL_CONNECTING:
changed = true;
fallthrough;
--
2.48.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/2] nvme-fc: rely on state transitions to handle connectivity loss
2025-02-14 8:02 [PATCH 0/2] nvme-fc: fix schedule in atomic context Daniel Wagner
2025-02-14 8:02 ` [PATCH 1/2] nvme: only allow entering LIVE from CONNECTING state Daniel Wagner
@ 2025-02-14 8:02 ` Daniel Wagner
2025-02-20 10:36 ` Sagi Grimberg
2025-02-20 8:00 ` [PATCH 0/2] nvme-fc: fix schedule in atomic context Daniel Wagner
` (2 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Daniel Wagner @ 2025-02-14 8:02 UTC (permalink / raw)
To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
James Smart, Hannes Reinecke, Shinichiro Kawasaki
Cc: linux-nvme, linux-kernel, Daniel Wagner
It's not possible to call nvme_state_ctrl_state with holding a spin
lock, because nvme_state_ctrl_state calls cancel_delayed_work_sync
when fastfail is enabled.
Instead syncing the ASSOC_FLAG and state transitions using a lock, it's
possible to only rely on the state machine transitions. That means
nvme_fc_ctrl_connectivity_loss should unconditionally call
nvme_reset_ctrl which avoids the read race on the ctrl state variable.
Actually, it's not necessary to test in which state the ctrl is, the
reset work will only scheduled when the state machine is in LIVE state.
In nvme_fc_create_association, the LIVE state can only be entered if it
was previously CONNECTING. If this is not possible then the reset
handler got triggered. Thus just error out here.
Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Closes: https://lore.kernel.org/all/denqwui6sl5erqmz2gvrwueyxakl5txzbbiu3fgebryzrfxunm@iwxuthct377m/
Fixes: ee59e3820ca9 ("nvme-fc: do not ignore connectivity loss during connecting")
Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
drivers/nvme/host/fc.c | 67 +++++---------------------------------------------
1 file changed, 6 insertions(+), 61 deletions(-)
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index f4f1866fbd5b8b05730a785c7d256108c9344e62..b9929a5a7f4e3f3a03953379aceb90f0c1a0b561 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -781,61 +781,12 @@ nvme_fc_abort_lsops(struct nvme_fc_rport *rport)
static void
nvme_fc_ctrl_connectivity_loss(struct nvme_fc_ctrl *ctrl)
{
- enum nvme_ctrl_state state;
- unsigned long flags;
-
dev_info(ctrl->ctrl.device,
"NVME-FC{%d}: controller connectivity lost. Awaiting "
"Reconnect", ctrl->cnum);
- spin_lock_irqsave(&ctrl->lock, flags);
set_bit(ASSOC_FAILED, &ctrl->flags);
- state = nvme_ctrl_state(&ctrl->ctrl);
- spin_unlock_irqrestore(&ctrl->lock, flags);
-
- switch (state) {
- case NVME_CTRL_NEW:
- case NVME_CTRL_LIVE:
- /*
- * Schedule a controller reset. The reset will terminate the
- * association and schedule the reconnect timer. Reconnects
- * will be attempted until either the ctlr_loss_tmo
- * (max_retries * connect_delay) expires or the remoteport's
- * dev_loss_tmo expires.
- */
- if (nvme_reset_ctrl(&ctrl->ctrl)) {
- dev_warn(ctrl->ctrl.device,
- "NVME-FC{%d}: Couldn't schedule reset.\n",
- ctrl->cnum);
- nvme_delete_ctrl(&ctrl->ctrl);
- }
- break;
-
- case NVME_CTRL_CONNECTING:
- /*
- * The association has already been terminated and the
- * controller is attempting reconnects. No need to do anything
- * futher. Reconnects will be attempted until either the
- * ctlr_loss_tmo (max_retries * connect_delay) expires or the
- * remoteport's dev_loss_tmo expires.
- */
- break;
-
- case NVME_CTRL_RESETTING:
- /*
- * Controller is already in the process of terminating the
- * association. No need to do anything further. The reconnect
- * step will kick in naturally after the association is
- * terminated.
- */
- break;
-
- case NVME_CTRL_DELETING:
- case NVME_CTRL_DELETING_NOIO:
- default:
- /* no action to take - let it delete */
- break;
- }
+ nvme_reset_ctrl(&ctrl->ctrl);
}
/**
@@ -3071,7 +3022,6 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
struct nvmefc_ls_rcv_op *disls = NULL;
unsigned long flags;
int ret;
- bool changed;
++ctrl->ctrl.nr_reconnects;
@@ -3177,23 +3127,18 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
else
ret = nvme_fc_recreate_io_queues(ctrl);
}
+ if (!ret && test_bit(ASSOC_FAILED, &ctrl->flags))
+ ret = -EIO;
if (ret)
goto out_term_aen_ops;
- spin_lock_irqsave(&ctrl->lock, flags);
- if (!test_bit(ASSOC_FAILED, &ctrl->flags))
- changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
- else
+ if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE)) {
ret = -EIO;
- spin_unlock_irqrestore(&ctrl->lock, flags);
-
- if (ret)
goto out_term_aen_ops;
+ }
ctrl->ctrl.nr_reconnects = 0;
-
- if (changed)
- nvme_start_ctrl(&ctrl->ctrl);
+ nvme_start_ctrl(&ctrl->ctrl);
return 0; /* Success */
--
2.48.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 0/2] nvme-fc: fix schedule in atomic context
2025-02-14 8:02 [PATCH 0/2] nvme-fc: fix schedule in atomic context Daniel Wagner
2025-02-14 8:02 ` [PATCH 1/2] nvme: only allow entering LIVE from CONNECTING state Daniel Wagner
2025-02-14 8:02 ` [PATCH 2/2] nvme-fc: rely on state transitions to handle connectivity loss Daniel Wagner
@ 2025-02-20 8:00 ` Daniel Wagner
2025-02-20 12:50 ` Shinichiro Kawasaki
2025-02-20 17:23 ` Keith Busch
4 siblings, 0 replies; 8+ messages in thread
From: Daniel Wagner @ 2025-02-20 8:00 UTC (permalink / raw)
To: Keith Busch
Cc: Jens Axboe, Christoph Hellwig, Sagi Grimberg, James Smart,
Hannes Reinecke, Shinichiro Kawasaki, linux-nvme, linux-kernel
On Fri, Feb 14, 2025 at 09:02:02AM +0100, Daniel Wagner wrote:
> Shinichiro reported [1] the recent change in the nvme-fc setup path [2]
> introduced a bug. I didn't spot the schedule call in
> nvme_change_ctrl_state.
>
> It turns out the locking is not necessary if we make the state machine a
> bit more restrictive and only allow entering the LIVE state from
> CONNECTING. If we do this, it's possible to ensure we either enter LIVE
> only if there was no connection loss event. Also the connection loss
> event handler should always trigger the reset handler to avoid a
> read-write race on the state machine state variable.
>
> I've tried to replicate the original problem once again and wrote a new
> blktest which tries to trigger the race condition. I let it run a for a
> while and nothing broke, but I can't be sure it is really gone. The rest
> of the blktests also passed. Unfortunatly, the test box with FC hardware
> is currently not working, so I can't test this with real hardware.
>
> [1] https://lore.kernel.org/all/denqwui6sl5erqmz2gvrwueyxakl5txzbbiu3fgebryzrfxunm@iwxuthct377m/
> [2] https://lore.kernel.org/all/20250109-nvme-fc-handle-com-lost-v4-3-fe5cae17b492@kernel.org/
ping
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] nvme-fc: fix schedule in atomic context
2025-02-14 8:02 [PATCH 0/2] nvme-fc: fix schedule in atomic context Daniel Wagner
` (2 preceding siblings ...)
2025-02-20 8:00 ` [PATCH 0/2] nvme-fc: fix schedule in atomic context Daniel Wagner
@ 2025-02-20 12:50 ` Shinichiro Kawasaki
2025-02-20 17:23 ` Keith Busch
4 siblings, 0 replies; 8+ messages in thread
From: Shinichiro Kawasaki @ 2025-02-20 12:50 UTC (permalink / raw)
To: Daniel Wagner
Cc: Keith Busch, Jens Axboe, hch, Sagi Grimberg, James Smart,
Hannes Reinecke, linux-nvme@lists.infradead.org,
linux-kernel@vger.kernel.org
On Feb 14, 2025 / 09:02, Daniel Wagner wrote:
> Shinichiro reported [1] the recent change in the nvme-fc setup path [2]
> introduced a bug. I didn't spot the schedule call in
> nvme_change_ctrl_state.
>
> It turns out the locking is not necessary if we make the state machine a
> bit more restrictive and only allow entering the LIVE state from
> CONNECTING. If we do this, it's possible to ensure we either enter LIVE
> only if there was no connection loss event. Also the connection loss
> event handler should always trigger the reset handler to avoid a
> read-write race on the state machine state variable.
>
> I've tried to replicate the original problem once again and wrote a new
> blktest which tries to trigger the race condition. I let it run a for a
> while and nothing broke, but I can't be sure it is really gone. The rest
> of the blktests also passed. Unfortunatly, the test box with FC hardware
> is currently not working, so I can't test this with real hardware.
>
> [1] https://lore.kernel.org/all/denqwui6sl5erqmz2gvrwueyxakl5txzbbiu3fgebryzrfxunm@iwxuthct377m/
> [2] https://lore.kernel.org/all/20250109-nvme-fc-handle-com-lost-v4-3-fe5cae17b492@kernel.org/
>
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
Thanks. I reconfirmed that this series avoids the failure I reported [1]. Also I
ran all nvme test cases with various transports and observed no regression.
Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 0/2] nvme-fc: fix schedule in atomic context
2025-02-14 8:02 [PATCH 0/2] nvme-fc: fix schedule in atomic context Daniel Wagner
` (3 preceding siblings ...)
2025-02-20 12:50 ` Shinichiro Kawasaki
@ 2025-02-20 17:23 ` Keith Busch
4 siblings, 0 replies; 8+ messages in thread
From: Keith Busch @ 2025-02-20 17:23 UTC (permalink / raw)
To: Daniel Wagner
Cc: Jens Axboe, Christoph Hellwig, Sagi Grimberg, James Smart,
Hannes Reinecke, Shinichiro Kawasaki, linux-nvme, linux-kernel
On Fri, Feb 14, 2025 at 09:02:02AM +0100, Daniel Wagner wrote:
> Shinichiro reported [1] the recent change in the nvme-fc setup path [2]
> introduced a bug. I didn't spot the schedule call in
> nvme_change_ctrl_state.
Thanks, applied to nvme-6.14.
^ permalink raw reply [flat|nested] 8+ messages in thread