Re: [PATCH 2/2] block: Split and submit bios in LBA order

[Jan Kara <jack@suse.cz>]

On Fri 17-03-23 12:59:38, Bart Van Assche wrote:
> Instead of submitting the bio fragment with the highest LBA first,
> submit the bio fragment with the lowest LBA first. If plugging is
> active, append requests at the end of the list with plugged requests
> instead of at the start. This approach prevents write errors when
> submitting large bios to host-managed zoned block devices.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Cc: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

...

> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index cc32ad0cd548..9b0f9f3fdba0 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1300,7 +1300,7 @@ static inline unsigned short blk_plug_max_rq_count(struct blk_plug *plug)
>  
>  static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq)
>  {
> -	struct request *last = rq_list_peek(&plug->mq_list);
> +	struct request *last = rq_list_peek(&plug->mq_list), **last_p;
>  
>  	if (!plug->rq_count) {
>  		trace_block_plug(rq->q);
> @@ -1317,7 +1317,10 @@ static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq)
>  	if (!plug->has_elevator && (rq->rq_flags & RQF_ELV))
>  		plug->has_elevator = true;
>  	rq->rq_next = NULL;
> -	rq_list_add(&plug->mq_list, rq);
> +	last_p = &plug->mq_list;
> +	while (*last_p)
> +		last_p = &(*last_p)->rq_next;
> +	rq_list_add_tail(&last_p, rq);
>  	plug->rq_count++;
>  }

Uh, I don't think we want to traverse the whole plug list each time we are
adding a request to it. We have just recently managed to avoid that at
least for single-device cases and apparently it was a win for fast devices
(see commit d38a9c04c0d5 ("block: only check previous entry for plug merge
attempt")).

If anything, I'd rather modify blk_mq_dispatch_plug_list() to add requests
to the queuelist in reverse order in this case, like I did it in
34e0a279a993 ("block: do not reverse request order when flushing plug
list") which is now in Jens' tree.

> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index dd5ce1137f04..5e01791967c0 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -228,6 +228,12 @@ static inline unsigned short req_get_ioprio(struct request *req)
>  	*(listptr) = rq;				\
>  } while (0)
>  
> +#define rq_list_add_tail(lastpptr, rq) do {		\
> +	(rq)->rq_next = NULL;				\
> +	**(lastpptr) = rq;				\
> +	*(lastpptr) = &rq->rq_next;			\
> +} while (0)
> +

And this function should be already in Jens's tree as part of commit
34e0a279a993 ("block: do not reverse request order when flushing plug
list").

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR