[PATCH] nvme core: allow controller RESETTING to RECONNECTING transition

[PATCH] nvme core: allow controller RESETTING to RECONNECTING transition

[jsmart2021]

From: James Smart <jsmart2021@gmail.com>

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

Signed-off-by: James Smart <james.smart at broadcom.com>
---
patch cut against the nvme-4.12 tree

 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 263946b23628..a265e528a02f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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

[PATCH] nvme core: allow controller RESETTING to RECONNECTING transition

[Sagi Grimberg]

> 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.

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.

     Signed-off-by: Sagi Grimberg <sagi at grimberg.me>

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d5e0906262ea..cfc8f8da89c8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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:
--

[PATCH] nvme core: allow controller RESETTING to RECONNECTING transition

[James Smart]

On 5/3/2017 12:59 AM, Sagi Grimberg wrote:
>
>> 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.
>
> 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.

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.


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

I don't have any problem with this.

-- james

[PATCH] nvme core: allow controller RESETTING to RECONNECTING transition

[Sagi Grimberg]

> 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.

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?

[PATCH] nvme core: allow controller RESETTING to RECONNECTING transition

[James Smart]

On 5/3/2017 9:55 AM, Sagi Grimberg wrote:
> 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...

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