Re: [PATCH V2 5/5] nvme: pci: simplify timeout handling

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

On Wed, May 02, 2018 at 10:23:20AM +0800, jianchao.wang wrote:
> Hi Ming
> 
> On 04/29/2018 11:41 PM, Ming Lei wrote:
> > +
> >  static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
> >  {
> >  	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> > @@ -1197,8 +1297,7 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
> >  	 */
> >  	if (nvme_should_reset(dev, csts)) {
> >  		nvme_warn_reset(dev, csts);
> > -		nvme_dev_disable(dev, false);
> > -		nvme_reset_ctrl(&dev->ctrl);
> > +		nvme_eh_schedule(dev);
> >  		return BLK_EH_HANDLED;
> >  	}
> >  
> > @@ -1224,8 +1323,8 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
> >  		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;
> > +		nvme_eh_schedule(dev);
> >  		return BLK_EH_HANDLED;
> >  	default:
> >  		break;
> > @@ -1240,14 +1339,12 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
> >  		dev_warn(dev->ctrl.device,
> >  			 "I/O %d QID %d timeout, reset controller\n",
> >  			 req->tag, nvmeq->qid);
> > -		nvme_dev_disable(dev, false);
> > -		nvme_reset_ctrl(&dev->ctrl);
> > -
> >  		/*
> >  		 * Mark the request as handled, since the inline shutdown
> >  		 * forces all outstanding requests to complete.
> >  		 */
> >  		nvme_req(req)->flags |= NVME_REQ_CANCELLED;
> > +		nvme_eh_schedule(dev);
> >  		return BLK_EH_HANDLED;
> >  	}
> >  
> > @@ -2246,8 +2343,8 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
> >  	if (pci_is_enabled(pdev)) {
> >  		u32 csts = readl(dev->bar + NVME_REG_CSTS);
> >  
> > -		if (dev->ctrl.state == NVME_CTRL_LIVE ||
> > -		    dev->ctrl.state == NVME_CTRL_RESETTING) {
> > +		if (shutdown && (dev->ctrl.state == NVME_CTRL_LIVE ||
> > +		    dev->ctrl.state == NVME_CTRL_RESETTING)) {
> >  			nvme_start_freeze(&dev->ctrl);
> >  			frozen = true;
> >  		}
> > @@ -2281,11 +2378,23 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
> >  	for (i = dev->ctrl.queue_count - 1; i >= 0; i--)
> >  		nvme_suspend_queue(&dev->queues[i]);
> >  
> > +	/* safe to sync timeout after queues are quiesced */
> > +	nvme_quiesce_timeout(&dev->ctrl);
> > +	blk_quiesce_timeout(dev->ctrl.admin_q);
> > +
> >  	nvme_pci_disable(dev);
> >  
> > +	/*
> > +	 * Both timeout and interrupt handler have been drained, and all
> > +	 * in-flight requests will be canceled now.
> > +	 */
> >  	blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
> >  	blk_mq_tagset_busy_iter(&dev->admin_tagset, nvme_cancel_request, &dev->ctrl);
> 
> 
> We need to return BLK_EH_RESET_TIMER in nvme_timeout then:
> 1. defer the completion. we can't unmap the io request before close the controller totally, so not BLK_EH_HANDLED.
> 2. nvme_cancel_request could complete it. blk_mq_complete_request is invoked by nvme_cancel_request, so not BLK_EH_NOT_HANDLED.

We don't need to change return value of .timeout() any more after
calling nvme_quiesce_timeout():

Thanks the single EH kthread, once nvme_quiesce_timeout() returns, all
timed-out requests have been handled already. Some of them may be completed,
and others may be handled as RESET_TIMER, but none of them are handled as
NOT_HANDLED because nvme_timeout() won't return that value.

So the following blk_mq_tagset_busy_iter(nvme_cancel_request) can handle
all in-flight requests because timeout is drained and quiesced.

Thanks,
Ming