[PATCH v2 1/3] nvme: use blk_mq_start_hw_queues() in nvme_kill_queues()

[PATCH v2 1/3] nvme: use blk_mq_start_hw_queues() in nvme_kill_queues()

[Ming Lei]

Inside nvme_kill_queues(), we have to start hw queues for
draining requests in sw queues, .dispatch list and requeue list,
so use blk_mq_start_hw_queues() instead of blk_mq_start_stopped_hw_queues()
which only run queues if queues are stopped, but the queues may have
been started already, for example nvme_start_queues() is called in reset work
function.

blk_mq_start_hw_queues() run hw queues in current context, instead
of running asynchronously like before. Given nvme_kill_queues() is
run from either remove context or reset worker context, both are fine
to run hw queue directly. And the mutex of namespaces_mutex isn't a
problem too becasue nvme_start_freeze() runs hw queue in this way
already.

Cc: stable@vger.kernel.org
Reported-by: Zhang Yi <yizhan@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/nvme/host/core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d5e0906262ea..ce0d96913ee6 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2437,7 +2437,13 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
 		revalidate_disk(ns->disk);
 		blk_set_queue_dying(ns->queue);
 		blk_mq_abort_requeue_list(ns->queue);
-		blk_mq_start_stopped_hw_queues(ns->queue, true);
+
+		/*
+		 * We have to force to start queues for avoiding hang
+		 * forever, and we have to make sure that queues won't
+		 * be stopped forever from now on.
+		 */
+		blk_mq_start_hw_queues(ns->queue);
 	}
 	mutex_unlock(&ctrl->namespaces_mutex);
 }
-- 
2.9.4

Re: [PATCH v2 1/3] nvme: use blk_mq_start_hw_queues() in nvme_kill_queues()

[Christoph Hellwig]

> index d5e0906262ea..ce0d96913ee6 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2437,7 +2437,13 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
>  		revalidate_disk(ns->disk);
>  		blk_set_queue_dying(ns->queue);
>  		blk_mq_abort_requeue_list(ns->queue);
> -		blk_mq_start_stopped_hw_queues(ns->queue, true);
> +
> +		/*
> +		 * We have to force to start queues for avoiding hang
> +		 * forever, and we have to make sure that queues won't
> +		 * be stopped forever from now on.
> +		 */

		/*
		 * Forcibly start all queues to avoid having stuck requests.
		 * Note: We must make sure to not stop the queues from
		 * now until the final removal.
		 */

Otherwise this looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 1/3] nvme: use blk_mq_start_hw_queues() in nvme_kill_queues()

[Ming Lei]

