Re: [PATCH 1/2] blk-mq: add requests in the tail of hctx->dispatch

[Jens Axboe <axboe@kernel.dk>]

On 08/30/2017 09:19 AM, Ming Lei wrote:
> It is more reasonable to add requests to ->dispatch in way
> of FIFO style, instead of LIFO style.
> 
> Also in this way, we can allow to insert request at the front
> of hw queue, which function is needed to fix one bug
> in blk-mq's implementation of blk_execute_rq()
> 
> Reported-by: Oleksandr Natalenko <oleksandr@natalenko.name>
> Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq-sched.c | 2 +-
>  block/blk-mq.c       | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index 4ab69435708c..8d97df40fc28 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -272,7 +272,7 @@ static bool blk_mq_sched_bypass_insert(struct blk_mq_hw_ctx *hctx,
>  	 * the dispatch list.
>  	 */
>  	spin_lock(&hctx->lock);
> -	list_add(&rq->queuelist, &hctx->dispatch);
> +	list_add_tail(&rq->queuelist, &hctx->dispatch);
>  	spin_unlock(&hctx->lock);
>  	return true;
>  }
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 4603b115e234..fed3d0c16266 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1067,7 +1067,7 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
>  		blk_mq_put_driver_tag(rq);
>  
>  		spin_lock(&hctx->lock);
> -		list_splice_init(list, &hctx->dispatch);
> +		list_splice_tail_init(list, &hctx->dispatch);
>  		spin_unlock(&hctx->lock);

I'm not convinced this is safe, there's actually a reason why the
request is added to the front and not the back. We do have
reorder_tags_to_front() as a safe guard, but I'd much rather get rid of
that than make this change.

What's your reasoning here? Your changelog doesn't really explain why
this fixes anything, it's very vague.

-- 
Jens Axboe