Re: [PATCH 5/5] blk-mq: make plug work for mutiple disks and queues

[Jeff Moyer <jmoyer@redhat.com>]

Shaohua Li <shli@fb.com> writes:

> Last patch makes plug work for multiple queue case. However it only
> works for single disk case, because it assumes only one request in the
> plug list. If a task is accessing multiple disks, eg MD/DM, the
> assumption is wrong. Let blk_attempt_plug_merge() record request from
> the same queue.

I understand the desire to be performant, and that's why you
piggy-backed the same_queue_rq onto this function, but it sure looks
hackish.  I'd almost rather walk the list a second time instead of add
warts like this.  To be perfectly clear, this is what I really don't
like about it: we're relying on the nuance that we will only add a
single request per queue to the plug_list in the mq case.  When you
start to look at what happens for the sq case, it doesn't make much
sense at all, as you'll just return the first entry in the list (last
one visited walking the list backwards) that has the same request queue.
That's fine if this code were mq specific, but it isn't.  It will just
lead to confusion for others reviewing the code, and may trip up anyone
modifying the mq plugging code.

I'll leave it up to Jens to decide if this is fit for inclusion.  The
patch /functionally/ looks fine to me.

> Cc: Jens Axboe <axboe@fb.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  block/blk-core.c | 10 +++++++---
>  block/blk-mq.c   | 11 ++++++-----
>  block/blk.h      |  3 ++-
>  3 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index d51ed61..a5e1574 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1521,7 +1521,8 @@ bool bio_attempt_front_merge(struct request_queue *q, struct request *req,
>   * Caller must ensure !blk_queue_nomerges(q) beforehand.
>   */
>  bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
> -			    unsigned int *request_count)
> +			    unsigned int *request_count,
> +			    struct request **same_queue_rq)
>  {
>  	struct blk_plug *plug;
>  	struct request *rq;
> @@ -1541,8 +1542,10 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
>  	list_for_each_entry_reverse(rq, plug_list, queuelist) {
>  		int el_ret;
>  
> -		if (rq->q == q)
> +		if (rq->q == q) {
>  			(*request_count)++;
> +			*same_queue_rq = rq;
> +		}

Out of the 3 callers of blk_attempt_plug_merge, only one will use the
result, yet all of them have to provide the argument.  How about just
handling NULL in there?

Cheers,
Jeff