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

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

On Wed, Aug 30, 2017 at 09:22:42AM -0600, Jens Axboe wrote:
> 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

reorder_tags_to_front() is for reordering the requests in current list,
this patch is for splicing list into hctx->dispatch, so I can't see
it isn't safe, or could you explain it a bit?

> that than make this change.
> 
> What's your reasoning here? Your changelog doesn't really explain why

Firstly the 2nd patch need to add one rq(such as RQF_PM) to the
front of the hw queue, the simple way is to add it to the front
of hctx->dispatch. Without this change, the 2nd patch can't work
at all.

Secondly this way is still reasonable:

	- one rq is added to hctx->dispatch because queue is busy
	- another rq is added to hctx->dispatch too because of same reason

so it is reasonable to to add list into hctx->dispatch in FIFO style.

Finally my patchset for 'improving SCSI-MQ perf' will change to not
dequeue rq from sw/scheduler if ->dispatch isn't flushed. I believe
it is reasonable and correct thing to do, with that change, there
won't be difference between the two styles.


-- 
Ming