Re: [PATCH V5 0/9] nvme: pci: fix & improve timeout handling

[Ming Lei <ming.lei@redhat.com>]

On Tue, May 15, 2018 at 05:56:14PM +0800, jianchao.wang wrote:
> Hi ming
> 
> On 05/15/2018 08:33 AM, Ming Lei wrote:
> > We still have to quiesce admin queue before canceling request, so looks
> > the following patch is better, so please ignore the above patch and try
> > the following one and see if your hang can be addressed:
> > 
> > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> > index f509d37b2fb8..c2adc76472a8 100644
> > --- a/drivers/nvme/host/pci.c
> > +++ b/drivers/nvme/host/pci.c
> > @@ -1741,8 +1741,7 @@ static int nvme_alloc_admin_tags(struct nvme_dev *dev)
> >  			dev->ctrl.admin_q = NULL;
> >  			return -ENODEV;
> >  		}
> > -	} else
> > -		blk_mq_unquiesce_queue(dev->ctrl.admin_q);
> > +	}
> >  
> >  	return 0;
> >  }
> > @@ -2520,6 +2519,12 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown, bool
> >  	 */
> >  	if (shutdown)
> >  		nvme_start_queues(&dev->ctrl);
> > +
> > +	/*
> > +	 * Avoid to suck reset because timeout may happen during reset and
> > +	 * reset may hang forever if admin queue is kept as quiesced
> > +	 */
> > +	blk_mq_unquiesce_queue(dev->ctrl.admin_q);
> >  	mutex_unlock(&dev->shutdown_lock);
> >  }
> 
> w/ patch above and patch below, both the warning and io hung issue didn't reproduce till now.
> 

That is great to see another issue which can be covered now, :-)

> 
> @@ -1450,6 +1648,7 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
>  {
>         struct nvme_dev *dev = nvmeq->dev;
>         int result;
> +       int cq_vector;
>  
>         if (dev->cmb && use_cmb_sqes && (dev->cmbsz & NVME_CMBSZ_SQS)) {
>                 unsigned offset = (qid - 1) * roundup(SQ_SIZE(nvmeq->q_depth),
> @@ -1462,15 +1661,16 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
>          * A queue's vector matches the queue identifier unless the controller
>          * has only one vector available.
>          */
> -       nvmeq->cq_vector = dev->num_vecs == 1 ? 0 : qid;
> -       result = adapter_alloc_cq(dev, qid, nvmeq);
> +       cq_vector = dev->num_vecs == 1 ? 0 : qid;
> +       result = adapter_alloc_cq(dev, qid, nvmeq, cq_vector);
>         if (result < 0)
> -               goto release_vector;
> +               goto out;
>  
>         result = adapter_alloc_sq(dev, qid, nvmeq);
>         if (result < 0)
>                 goto release_cq;
> -
> +       
> +       nvmeq->cq_vector = cq_vector;
>         nvme_init_queue(nvmeq, qid);
>         result = queue_request_irq(nvmeq);
>         if (result < 0)
> @@ -1479,12 +1679,12 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
>         return result;
>  
>   release_sq:
> +       nvmeq->cq_vector = -1;
>         dev->online_queues--;
>         adapter_delete_sq(dev, qid);
>   release_cq:
>         adapter_delete_cq(dev, qid);
> - release_vector:
> -       nvmeq->cq_vector = -1;
> + out:
>         return result;
>  }
> 

Looks a nice fix on nvme_create_queue(), but seems the change on 
adapter_alloc_cq() is missed in above patch.

Could you prepare a formal one so that I may integrate it to V6?

Thanks,
Ming