Re: [PATCH V3] blk-mq: introduce BLK_STS_DEV_RESOURCE

[Bart Van Assche <Bart.VanAssche@wdc.com>]

On Wed, 2018-01-24 at 00:16 +0800, Ming Lei wrote:
> @@ -1280,10 +1282,18 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
>  		 * - Some but not all block drivers stop a queue before
>  		 *   returning BLK_STS_RESOURCE. Two exceptions are scsi-mq
>  		 *   and dm-rq.
> +		 *
> +		 * If drivers return BLK_STS_RESOURCE and S_SCHED_RESTART
> +		 * bit is set, run queue after 10ms for avoiding IO hang
> +		 * because the queue may be idle and the RESTART mechanism
> +		 * can't work any more.
>  		 */
> -		if (!blk_mq_sched_needs_restart(hctx) ||
> +		needs_restart = blk_mq_sched_needs_restart(hctx);
> +		if (!needs_restart ||
>  		    (no_tag && list_empty_careful(&hctx->dispatch_wait.entry)))
>  			blk_mq_run_hw_queue(hctx, true);
> +		else if (needs_restart && (ret == BLK_STS_RESOURCE))
> +			blk_mq_delay_run_hw_queue(hctx, 10);
>  	}

My opinion about this patch is as follows:
* Changing a blk_mq_delay_run_hw_queue() call followed by return
  BLK_STS_DEV_RESOURCE into return BLK_STS_RESOURCE is wrong because it changes
  a guaranteed queue rerun into a queue rerun that may or may not happen
  depending on whether or not multiple queue runs happen simultaneously.
* This change makes block drivers less readable because anyone who encounters
  BLK_STS_DEV_RESOURCE will have to look up its definition to figure out what
  it's meaning is.
* We don't need the new status code BLK_STS_DEV_RESOURCE because a delayed
  queue run can be implemented easily with the existing block layer API.

Bart.