On Sun, May 21, 2017 at 08:20:02AM +0200, Christoph Hellwig wrote:
> > index d5e0906262ea..ce0d96913ee6 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -2437,7 +2437,13 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
> >  		revalidate_disk(ns->disk);
> >  		blk_set_queue_dying(ns->queue);
> >  		blk_mq_abort_requeue_list(ns->queue);
> > -		blk_mq_start_stopped_hw_queues(ns->queue, true);
> > +
> > +		/*
> > +		 * We have to force to start queues for avoiding hang
> > +		 * forever, and we have to make sure that queues won't
> > +		 * be stopped forever from now on.
> > +		 */
> 
> 		/*
> 		 * Forcibly start all queues to avoid having stuck requests.

The above is better.

> 		 * Note: We must make sure to not stop the queues from
> 		 * now until the final removal.

In theory, it should be OK to stop and start queues again before the final
removal, so how about the following:

 		 * Note: We must make sure to not put the queues into being stopped
		 forever from now until the final removal.

Thanks,
Ming

Re: [PATCH v2 1/3] nvme: use blk_mq_start_hw_queues() in nvme_kill_queues()

[Christoph Hellwig]

On Mon, May 22, 2017 at 09:35:58AM +0800, Ming Lei wrote:
> In theory, it should be OK to stop and start queues again before the final
> removal, so how about the following:
> 
>  		 * Note: We must make sure to not put the queues into being stopped
> 		 forever from now until the final removal.

Maybe:
		 * Note that we must ensure the queues are not stopped
		 * when the final removal happens.

?

Re: [PATCH v2 1/3] nvme: use blk_mq_start_hw_queues() in nvme_kill_queues()

[Ming Lei]

On Mon, May 22, 2017 at 02:51:40PM +0200, Christoph Hellwig wrote:
> On Mon, May 22, 2017 at 09:35:58AM +0800, Ming Lei wrote:
> > In theory, it should be OK to stop and start queues again before the final
> > removal, so how about the following:
> > 
> >  		 * Note: We must make sure to not put the queues into being stopped
> > 		 forever from now until the final removal.
> 
> Maybe:
> 		 * Note that we must ensure the queues are not stopped
> 		 * when the final removal happens.
> 
> ?

Yeah, that is it, :-)

Thanks,
Ming

Re: [PATCH v2 1/3] nvme: use blk_mq_start_hw_queues() in nvme_kill_queues()

[Keith Busch]

Looks good.

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

Re: [PATCH v2 1/3] nvme: use blk_mq_start_hw_queues() in nvme_kill_queues()

[Johannes Thumshirn]

On 05/20/2017 05:56 AM, Ming Lei wrote:
> Inside nvme_kill_queues(), we have to start hw queues for
> draining requests in sw queues, .dispatch list and requeue list,
> so use blk_mq_start_hw_queues() instead of blk_mq_start_stopped_hw_queues()
> which only run queues if queues are stopped, but the queues may have
> been started already, for example nvme_start_queues() is called in reset work
> function.
> 
> blk_mq_start_hw_queues() run hw queues in current context, instead
> of running asynchronously like before. Given nvme_kill_queues() is
> run from either remove context or reset worker context, both are fine
> to run hw queue directly. And the mutex of namespaces_mutex isn't a
> problem too becasue nvme_start_freeze() runs hw queue in this way
> already.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Zhang Yi <yizhan@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---


Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>


-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH v2 2/3] nvme: avoid to use blk_mq_abort_requeue_list()

[Ming Lei]

NVMe may add request into requeue list simply and not kick off the
requeue if hw queues are stopped. Then blk_mq_abort_requeue_list()
is called in both nvme_kill_queues() and nvme_ns_remove() for
dealing with this issue.

Unfortunately blk_mq_abort_requeue_list() is absolutely a
race maker, for example, one request may be requeued during
the aborting. So this patch just calls blk_mq_kick_requeue_list() in
nvme_kill_queues() to handle this issue like what nvme_start_queues()
does. Now all requests in requeue list when queues are stopped will be
handled by blk_mq_kick_requeue_list() when queues are restarted, either
in nvme_start_queues() or in nvme_kill_queues().

Cc: stable@vger.kernel.org
Reported-by: Zhang Yi <yizhan@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/nvme/host/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ce0d96913ee6..9ce0a0a16984 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2098,7 +2098,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 		if (ns->ndev)
 			nvme_nvm_unregister_sysfs(ns);
 		del_gendisk(ns->disk);
-		blk_mq_abort_requeue_list(ns->queue);
 		blk_cleanup_queue(ns->queue);
 	}
 
@@ -2436,7 +2435,6 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
 			continue;
 		revalidate_disk(ns->disk);
 		blk_set_queue_dying(ns->queue);
-		blk_mq_abort_requeue_list(ns->queue);
 
 		/*
 		 * We have to force to start queues for avoiding hang
@@ -2444,6 +2442,9 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
 		 * be stopped forever from now on.
 		 */
 		blk_mq_start_hw_queues(ns->queue);
+
+		/* draining requests in requeue list */
+		blk_mq_kick_requeue_list(q);
 	}
 	mutex_unlock(&ctrl->namespaces_mutex);
 }
