[PATCH 1/3] nvme-pci: Fix timeouts in connecting state

[PATCH 1/3] nvme-pci: Fix timeouts in connecting state

[Keith Busch]

We need to halt the controller immediately if we haven't completed
initialization as indicated by the new "connecting" state.

Fixes: ad70062cdb ("nvme-pci: introduce RECONNECTING state to mark initializing procedure")
Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index ab9c19525fa8..90e276c05f79 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1215,13 +1215,17 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 	 * cancellation error. All outstanding requests are completed on
 	 * shutdown, so we return BLK_EH_HANDLED.
 	 */
-	if (dev->ctrl.state == NVME_CTRL_RESETTING) {
+	switch (dev->ctrl.state) {
+	case NVME_CTRL_CONNECTING:
+	case NVME_CTRL_RESETTING:
 		dev_warn(dev->ctrl.device,
 			 "I/O %d QID %d timeout, disable controller\n",
 			 req->tag, nvmeq->qid);
 		nvme_dev_disable(dev, false);
 		nvme_req(req)->flags |= NVME_REQ_CANCELLED;
 		return BLK_EH_HANDLED;
+	default:
+		break;
 	}
 
 	/*
-- 
2.14.3

[PATCH 2/3] nvme: Sync queues on controller resets

[Keith Busch]

This patch has the nvme pci driver synchronize request queues to ensure
starting the controller is not racing with a previously running timeout
handler.

Reported-by: Jianchao Wang <jianchao.w.wang at oracle.com>
Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 15 ++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/pci.c  |  1 +
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 2fd8688cfa47..a9bce23a991f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3542,12 +3542,25 @@ void nvme_start_queues(struct nvme_ctrl *ctrl)
 	struct nvme_ns *ns;
 
 	mutex_lock(&ctrl->namespaces_mutex);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	list_for_each_entry(ns, &ctrl->namespaces, list) {
 		blk_mq_unquiesce_queue(ns->queue);
+		blk_mq_kick_requeue_list(ns->queue);
+	}
 	mutex_unlock(&ctrl->namespaces_mutex);
 }
 EXPORT_SYMBOL_GPL(nvme_start_queues);
 
+void nvme_sync_queues(struct nvme_ctrl *ctrl)
+{
+	struct nvme_ns *ns;
+
+	mutex_lock(&ctrl->namespaces_mutex);
+	list_for_each_entry(ns, &ctrl->namespaces, list)
+		blk_sync_queue(ns->queue);
+	mutex_unlock(&ctrl->namespaces_mutex);
+}
+EXPORT_SYMBOL_GPL(nvme_sync_queues);
+
 int nvme_reinit_tagset(struct nvme_ctrl *ctrl, struct blk_mq_tag_set *set)
 {
 	if (!ctrl->ops->reinit_request)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 27e31c00b306..466081e5f680 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -374,6 +374,7 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 
 void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
+void nvme_sync_queues(struct nvme_ctrl *ctrl);
 void nvme_kill_queues(struct nvme_ctrl *ctrl);
 void nvme_unfreeze(struct nvme_ctrl *ctrl);
 void nvme_wait_freeze(struct nvme_ctrl *ctrl);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 90e276c05f79..7a2e4383c468 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2290,6 +2290,7 @@ static void nvme_reset_work(struct work_struct *work)
 	 */
 	if (dev->ctrl.ctrl_config & NVME_CC_ENABLE)
 		nvme_dev_disable(dev, false);
+	nvme_sync_queues(&dev->ctrl);
 
 	/*
 	 * Introduce CONNECTING state from nvme-fc/rdma transports to mark the
-- 
2.14.3

[PATCH 2/3] nvme: Sync queues on controller resets

[jianchao.wang]

Hi Keith

Thanks for your time and patch for this.

However, as I shared last time, there is defect here. Please refer to.

There could be a circular pattern here. Please consider the following scenario:

timeout_work context                    reset_work context
nvme_timeout                            nvme_reset_work
  -> nvme_dev_disable                     -> nvme_sync_queues // hold namespace_mutex
    -> nvme_stop_queues                     -> blk_sync_queue
      -> require namespaces_mutex               -> cancel_work_sync(&q->timeout_work)

On the other hand, the blk_mq_kick_requeue_list() should be also added in nvme_kill_queues
for the case of queue_count < 2

Thanks
Jianchao

On 02/10/2018 01:41 AM, Keith Busch wrote:
> This patch has the nvme pci driver synchronize request queues to ensure
> starting the controller is not racing with a previously running timeout
> handler.
> 
> Reported-by: Jianchao Wang <jianchao.w.wang at oracle.com>
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
>  drivers/nvme/host/core.c | 15 ++++++++++++++-
>  drivers/nvme/host/nvme.h |  1 +
>  drivers/nvme/host/pci.c  |  1 +
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 2fd8688cfa47..a9bce23a991f 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3542,12 +3542,25 @@ void nvme_start_queues(struct nvme_ctrl *ctrl)
>  	struct nvme_ns *ns;
>  
>  	mutex_lock(&ctrl->namespaces_mutex);
> -	list_for_each_entry(ns, &ctrl->namespaces, list)
> +	list_for_each_entry(ns, &ctrl->namespaces, list) {
>  		blk_mq_unquiesce_queue(ns->queue);
> +		blk_mq_kick_requeue_list(ns->queue);
> +	}
>  	mutex_unlock(&ctrl->namespaces_mutex);
>  }
>  EXPORT_SYMBOL_GPL(nvme_start_queues);
>  
> +void nvme_sync_queues(struct nvme_ctrl *ctrl)
> +{
> +	struct nvme_ns *ns;
> +
> +	mutex_lock(&ctrl->namespaces_mutex);
> +	list_for_each_entry(ns, &ctrl->namespaces, list)
> +		blk_sync_queue(ns->queue);
> +	mutex_unlock(&ctrl->namespaces_mutex);
> +}
> +EXPORT_SYMBOL_GPL(nvme_sync_queues);
> +
>  int nvme_reinit_tagset(struct nvme_ctrl *ctrl, struct blk_mq_tag_set *set)
>  {
>  	if (!ctrl->ops->reinit_request)
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 27e31c00b306..466081e5f680 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -374,6 +374,7 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
>  
>  void nvme_stop_queues(struct nvme_ctrl *ctrl);
>  void nvme_start_queues(struct nvme_ctrl *ctrl);
> +void nvme_sync_queues(struct nvme_ctrl *ctrl);
>  void nvme_kill_queues(struct nvme_ctrl *ctrl);
>  void nvme_unfreeze(struct nvme_ctrl *ctrl);
>  void nvme_wait_freeze(struct nvme_ctrl *ctrl);
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 90e276c05f79..7a2e4383c468 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2290,6 +2290,7 @@ static void nvme_reset_work(struct work_struct *work)
>  	 */
>  	if (dev->ctrl.ctrl_config & NVME_CC_ENABLE)
>  		nvme_dev_disable(dev, false);
> +	nvme_sync_queues(&dev->ctrl);
>  
>  	/*
>  	 * Introduce CONNECTING state from nvme-fc/rdma transports to mark the
> 

[PATCH 2/3] nvme: Sync queues on controller resets

[jianchao.wang]

Hi Keith

On 02/10/2018 09:55 AM, jianchao.wang wrote:
> Hi Keith
> 
> Thanks for your time and patch for this.
> 
> However, as I shared last time, there is defect here. Please refer to.
> 
> There could be a circular pattern here. Please consider the following scenario:
> 
> timeout_work context                    reset_work context
> nvme_timeout                            nvme_reset_work
>   -> nvme_dev_disable                     -> nvme_sync_queues // hold namespace_mutex
>     -> nvme_stop_queues                     -> blk_sync_queue
>       -> require namespaces_mutex               -> cancel_work_sync(&q->timeout_work)
> 

Looks like we could use rwsem to replace namespaces_mutex.

Thanks
Jianchao


> On the other hand, the blk_mq_kick_requeue_list() should be also added in nvme_kill_queues
> for the case of queue_count < 2
> 
> Thanks
> Jianchao
> 
> On 02/10/2018 01:41 AM, Keith Busch wrote:
>> This patch has the nvme pci driver synchronize request queues to ensure
>> starting the controller is not racing with a previously running timeout
>> handler.
>>
>> Reported-by: Jianchao Wang <jianchao.w.wang at oracle.com>
>> Signed-off-by: Keith Busch <keith.busch at intel.com>
>> ---
>>  drivers/nvme/host/core.c | 15 ++++++++++++++-
>>  drivers/nvme/host/nvme.h |  1 +
>>  drivers/nvme/host/pci.c  |  1 +
>>  3 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 2fd8688cfa47..a9bce23a991f 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -3542,12 +3542,25 @@ void nvme_start_queues(struct nvme_ctrl *ctrl)
>>  	struct nvme_ns *ns;
>>  
>>  	mutex_lock(&ctrl->namespaces_mutex);
>> -	list_for_each_entry(ns, &ctrl->namespaces, list)
>> +	list_for_each_entry(ns, &ctrl->namespaces, list) {
>>  		blk_mq_unquiesce_queue(ns->queue);
>> +		blk_mq_kick_requeue_list(ns->queue);
>> +	}
>>  	mutex_unlock(&ctrl->namespaces_mutex);
>>  }
>>  EXPORT_SYMBOL_GPL(nvme_start_queues);
>>  
>> +void nvme_sync_queues(struct nvme_ctrl *ctrl)
>> +{
>> +	struct nvme_ns *ns;
>> +
>> +	mutex_lock(&ctrl->namespaces_mutex);
>> +	list_for_each_entry(ns, &ctrl->namespaces, list)
>> +		blk_sync_queue(ns->queue);
>> +	mutex_unlock(&ctrl->namespaces_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(nvme_sync_queues);
>> +
>>  int nvme_reinit_tagset(struct nvme_ctrl *ctrl, struct blk_mq_tag_set *set)
>>  {
>>  	if (!ctrl->ops->reinit_request)
>> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>> index 27e31c00b306..466081e5f680 100644
>> --- a/drivers/nvme/host/nvme.h
>> +++ b/drivers/nvme/host/nvme.h
>> @@ -374,6 +374,7 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
>>  
>>  void nvme_stop_queues(struct nvme_ctrl *ctrl);
>>  void nvme_start_queues(struct nvme_ctrl *ctrl);
>> +void nvme_sync_queues(struct nvme_ctrl *ctrl);
>>  void nvme_kill_queues(struct nvme_ctrl *ctrl);
>>  void nvme_unfreeze(struct nvme_ctrl *ctrl);
>>  void nvme_wait_freeze(struct nvme_ctrl *ctrl);
>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>> index 90e276c05f79..7a2e4383c468 100644
>> --- a/drivers/nvme/host/pci.c
>> +++ b/drivers/nvme/host/pci.c
>> @@ -2290,6 +2290,7 @@ static void nvme_reset_work(struct work_struct *work)
>>  	 */
>>  	if (dev->ctrl.ctrl_config & NVME_CC_ENABLE)
>>  		nvme_dev_disable(dev, false);
>> +	nvme_sync_queues(&dev->ctrl);
>>  
>>  	/*
>>  	 * Introduce CONNECTING state from nvme-fc/rdma transports to mark the
>>
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Dnvme&d=DwICAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=-ffcRNVB1ERVD0CZrnJUyZOFBETbuy0WHX7ksyM8Dcs&s=KWBywqLDbcong1rA6oK0dD_hff4FSr7Y86aUkY-gskE&e=
> 

[PATCH 2/3] nvme: Sync queues on controller resets

[Keith Busch]

On Sun, Feb 11, 2018@09:53:03AM +0800, jianchao.wang wrote:
> On 02/10/2018 09:55 AM, jianchao.wang wrote:
> > There could be a circular pattern here. Please consider the following scenario:
> > 
> > timeout_work context                    reset_work context
> > nvme_timeout                            nvme_reset_work
> >   -> nvme_dev_disable                     -> nvme_sync_queues // hold namespace_mutex
> >     -> nvme_stop_queues                     -> blk_sync_queue
> >       -> require namespaces_mutex               -> cancel_work_sync(&q->timeout_work)
> > 
> 
> Looks like we could use rwsem to replace namespaces_mutex.

Looks like rwsem is queued up for 4.17. I'll send an update based on
that. I guess this one and 3/3 can wait for 4.17, but 1/3 should still
go in 4.16.

[PATCH 2/3] nvme: Sync queues on controller resets

[jianchao.wang]

Hi Keith

On 02/13/2018 05:46 AM, Keith Busch wrote:
> On Sun, Feb 11, 2018@09:53:03AM +0800, jianchao.wang wrote:
>> On 02/10/2018 09:55 AM, jianchao.wang wrote:
>>> There could be a circular pattern here. Please consider the following scenario:
>>>
>>> timeout_work context                    reset_work context
>>> nvme_timeout                            nvme_reset_work
>>>   -> nvme_dev_disable                     -> nvme_sync_queues // hold namespace_mutex
>>>     -> nvme_stop_queues                     -> blk_sync_queue
>>>       -> require namespaces_mutex               -> cancel_work_sync(&q->timeout_work)
>>>
>>
>> Looks like we could use rwsem to replace namespaces_mutex.
> 
> Looks like rwsem is queued up for 4.17. I'll send an update based on
> that. I guess this one and 3/3 can wait for 4.17, but 1/3 should still
> go in 4.16.
> 

Would you please queue this patch for next ?
I incurred this issue when NVMe card died with a lot of in-flight requests.

Thanks
Jianchao

[PATCH 2/3] nvme: Sync queues on controller resets

[Keith Busch]

On Wed, Apr 18, 2018@10:26:28AM +0800, jianchao.wang wrote:
> Hi Keith
> 
> On 02/13/2018 05:46 AM, Keith Busch wrote:
> > On Sun, Feb 11, 2018@09:53:03AM +0800, jianchao.wang wrote:
> >> On 02/10/2018 09:55 AM, jianchao.wang wrote:
> >>> There could be a circular pattern here. Please consider the following scenario:
> >>>
> >>> timeout_work context                    reset_work context
> >>> nvme_timeout                            nvme_reset_work
> >>>   -> nvme_dev_disable                     -> nvme_sync_queues // hold namespace_mutex
> >>>     -> nvme_stop_queues                     -> blk_sync_queue
> >>>       -> require namespaces_mutex               -> cancel_work_sync(&q->timeout_work)
> >>>
> >>
> >> Looks like we could use rwsem to replace namespaces_mutex.
> > 
> > Looks like rwsem is queued up for 4.17. I'll send an update based on
> > that. I guess this one and 3/3 can wait for 4.17, but 1/3 should still
> > go in 4.16.
> > 
> 
> Would you please queue this patch for next ?
> I incurred this issue when NVMe card died with a lot of in-flight requests.

Right, thanks for the reminder. I'll need to rebase and resend this.
Travelling at the moment, so may be another day or two. Thanks again!

[PATCH 3/3] nvme: Fail controller on timeouts during reset

[Keith Busch]

We can't schedule a second controller reset if the controller fails while
the driver is already attempting to start it. Synchronous admin commands
are already handled appropriately since they are never retried and the
completion status is read directly. Asynchronous IO commands, however,
were previously undetected.

This patch fixes that by preventing retries on IO commands during
controller connecting states, and directing the controller to a failed
state after aborting the timed out commands. Without this patch, a
controller that fails IO commands during start up would hang indefinitely.

Reported-by: Jianchao Wang <jianchao.w.wang at oracle.com>
Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 6 ++++--
 drivers/nvme/host/pci.c  | 6 +++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a9bce23a991f..c0f4771d79a2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -240,13 +240,15 @@ EXPORT_SYMBOL_GPL(nvme_complete_rq);
 
 void nvme_cancel_request(struct request *req, void *data, bool reserved)
 {
+	struct nvme_ctrl *ctrl = data;
 	if (!blk_mq_request_started(req))
 		return;
 
-	dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
-				"Cancelling I/O %d", req->tag);
+	dev_dbg_ratelimited(ctrl->device, "Cancelling I/O %d", req->tag);
 
 	nvme_req(req)->status = NVME_SC_ABORT_REQ;
+	if (ctrl->state == NVME_CTRL_CONNECTING)
+		nvme_req(req)->status |= NVME_SC_DNR;
 	blk_mq_complete_request(req);
 
 }
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 7a2e4383c468..77929d35eae8 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1212,11 +1212,15 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 	/*
 	 * Shutdown immediately if controller times out while starting. The
 	 * reset work will see the pci device disabled when it gets the forced
-	 * cancellation error. All outstanding requests are completed on
+	 * cancellation error. The driver won't see the status if it is waiting
+	 * on asynchronous comands, so we set the state to deleting to prevent
+	 * it from progressing. All outstanding requests are completed on
 	 * shutdown, so we return BLK_EH_HANDLED.
 	 */
 	switch (dev->ctrl.state) {
 	case NVME_CTRL_CONNECTING:
+		nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
+		/* FALLTHRU */
 	case NVME_CTRL_RESETTING:
 		dev_warn(dev->ctrl.device,
 			 "I/O %d QID %d timeout, disable controller\n",
-- 
2.14.3

[PATCH 3/3] nvme: Fail controller on timeouts during reset

[jianchao.wang]

Hi Keith

Thanks for your precious time and patch for this.

On 02/10/2018 01:41 AM, Keith Busch wrote:
> We can't schedule a second controller reset if the controller fails while
> the driver is already attempting to start it. Synchronous admin commands
> are already handled appropriately since they are never retried and the
> completion status is read directly. Asynchronous IO commands, however,
> were previously undetected.
> 
> This patch fixes that by preventing retries on IO commands during
> controller connecting states, and directing the controller to a failed
> state after aborting the timed out commands. Without this patch, a
> controller that fails IO commands during start up would hang indefinitely.
> 
> Reported-by: Jianchao Wang <jianchao.w.wang at oracle.com>
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
>  drivers/nvme/host/core.c | 6 ++++--
>  drivers/nvme/host/pci.c  | 6 +++++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index a9bce23a991f..c0f4771d79a2 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -240,13 +240,15 @@ EXPORT_SYMBOL_GPL(nvme_complete_rq);
>  
>  void nvme_cancel_request(struct request *req, void *data, bool reserved)
>  {
> +	struct nvme_ctrl *ctrl = data;
>  	if (!blk_mq_request_started(req))
>  		return;
>  
> -	dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
> -				"Cancelling I/O %d", req->tag);
> +	dev_dbg_ratelimited(ctrl->device, "Cancelling I/O %d", req->tag);
>  
>  	nvme_req(req)->status = NVME_SC_ABORT_REQ;
> +	if (ctrl->state == NVME_CTRL_CONNECTING)
> +		nvme_req(req)->status |= NVME_SC_DNR;
>  	blk_mq_complete_request(req);
>  
>  }
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 7a2e4383c468..77929d35eae8 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1212,11 +1212,15 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
>  	/*
>  	 * Shutdown immediately if controller times out while starting. The
>  	 * reset work will see the pci device disabled when it gets the forced
> -	 * cancellation error. All outstanding requests are completed on
> +	 * cancellation error. The driver won't see the status if it is waiting
> +	 * on asynchronous comands, so we set the state to deleting to prevent
> +	 * it from progressing. All outstanding requests are completed on
>  	 * shutdown, so we return BLK_EH_HANDLED.
>  	 */
>  	switch (dev->ctrl.state) {
>  	case NVME_CTRL_CONNECTING:

It seems that Max's commit (Fix host side state machine) have not been committed.
So it should be RECONNECTING here.

> +		nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
> +		/* FALLTHRU */
>  	case NVME_CTRL_RESETTING:
>  		dev_warn(dev->ctrl.device,
>  			 "I/O %d QID %d timeout, disable controller\n",
> 
Test this patch with my patchset nvme-pci: fixes on nvme_timeout and nvme_dev_disable and nvme: nvme: change namespaces_mutext to namespaces_rwsem
Please refer to following branch
https://github.com/jianchwa/linux-blcok.git nvme_fixes_V4_plus_rwsem
The IO hang I met had gone.

Following is the log
[ 1111.361576] Intercept IO 65 state 4 (4 is RECONNECTING state)
[ 1141.953043] print_req_error: I/O error, dev nvme0n1, sector 131936
[ 1141.953325] nvme nvme0: failed to mark controller state 1
[ 1141.953330] nvme nvme0: Removing after probe failure status: 0
[ 1142.008154] nvme0n1: detected capacity change from 128035676160 to 0
[ 1142.184162] nvme nvme0: failed to set APST feature (-19



Sincerely
Jianchao

[PATCH 3/3] nvme: Fail controller on timeouts during reset

[Sagi Grimberg]

>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>> index 7a2e4383c468..77929d35eae8 100644
>> --- a/drivers/nvme/host/pci.c
>> +++ b/drivers/nvme/host/pci.c
>> @@ -1212,11 +1212,15 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
>>   	/*
>>   	 * Shutdown immediately if controller times out while starting. The
>>   	 * reset work will see the pci device disabled when it gets the forced
>> -	 * cancellation error. All outstanding requests are completed on
>> +	 * cancellation error. The driver won't see the status if it is waiting
>> +	 * on asynchronous comands, so we set the state to deleting to prevent
>> +	 * it from progressing. All outstanding requests are completed on
>>   	 * shutdown, so we return BLK_EH_HANDLED.
>>   	 */
>>   	switch (dev->ctrl.state) {
>>   	case NVME_CTRL_CONNECTING:
> 
> It seems that Max's commit (Fix host side state machine) have not been committed.
> So it should be RECONNECTING here.

Actually, they are already in nvme-4.16-rc.

I'd like to pick these up as well once they converge (even
taking 1+3 would be a good start).

[PATCH 3/3] nvme: Fail controller on timeouts during reset

[jianchao.wang]

Hi Sagi

On 02/11/2018 05:53 PM, Sagi Grimberg wrote:
> Actually, they are already in nvme-4.16-rc.
> 
> I'd like to pick these up as well once they converge (even
> taking 1+3 would be a good start).

Please don't refer to my test result here.
It is tested on my patchset, not the currently source code on your git tree.

Thanks
Jianchao

[PATCH 3/3] nvme: Fail controller on timeouts during reset

[Sagi Grimberg]

> Hi Sagi
> 
> On 02/11/2018 05:53 PM, Sagi Grimberg wrote:
>> Actually, they are already in nvme-4.16-rc.
>>
>> I'd like to pick these up as well once they converge (even
>> taking 1+3 would be a good start).
> 
> Please don't refer to my test result here.
> It is tested on my patchset, not the currently source code on your git tree.

OK, what I don't understand is what is your feedback on this patchset as
I'd like to take it for 4.16-rc and I want a review for it

[PATCH 3/3] nvme: Fail controller on timeouts during reset

[jianchao.wang]

Hi Sagi

Sorry for bothering you.

On 02/13/2018 02:37 AM, Sagi Grimberg wrote:
> 
>> Hi Sagi
>>
>> On 02/11/2018 05:53 PM, Sagi Grimberg wrote:
>>> Actually, they are already in nvme-4.16-rc.
>>>
>>> I'd like to pick these up as well once they converge (even
>>> taking 1+3 would be a good start).
>>
>> Please don't refer to my test result here.
>> It is tested on my patchset, not the currently source code on your git tree.
> 
> OK, what I don't understand is what is your feedback on this patchset as
> I'd like to take it for 4.16-rc and I want a review for it

This bug was seen when I tested the patchset nvme-pci: fixes on nvme_timeout and nvme_dev_disable patchset.
Because it is also a issue for current source code, so I reported this and had some talking with Keith,
then Keith gave this solution (Really appreciate for this).
Then I ported this with the patchset nvme-pci: fixes on nvme_timeout and nvme_dev_disable patchset and the issue
I met was gone. So I posted a feedback here as you saw.

It is certainly fine to fix that issue.

Really sorry for bothering you.


Sincerely
Jianchao	

[PATCH 1/3] nvme-pci: Fix timeouts in connecting state

[jianchao.wang]

Hi Keith

Thanks for your time and patch for this.

Hi Keith

On 02/10/2018 01:41 AM, Keith Busch wrote:
> We need to halt the controller immediately if we haven't completed
> initialization as indicated by the new "connecting" state.
> 
> Fixes: ad70062cdb ("nvme-pci: introduce RECONNECTING state to mark initializing procedure")
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
>  drivers/nvme/host/pci.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index ab9c19525fa8..90e276c05f79 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1215,13 +1215,17 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
>  	 * cancellation error. All outstanding requests are completed on
>  	 * shutdown, so we return BLK_EH_HANDLED.
>  	 */
> -	if (dev->ctrl.state == NVME_CTRL_RESETTING) {
> +	switch (dev->ctrl.state) {
> +	case NVME_CTRL_CONNECTING:
> +	case NVME_CTRL_RESETTING:
>  		dev_warn(dev->ctrl.device,
>  			 "I/O %d QID %d timeout, disable controller\n",
>  			 req->tag, nvmeq->qid);
>  		nvme_dev_disable(dev, false);
>  		nvme_req(req)->flags |= NVME_REQ_CANCELLED;
>  		return BLK_EH_HANDLED;
> +	default:
> +		break;
>  	}
>  
>  	/*
> 
This patch should be merged with the 3th one.

Thanks
Jianchao

[PATCH 1/3] nvme-pci: Fix timeouts in connecting state

[Keith Busch]

On Sat, Feb 10, 2018@10:14:46AM +0800, jianchao.wang wrote:
> This patch should be merged with the 3th one.

Why? This one fixes a regression introduced in the merge window, the
other is fixes something entirely different.

[PATCH 1/3] nvme-pci: Fix timeouts in connecting state

[jianchao.wang]

Hi Keith

Thanks for your kindly response.

On 02/12/2018 10:18 PM, Keith Busch wrote:
> On Sat, Feb 10, 2018@10:14:46AM +0800, jianchao.wang wrote:
>> This patch should be merged with the 3th one.
> 
> Why? This one fixes a regression introduced in the merge window, the
> other is fixes something entirely different.
> 

This patch could not fix the issue totally, it will not work well with
your 3rd patch.

Thanks
Jianchao

[PATCH 1/3] nvme-pci: Fix timeouts in connecting state

[jianchao.wang]

Really sorry for misspell

On 02/13/2018 10:21 AM, jianchao.wang wrote:
> This patch could not fix the issue totally, it will not work well with
> your 3rd patch

s/with/without

Sincerely
Jianchao

[PATCH 1/3] nvme-pci: Fix timeouts in connecting state

[Keith Busch]

On Tue, Feb 13, 2018@10:33:03AM +0800, jianchao.wang wrote:
> On 02/13/2018 10:21 AM, jianchao.wang wrote:
> > This patch could not fix the issue totally, it will not work well with
> > your 3rd patch
> 
> s/with/without

The first patch fixes a regression introduced in this merge window and
gets us back to functionality we had before. The third patch fixes a
theoretical issue that AFAIK has never actually happened in real life.