| Message ID | 20170520035605.21785-2-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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>
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
Looks good.
Reviewed-by: Keith Busch <keith.busch@intel.com>
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>
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. ?
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
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); }
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(-)