-- 
2.9.4

Re: [PATCH v2 2/3] nvme: avoid to use blk_mq_abort_requeue_list()

[Ming Lei]

On Sat, May 20, 2017 at 11:56:04AM +0800, Ming Lei wrote:
> NVMe may add request into requeue list simply and not kick off the
> requeue if hw queues are stopped. Then blk_mq_abort_requeue_list()
> is called in both nvme_kill_queues() and nvme_ns_remove() for
> dealing with this issue.
> 
> Unfortunately blk_mq_abort_requeue_list() is absolutely a
> race maker, for example, one request may be requeued during
> the aborting. So this patch just calls blk_mq_kick_requeue_list() in
> nvme_kill_queues() to handle this issue like what nvme_start_queues()
> does. Now all requests in requeue list when queues are stopped will be
> handled by blk_mq_kick_requeue_list() when queues are restarted, either
> in nvme_start_queues() or in nvme_kill_queues().
> 
> Cc: stable@vger.kernel.org
> Reported-by: Zhang Yi <yizhan@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/nvme/host/core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index ce0d96913ee6..9ce0a0a16984 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2098,7 +2098,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>  		if (ns->ndev)
>  			nvme_nvm_unregister_sysfs(ns);
>  		del_gendisk(ns->disk);
> -		blk_mq_abort_requeue_list(ns->queue);
>  		blk_cleanup_queue(ns->queue);
>  	}
>  
> @@ -2436,7 +2435,6 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
>  			continue;
>  		revalidate_disk(ns->disk);
>  		blk_set_queue_dying(ns->queue);
> -		blk_mq_abort_requeue_list(ns->queue);
>  
>  		/*
>  		 * We have to force to start queues for avoiding hang
> @@ -2444,6 +2442,9 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
>  		 * be stopped forever from now on.
>  		 */
>  		blk_mq_start_hw_queues(ns->queue);
> +
> +		/* draining requests in requeue list */
> +		blk_mq_kick_requeue_list(q);

oops, the above line causes build failure, please
take the following one.

---

>From e70cfe2edacf278e3e6605f8c08e01ebf65bff01 Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@redhat.com>
Date: Wed, 17 May 2017 09:02:07 +0800
Subject: [PATCH v2 2/3] nvme: avoid to use blk_mq_abort_requeue_list()

NVMe may add request into requeue list simply and not kick off the
requeue if hw queues are stopped. Then blk_mq_abort_requeue_list()
is called in both nvme_kill_queues() and nvme_ns_remove() for
dealing with this issue.

Unfortunately blk_mq_abort_requeue_list() is absolutely a
race maker, for example, one request may be requeued during
the aborting. So this patch just calls blk_mq_kick_requeue_list() in
nvme_kill_queues() to handle this issue like what nvme_start_queues()
does. Now all requests in requeue list when queues are stopped will be
handled by blk_mq_kick_requeue_list() when queues are restarted, either
in nvme_start_queues() or in nvme_kill_queues().

Cc: stable@vger.kernel.org
Reported-by: Zhang Yi <yizhan@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/nvme/host/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ce0d96913ee6..e344f5d7c1bc 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2098,7 +2098,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 		if (ns->ndev)
 			nvme_nvm_unregister_sysfs(ns);
 		del_gendisk(ns->disk);
-		blk_mq_abort_requeue_list(ns->queue);
 		blk_cleanup_queue(ns->queue);
 	}
 
@@ -2436,7 +2435,6 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
 			continue;
 		revalidate_disk(ns->disk);
 		blk_set_queue_dying(ns->queue);
-		blk_mq_abort_requeue_list(ns->queue);
 
 		/*
 		 * We have to force to start queues for avoiding hang
@@ -2444,6 +2442,9 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
 		 * be stopped forever from now on.
 		 */
 		blk_mq_start_hw_queues(ns->queue);
