nvme core: allow controller RESETTING to RECONNECTING transition

From: James Smart <[email protected]>

Allow controller state transition : RESETTING to RECONNECTING

I intend to have the nvme_fc transport set the state to RESETTING when tearing down the current association (error or controller reset), then transitioning to RECONNECTING when attempting to establish a new association.

-- james

---
patch cut against the nvme-4.12 tree

 drivers/nvme/host/core.c | 1 +
 1 file changed, 1 insertion(+)

index 263946b23628..a265e528a02f 100644
@@ -171,6 +171,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 	case NVME_CTRL_RECONNECTING:
 		switch (old_state) {
 		case NVME_CTRL_LIVE:
+		case NVME_CTRL_RESETTING:
 			changed = true;
 			/* FALLTHRU */
 		default:

2.11.0

I'm not sure this is a good idea. I think that the semantics of RESETTING state is that we are performing a controller reset, RECONNECTING semantics means we are trying to reestablish our controller session. It seems that mixing these states is just confusing.

In fact, I have a patch in the pipe that disallows the state transition from RECONNECTING to RESETTING:

--
commit 210917a2a84fc8c19c37a899d5783bd0fcaaf1ac

Author: Sagi Grimberg <sagi at grimberg.me>
Date:   Sun Apr 30 12:35:44 2017 +0300

nvme: Don't allow to reset a reconnecting controller

The reset operation is guaranteed to fail for all scenarios but the esoteric case where in the last reconnect attempt concurrent with the reset we happen to successfully reconnect.

We just deny initiating a reset if we are reconnecting.

index d5e0906262ea..cfc8f8da89c8 100644
@@ -165,7 +165,6 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
                 switch (old_state) {
                 case NVME_CTRL_NEW:
                 case NVME_CTRL_LIVE:
-               case NVME_CTRL_RECONNECTING:
                         changed = true;
                         /* FALLTHRU */
                 default:

--

I'm not following as, at a high level, it sounds like we're saying the same thing. I'm sure the difference is in the definitions of "controller reset" and "reestablish our controller session".

here's how I view them:

RESETTING: stopping the blk queues, killing the transport queues/connections and outstanding io on them, then formally tearing down the fabric association. Officially, RESETTING would be when CC.EN=0 is done. But that can only occur if there is connectivity to the target and can use the admin connection for a Set_Property command. All the same actions take place except the Set_Property on cases where you lose connectivity. I'm viewing all of these actions, of terminating the original transport association, as RESETTING.

RECONNECTING: restarting the association - creating transport queues/connections, reprobing the controller and re-releasing block queues. I'm viewing all of the actions to create a new transport association, as RECONNECTING.

on FC, I was going to: move the controller from LIVE->RESETTING when tearing down the association, whether invoked by the core reset interface or upon detecting an error and independent of whether I can send an CC.EN=0 (which I'll do if connected); and after teardown, from RESETTING->CONNECTING as I start the new association. And if the new association can't be immediately created: a) if there is connectivity, use the same periodic retry based on max_reconnects and reconnect_delay; and b) if there isn't connectivity, delay until connectivity occurs or a timeout.

I find it more confusing with what rdma has:

RESETTING: When invoked by core layer to reset the ctrl. stops block queues, kills transport queues/connections and outstanding ios, attempts immediate new association with target, creating transport queues/connections and releasing io. AND if new association fails, device is deleted.

RECONNECTING: transport error detected. stops block queues, kills transport queues/connections and outstanding ios, attempts new association with target, creating transport queues/connections and releasing io. AND if new association fails, retries the new association connect per max_reconnects/reconnect_delay before giving up.

As I interpret them, the states reflect why/how it was torn down/reconnecting (core layer invoked and CC.EN written vs transport detected/deleted), and whether a reconnect will be retried or not. State of what its doing is lost a bit.

I don't have any problem with this.

-- james

This sort of discussion really calls for a unification of control and the error handling flows for all transports, with a single state machine semantics. I'm currently looking into it, but I suspect it'll take me a little while...

Christoph, Keith, any thoughts?

I don't think it has to be that drastic, but at least a common understanding of what the states mean/span should be known.

-- james