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

["jianchao.wang" <jianchao.w.wang@oracle.com>]

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.

Thanks
Jianchao