+
+		/* draining requests in requeue list */
+		blk_mq_kick_requeue_list(ns->queue);
 	}
 	mutex_unlock(&ctrl->namespaces_mutex);
 }
-- 
2.9.4

Re: [PATCH v2 2/3] nvme: avoid to use blk_mq_abort_requeue_list()

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/3] nvme: avoid to use blk_mq_abort_requeue_list()

[Johannes Thumshirn]

On 05/20/2017 07:34 AM, Ming Lei wrote:
> On Sat, May 20, 2017 at 11:56:04AM +0800, Ming Lei wrote:
>> NVMe may add request into requeue list simply and not kick off the
>> requeue if hw queues are stopped. Then blk_mq_abort_requeue_list()
>> is called in both nvme_kill_queues() and nvme_ns_remove() for
>> dealing with this issue.
>>
>> Unfortunately blk_mq_abort_requeue_list() is absolutely a
>> race maker, for example, one request may be requeued during
>> the aborting. So this patch just calls blk_mq_kick_requeue_list() in
>> nvme_kill_queues() to handle this issue like what nvme_start_queues()
>> does. Now all requests in requeue list when queues are stopped will be
>> handled by blk_mq_kick_requeue_list() when queues are restarted, either
>> in nvme_start_queues() or in nvme_kill_queues().
>>
>> Cc: stable@vger.kernel.org
>> Reported-by: Zhang Yi <yizhan@redhat.com>
>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>> ---
>>  drivers/nvme/host/core.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index ce0d96913ee6..9ce0a0a16984 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -2098,7 +2098,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>>  		if (ns->ndev)
>>  			nvme_nvm_unregister_sysfs(ns);
>>  		del_gendisk(ns->disk);
>> -		blk_mq_abort_requeue_list(ns->queue);
>>  		blk_cleanup_queue(ns->queue);
>>  	}
>>  
>> @@ -2436,7 +2435,6 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
>>  			continue;
>>  		revalidate_disk(ns->disk);
>>  		blk_set_queue_dying(ns->queue);
>> -		blk_mq_abort_requeue_list(ns->queue);
>>  
>>  		/*
>>  		 * We have to force to start queues for avoiding hang
>> @@ -2444,6 +2442,9 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
>>  		 * be stopped forever from now on.
>>  		 */
>>  		blk_mq_start_hw_queues(ns->queue);
>> +
>> +		/* draining requests in requeue list */
>> +		blk_mq_kick_requeue_list(q);
> 
> oops, the above line causes build failure, please
> take the following one.
> 
> ---
> 
> From e70cfe2edacf278e3e6605f8c08e01ebf65bff01 Mon Sep 17 00:00:00 2001
> From: Ming Lei <ming.lei@redhat.com>
> Date: Wed, 17 May 2017 09:02:07 +0800
> Subject: [PATCH v2 2/3] nvme: avoid to use blk_mq_abort_requeue_list()
> 
> NVMe may add request into requeue list simply and not kick off the
> requeue if hw queues are stopped. Then blk_mq_abort_requeue_list()
> is called in both nvme_kill_queues() and nvme_ns_remove() for
> dealing with this issue.
> 
> Unfortunately blk_mq_abort_requeue_list() is absolutely a
> race maker, for example, one request may be requeued during
> the aborting. So this patch just calls blk_mq_kick_requeue_list() in
> nvme_kill_queues() to handle this issue like what nvme_start_queues()
> does. Now all requests in requeue list when queues are stopped will be
> handled by blk_mq_kick_requeue_list() when queues are restarted, either
> in nvme_start_queues() or in nvme_kill_queues().
> 
> Cc: stable@vger.kernel.org
> Reported-by: Zhang Yi <yizhan@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>


-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH v2 2/3] nvme: avoid to use blk_mq_abort_requeue_list()

[Keith Busch]

Looks good.

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