Re: [PATCH V4 2/6] blk-mq: pass hctx to blk_mq_dispatch_rq_list

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

On Tue, Jun 02, 2020 at 09:39:18AM +0000, Damien Le Moal wrote:
> On 2020/06/02 18:32, Ming Lei wrote:
> > On Tue, Jun 02, 2020 at 09:25:01AM +0000, Damien Le Moal wrote:
> >> On 2020/06/02 18:15, Ming Lei wrote:
> >>> All requests in the 'list' of blk_mq_dispatch_rq_list belong to same
> >>> hctx, so it is better to pass hctx instead of request queue, because
> >>> blk-mq's dispatch target is hctx instead of request queue.
> >>>
> >>> Cc: Sagi Grimberg <sagi@grimberg.me>
> >>> Cc: Baolin Wang <baolin.wang7@gmail.com>
> >>> Cc: Christoph Hellwig <hch@infradead.org>
> >>> Reviewed-by: Christoph Hellwig <hch@lst.de>
> >>> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> >>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> >>> Tested-by: Baolin Wang <baolin.wang7@gmail.com>
> >>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> >>> ---
> >>>  block/blk-mq-sched.c | 14 ++++++--------
> >>>  block/blk-mq.c       |  6 +++---
> >>>  block/blk-mq.h       |  2 +-
> >>>  3 files changed, 10 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> >>> index a31e281e9d31..632c6f8b63f7 100644
> >>> --- a/block/blk-mq-sched.c
> >>> +++ b/block/blk-mq-sched.c
> >>> @@ -96,10 +96,9 @@ static int blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
> >>>  	struct elevator_queue *e = q->elevator;
> >>>  	LIST_HEAD(rq_list);
> >>>  	int ret = 0;
> >>> +	struct request *rq;
> >>>  
> >>>  	do {
> >>> -		struct request *rq;
> >>> -
> >>>  		if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
> >>>  			break;
> >>>  
> >>> @@ -131,7 +130,7 @@ static int blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
> >>>  		 * in blk_mq_dispatch_rq_list().
> >>>  		 */
> >>>  		list_add(&rq->queuelist, &rq_list);
> >>> -	} while (blk_mq_dispatch_rq_list(q, &rq_list, true));
> >>> +	} while (blk_mq_dispatch_rq_list(rq->mq_hctx, &rq_list, true));
> >>
> >> Why not use the hctx argument passed to the function instead of rq->mq_hctx ?
> > 
> > e->type->ops.dispatch_request(hctx) may return one request which's
> > .mq_hctx isn't same with the 'hctx' argument, so far bfq and deadline
> > may do that.
> 
> Ah, OK. But then all requests in rq_list may have different hctx. So is it wise
> to pass hctx as an argument to blk_mq_dispatch_rq_list() ? The loop in that

&rq_list is one single request list.

> function will still need to look at each rq hctx (hctx = rq->mq_hctx) for the
> budget. So the hctx argument may not be needed at all, no ? Am I missing something ?

The final patch will add batching dispatch support, and more requests in
same hctx will be added to this list, at that time, the hctx argument
becomes reasonable.


Thanks,
Ming