[PATCH v3 0/3] Fix host side state machine

[PATCH v3 0/3] Fix host side state machine

[Max Gurtovoy]

Hi all,
this series is rebased above nvme-4.16.
Actually there is a still missing part in this tree (but I tested it on
my own "stable" mixed kernel with this patch):
"nvme-rdma: fix concurrent reset and reconnect" from Sagi.

The first motivation for this series was fixing RDMA initiator that crushes in
case we fail during initial connect and start error recovery during initial
connection establishment.
This patchset also renames NVME_CTRL_RECONNECTING to NVME_CTRL_CONNECTING as
this state doesn't represent only a reconnection flow but also used for
initialization process.

The tested transport for these patches was RDMA.
It will be appriciated if someone can run this series with real FC HW (to test
patch #3).

changes from V2 (James will implement the support for FC and resubmit the deleted patch):
 - removed FC support
 - removed the "nvme: delete NVME_CTRL_NEW --> NVME_CTRL_LIVE transition"

changes from V1:
 - Added FC support

Max Gurtovoy (3):
  nvme: rename NVME_CTRL_RECONNECTING state to NVME_CTRL_CONNECTING
  nvme-rdma: use NVME_CTRL_CONNECTING state to mark init process
  nvme: delete NVME_CTRL_LIVE --> NVME_CTRL_CONNECTING transition

 drivers/nvme/host/core.c    | 12 ++++++------
 drivers/nvme/host/fabrics.h |  9 +++++----
 drivers/nvme/host/fc.c      | 14 +++++++-------
 drivers/nvme/host/nvme.h    |  2 +-
 drivers/nvme/host/pci.c     |  8 ++++----
 drivers/nvme/host/rdma.c    |  7 +++++--
 6 files changed, 28 insertions(+), 24 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] nvme: rename NVME_CTRL_RECONNECTING state to NVME_CTRL_CONNECTING

[Max Gurtovoy]

In pci transport, this state is used to mark the initialization
process. This should be also used in other transports as well.

Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/core.c    | 10 +++++-----
 drivers/nvme/host/fabrics.h |  9 +++++----
 drivers/nvme/host/fc.c      | 14 +++++++-------
 drivers/nvme/host/nvme.h    |  2 +-
 drivers/nvme/host/pci.c     |  8 ++++----
 drivers/nvme/host/rdma.c    |  4 ++--
 6 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b3af8e9..c1a9706 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -265,7 +265,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 	switch (new_state) {
 	case NVME_CTRL_ADMIN_ONLY:
 		switch (old_state) {
-		case NVME_CTRL_RECONNECTING:
+		case NVME_CTRL_CONNECTING:
 			changed = true;
 			/* FALLTHRU */
 		default:
@@ -276,7 +276,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		switch (old_state) {
 		case NVME_CTRL_NEW:
 		case NVME_CTRL_RESETTING:
-		case NVME_CTRL_RECONNECTING:
+		case NVME_CTRL_CONNECTING:
 			changed = true;
 			/* FALLTHRU */
 		default:
@@ -294,7 +294,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 			break;
 		}
 		break;
-	case NVME_CTRL_RECONNECTING:
+	case NVME_CTRL_CONNECTING:
 		switch (old_state) {
 		case NVME_CTRL_LIVE:
 		case NVME_CTRL_RESETTING:
@@ -309,7 +309,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		case NVME_CTRL_LIVE:
 		case NVME_CTRL_ADMIN_ONLY:
 		case NVME_CTRL_RESETTING:
-		case NVME_CTRL_RECONNECTING:
+		case NVME_CTRL_CONNECTING:
 			changed = true;
 			/* FALLTHRU */
 		default:
@@ -2682,7 +2682,7 @@ static ssize_t nvme_sysfs_show_state(struct device *dev,
 		[NVME_CTRL_LIVE]	= "live",
 		[NVME_CTRL_ADMIN_ONLY]	= "only-admin",
 		[NVME_CTRL_RESETTING]	= "resetting",
-		[NVME_CTRL_RECONNECTING]= "reconnecting",
+		[NVME_CTRL_CONNECTING]	= "connecting",
 		[NVME_CTRL_DELETING]	= "deleting",
 		[NVME_CTRL_DEAD]	= "dead",
 	};
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 25b19f7..a3145d9 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -171,13 +171,14 @@ static inline blk_status_t nvmf_check_init_req(struct nvme_ctrl *ctrl,
 	    cmd->common.opcode != nvme_fabrics_command ||
 	    cmd->fabrics.fctype != nvme_fabrics_type_connect) {
 		/*
-		 * Reconnecting state means transport disruption, which can take
-		 * a long time and even might fail permanently, fail fast to
-		 * give upper layers a chance to failover.
+		 * Connecting state means transport disruption or initial
+		 * establishment, which can take a long time and even might
+		 * fail permanently, fail fast to give upper layers a chance
+		 * to failover.
 		 * Deleting state means that the ctrl will never accept commands
 		 * again, fail it permanently.
 		 */
-		if (ctrl->state == NVME_CTRL_RECONNECTING ||
+		if (ctrl->state == NVME_CTRL_CONNECTING ||
 		    ctrl->state == NVME_CTRL_DELETING) {
 			nvme_req(rq)->status = NVME_SC_ABORT_REQ;
 			return BLK_STS_IOERR;
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index b76ba46..b679971 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -534,7 +534,7 @@ static void __nvme_fc_delete_hw_queue(struct nvme_fc_ctrl *,
 {
 	switch (ctrl->ctrl.state) {
 	case NVME_CTRL_NEW:
-	case NVME_CTRL_RECONNECTING:
+	case NVME_CTRL_CONNECTING:
 		/*
 		 * As all reconnects were suppressed, schedule a
 		 * connect.
@@ -779,7 +779,7 @@ static void __nvme_fc_delete_hw_queue(struct nvme_fc_ctrl *,
 		}
 		break;
 
-	case NVME_CTRL_RECONNECTING:
+	case NVME_CTRL_CONNECTING:
 		/*
 		 * The association has already been terminated and the
 		 * controller is attempting reconnects.  No need to do anything
@@ -1724,7 +1724,7 @@ enum {
 	if (status &&
 	    (blk_queue_dying(rq->q) ||
 	     ctrl->ctrl.state == NVME_CTRL_NEW ||
-	     ctrl->ctrl.state == NVME_CTRL_RECONNECTING))
+	     ctrl->ctrl.state == NVME_CTRL_CONNECTING))
 		status |= cpu_to_le16(NVME_SC_DNR << 1);
 
 	if (__nvme_fc_fcpop_chk_teardowns(ctrl, op))
@@ -2951,7 +2951,7 @@ static inline blk_status_t nvme_fc_is_ready(struct nvme_fc_queue *queue,
 	unsigned long recon_delay = ctrl->ctrl.opts->reconnect_delay * HZ;
 	bool recon = true;
 
-	if (ctrl->ctrl.state != NVME_CTRL_RECONNECTING)
+	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING)
 		return;
 
 	if (portptr->port_state == FC_OBJSTATE_ONLINE)
@@ -2999,10 +2999,10 @@ static inline blk_status_t nvme_fc_is_ready(struct nvme_fc_queue *queue,
 	/* will block will waiting for io to terminate */
 	nvme_fc_delete_association(ctrl);
 
-	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RECONNECTING)) {
+	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
 		dev_err(ctrl->ctrl.device,
 			"NVME-FC{%d}: error_recovery: Couldn't change state "
-			"to RECONNECTING\n", ctrl->cnum);
+			"to CONNECTING\n", ctrl->cnum);
 		return;
 	}
 
@@ -3203,7 +3203,7 @@ static inline blk_status_t nvme_fc_is_ready(struct nvme_fc_queue *queue,
 	 * transport errors (frame drop, LS failure) inherently must kill
 	 * the association. The transport is coded so that any command used
 	 * to create the association (prior to a LIVE state transition
-	 * while NEW or RECONNECTING) will fail if it completes in error or
+	 * while NEW or CONNECTING) will fail if it completes in error or
 	 * times out.
 	 *
 	 * As such: as the connect request was mostly likely due to a
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 8e7fc1b..91529c9 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -123,7 +123,7 @@ enum nvme_ctrl_state {
 	NVME_CTRL_LIVE,
 	NVME_CTRL_ADMIN_ONLY,    /* Only admin queue live */
 	NVME_CTRL_RESETTING,
-	NVME_CTRL_RECONNECTING,
+	NVME_CTRL_CONNECTING,
 	NVME_CTRL_DELETING,
 	NVME_CTRL_DEAD,
 };
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 0bc6a9e..8060ee9 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1143,7 +1143,7 @@ static bool nvme_should_reset(struct nvme_dev *dev, u32 csts)
 	/* If there is a reset/reinit ongoing, we shouldn't reset again. */
 	switch (dev->ctrl.state) {
 	case NVME_CTRL_RESETTING:
-	case NVME_CTRL_RECONNECTING:
+	case NVME_CTRL_CONNECTING:
 		return false;
 	default:
 		break;
@@ -2290,12 +2290,12 @@ static void nvme_reset_work(struct work_struct *work)
 		nvme_dev_disable(dev, false);
 
 	/*
-	 * Introduce RECONNECTING state from nvme-fc/rdma transports to mark the
+	 * Introduce CONNECTING state from nvme-fc/rdma transports to mark the
 	 * initializing procedure here.
 	 */
-	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RECONNECTING)) {
+	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_CONNECTING)) {
 		dev_warn(dev->ctrl.device,
-			"failed to mark controller RECONNECTING\n");
+			"failed to mark controller CONNECTING\n");
 		goto out;
 	}
 
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 6c2fdfa..219c6a0 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -887,7 +887,7 @@ static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)
 static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl)
 {
 	/* If we are resetting/deleting then do nothing */
-	if (ctrl->ctrl.state != NVME_CTRL_RECONNECTING) {
+	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING) {
 		WARN_ON_ONCE(ctrl->ctrl.state == NVME_CTRL_NEW ||
 			ctrl->ctrl.state == NVME_CTRL_LIVE);
 		return;
@@ -978,7 +978,7 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
 
 static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
 {
-	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RECONNECTING))
+	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
 		return;
 
 	queue_work(nvme_wq, &ctrl->err_work);
-- 
1.8.3.1

[PATCH 2/3] nvme-rdma: use NVME_CTRL_CONNECTING state to mark init process

[Max Gurtovoy]

In order to avoid concurrent error recovery during initialization
process (allowed by the NVME_CTRL_NEW --> NVME_CTRL_RESETTING transition)
we must mark the ctrl as CONNECTING before initial connection
establisment.

Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/core.c | 1 +
 drivers/nvme/host/rdma.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c1a9706..1d7e304 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -296,6 +296,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		break;
 	case NVME_CTRL_CONNECTING:
 		switch (old_state) {
+		case NVME_CTRL_NEW:
 		case NVME_CTRL_LIVE:
 		case NVME_CTRL_RESETTING:
 			changed = true;
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 219c6a0..d86aee7 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1930,6 +1930,9 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 	if (!ctrl->queues)
 		goto out_uninit_ctrl;
 
+	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING);
+	WARN_ON_ONCE(!changed);
+
 	ret = nvme_rdma_configure_admin_queue(ctrl, true);
 	if (ret)
 		goto out_kfree_queues;
-- 
1.8.3.1

[PATCH 3/3] nvme: delete NVME_CTRL_LIVE --> NVME_CTRL_CONNECTING transition

[Max Gurtovoy]

There is no logical reason to move from live state to connecting
state. In case of initial connection establishment, the transition
should be NVME_CTRL_NEW --> NVME_CTRL_CONNECTING --> NVME_CTRL_LIVE.
In case of error recovery or reset, the transition should be
NVME_CTRL_LIVE --> NVME_CTRL_RESETTING --> NVME_CTRL_CONNECTING -->
NVME_CTRL_LIVE.

Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1d7e304..c2a3700 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -297,7 +297,6 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 	case NVME_CTRL_CONNECTING:
 		switch (old_state) {
 		case NVME_CTRL_NEW:
-		case NVME_CTRL_LIVE:
 		case NVME_CTRL_RESETTING:
 			changed = true;
 			/* FALLTHRU */
-- 
1.8.3.1

[PATCH 1/3] nvme: rename NVME_CTRL_RECONNECTING state to NVME_CTRL_CONNECTING

[James Smart]

On 1/31/2018 8:31 AM, Max Gurtovoy wrote:
> In pci transport, this state is used to mark the initialization
> process. This should be also used in other transports as well.
>
> Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
> ---

Reviewed-by: James Smart <james.smart at broadcom.com>

[PATCH 2/3] nvme-rdma: use NVME_CTRL_CONNECTING state to mark init process

[James Smart]

On 1/31/2018 8:31 AM, Max Gurtovoy wrote:
> In order to avoid concurrent error recovery during initialization
> process (allowed by the NVME_CTRL_NEW --> NVME_CTRL_RESETTING transition)
> we must mark the ctrl as CONNECTING before initial connection
> establisment.
>
> Signed-off-by: Max Gurtovoy <maxg at mellanox.com>


Reviewed-by: James Smart <james.smart at broadcom.com>

Prefer to have Sagi sign off as well on the rdma part

-- james

[PATCH 3/3] nvme: delete NVME_CTRL_LIVE --> NVME_CTRL_CONNECTING transition

[James Smart]

On 1/31/2018 8:31 AM, Max Gurtovoy wrote:
> There is no logical reason to move from live state to connecting
> state. In case of initial connection establishment, the transition
> should be NVME_CTRL_NEW --> NVME_CTRL_CONNECTING --> NVME_CTRL_LIVE.
> In case of error recovery or reset, the transition should be
> NVME_CTRL_LIVE --> NVME_CTRL_RESETTING --> NVME_CTRL_CONNECTING -->
> NVME_CTRL_LIVE.
>
> Signed-off-by: Max Gurtovoy <maxg at mellanox.com>


Reviewed-by: James Smart <james.smart at broadcom.com>

[PATCH v3 0/3] Fix host side state machine

[Sagi Grimberg]

> Hi all,
> this series is rebased above nvme-4.16.
> Actually there is a still missing part in this tree (but I tested it on
> my own "stable" mixed kernel with this patch):
> "nvme-rdma: fix concurrent reset and reconnect" from Sagi.
> 
> The first motivation for this series was fixing RDMA initiator that crushes in
> case we fail during initial connect and start error recovery during initial
> connection establishment.
> This patchset also renames NVME_CTRL_RECONNECTING to NVME_CTRL_CONNECTING as
> this state doesn't represent only a reconnection flow but also used for
> initialization process.
> 
> The tested transport for these patches was RDMA.
> It will be appriciated if someone can run this series with real FC HW (to test
> patch #3).

The series looks fine to me.

I've picked it up, but I prefer to hear from Keith/Christoph before
adding it to the next pr round.

[PATCH v3 0/3] Fix host side state machine

[Max Gurtovoy]

On 2/5/2018 4:46 PM, Sagi Grimberg wrote:
> 
>> Hi all,
>> this series is rebased above nvme-4.16.
>> Actually there is a still missing part in this tree (but I tested it on
>> my own "stable" mixed kernel with this patch):
>> "nvme-rdma: fix concurrent reset and reconnect" from Sagi.
>>
>> The first motivation for this series was fixing RDMA initiator that 
>> crushes in
>> case we fail during initial connect and start error recovery during 
>> initial
>> connection establishment.
>> This patchset also renames NVME_CTRL_RECONNECTING to 
>> NVME_CTRL_CONNECTING as
>> this state doesn't represent only a reconnection flow but also used for
>> initialization process.
>>
>> The tested transport for these patches was RDMA.
>> It will be appriciated if someone can run this series with real FC HW 
>> (to test
>> patch #3).
> 
> The series looks fine to me.
> 
> I've picked it up, but I prefer to hear from Keith/Christoph before
> adding it to the next pr round.

Me too :)

Afterwards I'll prepare a fix to 4.15-stable.

-Max.

[PATCH 1/3] nvme: rename NVME_CTRL_RECONNECTING state to NVME_CTRL_CONNECTING

[Sagi Grimberg]

> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 6c2fdfa..219c6a0 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -887,7 +887,7 @@ static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)
>   static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl)
>   {
>   	/* If we are resetting/deleting then do nothing */
> -	if (ctrl->ctrl.state != NVME_CTRL_RECONNECTING) {
> +	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING) {
>   		WARN_ON_ONCE(ctrl->ctrl.state == NVME_CTRL_NEW ||
>   			ctrl->ctrl.state == NVME_CTRL_LIVE);
>   		return;
> @@ -978,7 +978,7 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
>   
>   static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
>   {
> -	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RECONNECTING))
> +	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
>   		return;
>   
>   	queue_work(nvme_wq, &ctrl->err_work);
> 
What is this based on? It does not apply on nvme-4.16.

[PATCH v3 0/3] Fix host side state machine

[Keith Busch]

On Wed, Jan 31, 2018@06:31:23PM +0200, Max Gurtovoy wrote:
> Hi all,
> this series is rebased above nvme-4.16.
> Actually there is a still missing part in this tree (but I tested it on
> my own "stable" mixed kernel with this patch):
> "nvme-rdma: fix concurrent reset and reconnect" from Sagi.
> 
> The first motivation for this series was fixing RDMA initiator that crushes in
> case we fail during initial connect and start error recovery during initial
> connection establishment.
> This patchset also renames NVME_CTRL_RECONNECTING to NVME_CTRL_CONNECTING as
> this state doesn't represent only a reconnection flow but also used for
> initialization process.
> 
> The tested transport for these patches was RDMA.
> It will be appriciated if someone can run this series with real FC HW (to test
> patch #3).

Looks good to me.

Reviewed-by: Keith Busch <keith.busch at intel.com>

[PATCH 1/3] nvme: rename NVME_CTRL_RECONNECTING state to NVME_CTRL_CONNECTING

[Sagi Grimberg]

On 02/08/2018 05:28 PM, Sagi Grimberg wrote:
> 
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index 6c2fdfa..219c6a0 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -887,7 +887,7 @@ static void nvme_rdma_free_ctrl(struct nvme_ctrl 
>> *nctrl)
>> ? static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl)
>> ? {
>> ????? /* If we are resetting/deleting then do nothing */
>> -??? if (ctrl->ctrl.state != NVME_CTRL_RECONNECTING) {
>> +??? if (ctrl->ctrl.state != NVME_CTRL_CONNECTING) {
>> ????????? WARN_ON_ONCE(ctrl->ctrl.state == NVME_CTRL_NEW ||
>> ????????????? ctrl->ctrl.state == NVME_CTRL_LIVE);
>> ????????? return;
>> @@ -978,7 +978,7 @@ static void nvme_rdma_error_recovery_work(struct 
>> work_struct *work)
>> ? static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
>> ? {
>> -??? if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RECONNECTING))
>> +??? if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
>> ????????? return;
>> ????? queue_work(nvme_wq, &ctrl->err_work);
>>
> What is this based on? It does not apply on nvme-4.16.

Max, I fixed it up locally in nvme-4.16-rc, please verify that its
correct and passes your local tests.

[PATCH 1/3] nvme: rename NVME_CTRL_RECONNECTING state to NVME_CTRL_CONNECTING

[Max Gurtovoy]

On 2/8/2018 6:19 PM, Sagi Grimberg wrote:
> 
> 
> On 02/08/2018 05:28 PM, Sagi Grimberg wrote:
>>
>>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>>> index 6c2fdfa..219c6a0 100644
>>> --- a/drivers/nvme/host/rdma.c
>>> +++ b/drivers/nvme/host/rdma.c
>>> @@ -887,7 +887,7 @@ static void nvme_rdma_free_ctrl(struct nvme_ctrl 
>>> *nctrl)
>>> ? static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl)
>>> ? {
>>> ????? /* If we are resetting/deleting then do nothing */
>>> -??? if (ctrl->ctrl.state != NVME_CTRL_RECONNECTING) {
>>> +??? if (ctrl->ctrl.state != NVME_CTRL_CONNECTING) {
>>> ????????? WARN_ON_ONCE(ctrl->ctrl.state == NVME_CTRL_NEW ||
>>> ????????????? ctrl->ctrl.state == NVME_CTRL_LIVE);
>>> ????????? return;
>>> @@ -978,7 +978,7 @@ static void nvme_rdma_error_recovery_work(struct 
>>> work_struct *work)
>>> ? static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
>>> ? {
>>> -??? if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RECONNECTING))
>>> +??? if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
>>> ????????? return;
>>> ????? queue_work(nvme_wq, &ctrl->err_work);
>>>
>> What is this based on? It does not apply on nvme-4.16.
> 
> Max, I fixed it up locally in nvme-4.16-rc, please verify that its
> correct and passes your local tests.

Sagi,
I guess you applied it after rebasing, I've mentioned that there was 
still 1 commit missing in nvme-4.16 (your reset fix).
I'll run tests on nvme-4.16-rc and update soon.

-Max.

[PATCH 1/3] nvme: rename NVME_CTRL_RECONNECTING state to NVME_CTRL_CONNECTING

[Sagi Grimberg]

>> Max, I fixed it up locally in nvme-4.16-rc, please verify that its
>> correct and passes your local tests.
> 
> Sagi,
> I guess you applied it after rebasing, I've mentioned that there was 
> still 1 commit missing in nvme-4.16 (your reset fix).

Which one?

> I'll run tests on nvme-4.16-rc and update soon.

Thanks

[PATCH 1/3] nvme: rename NVME_CTRL_RECONNECTING state to NVME_CTRL_CONNECTING

[Max Gurtovoy]

On 2/13/2018 12:46 PM, Sagi Grimberg wrote:
>>> Max, I fixed it up locally in nvme-4.16-rc, please verify that its
>>> correct and passes your local tests.
>>
>> Sagi,
>> I guess you applied it after rebasing, I've mentioned that there was 
>> still 1 commit missing in nvme-4.16 (your reset fix).
> 
> Which one?

I guess this one caused the conflict:
d5bf4b7f4 nvme-rdma: fix concurrent reset and reconnect
> 
>> I'll run tests on nvme-4.16-rc and update soon.
> 
> Thanks


During port toggle with traffic (using dm-multipath) I see some warnings 
during ib_destroy_qp that say there are still mrs_used.
and therefore also warning in ib_dealloc_pd that says refcount on pd is 
not 0.

I'll debug it tomorrow hopefully and update.

-Max.

[PATCH 1/3] nvme: rename NVME_CTRL_RECONNECTING state to NVME_CTRL_CONNECTING

[Max Gurtovoy]

On 2/13/2018 12:46 PM, Sagi Grimberg wrote:
>>> Max, I fixed it up locally in nvme-4.16-rc, please verify that its
>>> correct and passes your local tests.
>>
>> Sagi,
>> I guess you applied it after rebasing, I've mentioned that there was 
>> still 1 commit missing in nvme-4.16 (your reset fix).
> 
> Which one?

I guess this one caused the conflict:
d5bf4b7f4 nvme-rdma: fix concurrent reset and reconnect
> 
>> I'll run tests on nvme-4.16-rc and update soon.
> 
> Thanks


During port toggle with traffic (using dm-multipath) I see some 
warnings during ib_destroy_qp that say there are still mrs_used.
and therefore also in ib_dealloc_pd that says refcount on pd is not 0.

I'll debug it tomorrow hopefully and update.

-Max.

[PATCH 1/3] nvme: rename NVME_CTRL_RECONNECTING state to NVME_CTRL_CONNECTING

[Sagi Grimberg]

> During port toggle with traffic (using dm-multipath) I see some
> warnings during ib_destroy_qp that say there are still mrs_used.
> and therefore also in ib_dealloc_pd that says refcount on pd is not 0.
> 
> I'll debug it tomorrow hopefully and update.

Is this a regression that happened due to your patch set?

[PATCH 1/3] nvme: rename NVME_CTRL_RECONNECTING state to NVME_CTRL_CONNECTING

[Max Gurtovoy]

On 2/14/2018 3:40 PM, Sagi Grimberg wrote:
> 
>> During port toggle with traffic (using dm-multipath) I see some
>> warnings during ib_destroy_qp that say there are still mrs_used.
>> and therefore also in ib_dealloc_pd that says refcount on pd is not 0.
>>
>> I'll debug it tomorrow hopefully and update.
> 
> Is this a regression that happened due to your patch set?

I don't think so. Without my patches we crash.
I see that we have a timeout on admin_q, and then I/O error:


[Wed Feb 14 14:10:59 2018] nvme nvme0: I/O 0 QID 0 timeout, reset controller
[Wed Feb 14 14:10:59 2018] nvme nvme0: failed nvme_keep_alive_end_io 
error=10
[Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
sector 704258460
[Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
sector 388820158
[Wed Feb 14 14:10:59 2018] ib_mr_pool_destroy: destroyed 121 mrs, 
mrs_used 6 for qp 000000008182fc6f
[Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
sector 489120554
[Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
sector 399385206
[Wed Feb 14 14:10:59 2018] device-mapper: multipath: Failing path 259:0.
[Wed Feb 14 14:10:59 2018] WARNING: CPU: 9 PID: 12333 at 
drivers/infiniband/core//verbs.c:1524 ib_destroy_qp+0x159/0x170 [ib_core]
[Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
sector 269330912
[Wed Feb 14 14:10:59 2018] Modules linked in:
[Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
sector 211936734
[Wed Feb 14 14:10:59 2018]  nvme_rdma(OE)
[Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
sector 383446442
[Wed Feb 14 14:10:59 2018]  nvme_fabrics(OE) nvme_core(OE)
[Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
sector 160594228


for some reason not all commands complete before we destroy the QP (we 
use dm-multipath here).

In iser (we also saw that the pool has registered regions) we created 
all_list and we free the MRs from there...


-Max.

[PATCH 1/3] nvme: rename NVME_CTRL_RECONNECTING state to NVME_CTRL_CONNECTING

[Max Gurtovoy]

On 2/14/2018 4:20 PM, Max Gurtovoy wrote:
> 
> 
> On 2/14/2018 3:40 PM, Sagi Grimberg wrote:
>>
>>> During port toggle with traffic (using dm-multipath) I see some
>>> warnings during ib_destroy_qp that say there are still mrs_used.
>>> and therefore also in ib_dealloc_pd that says refcount on pd is not 0.
>>>
>>> I'll debug it tomorrow hopefully and update.
>>
>> Is this a regression that happened due to your patch set?
> 
> I don't think so. Without my patches we crash.
> I see that we have a timeout on admin_q, and then I/O error:
> 
> 
> [Wed Feb 14 14:10:59 2018] nvme nvme0: I/O 0 QID 0 timeout, reset 
> controller
> [Wed Feb 14 14:10:59 2018] nvme nvme0: failed nvme_keep_alive_end_io 
> error=10
> [Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
> sector 704258460
> [Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
> sector 388820158
> [Wed Feb 14 14:10:59 2018] ib_mr_pool_destroy: destroyed 121 mrs, 
> mrs_used 6 for qp 000000008182fc6f
> [Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
> sector 489120554
> [Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
> sector 399385206
> [Wed Feb 14 14:10:59 2018] device-mapper: multipath: Failing path 259:0.
> [Wed Feb 14 14:10:59 2018] WARNING: CPU: 9 PID: 12333 at 
> drivers/infiniband/core//verbs.c:1524 ib_destroy_qp+0x159/0x170 [ib_core]
> [Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
> sector 269330912
> [Wed Feb 14 14:10:59 2018] Modules linked in:
> [Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
> sector 211936734
> [Wed Feb 14 14:10:59 2018]? nvme_rdma(OE)
> [Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
> sector 383446442
> [Wed Feb 14 14:10:59 2018]? nvme_fabrics(OE) nvme_core(OE)
> [Wed Feb 14 14:10:59 2018] print_req_error: I/O error, dev nvme0n1, 
> sector 160594228
> 
> 
> for some reason not all commands complete before we destroy the QP (we 
> use dm-multipath here).

What I did is freezing the queue and it seems to help. But now the 
failover takes ~cmd_timeout secs:


@@ -931,6 +938,8 @@ static void nvme_rdma_error_recovery_work(struct 
work_struct *work)
         nvme_stop_keep_alive(&ctrl->ctrl);

         if (ctrl->ctrl.queue_count > 1) {
+               nvme_start_freeze(&ctrl->ctrl);
+               nvme_wait_freeze(&ctrl->ctrl);
                 nvme_stop_queues(&ctrl->ctrl);
                 blk_mq_tagset_busy_iter(&ctrl->tag_set,
                                         nvme_cancel_request, &ctrl->ctrl);
@@ -948,6 +957,7 @@ static void nvme_rdma_error_recovery_work(struct 
work_struct *work)
          */
         blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
         nvme_start_queues(&ctrl->ctrl);
+       nvme_unfreeze(&ctrl->ctrl);

         if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
                 /* state change failure should never happen */


I borrowed it from the nvme pci driver to make sure we won't receive any 
new requests (even timed out requests).
Maybe Keith can look on that ?

I don't think the above is the right solution but we must to make sure 
we don't get any request during reconnection or after freeing the QP.
We might end up with free/alloc the tagset on each reconnection or at 
least use blk_mq_queue_reinit.

I'll try it too.


> 
> In iser (we also saw that the pool has registered regions) we created 
> all_list and we free the MRs from there...
> 
> 
> -Max.