Re: [PATCH] blk-mq: provide a default .bio_merge

[Christoph Hellwig <hch@infradead.org>]

On Sat, May 13, 2017 at 12:20:54AM +0800, Ming Lei wrote:
> Before blk-mq is introduced, I/O is merged before putting into
> plug queue, but blk-mq changed the order and makes merging
> basically impossible until mq-deadline is introduced. Then it
> is observed that throughput of sequential I/O is degraded about
> 10%~20% on virtio-blk in the test[1] if IO schedluer isn't used.
> 
> This patch provides a default per-sw-queue bio merging if there
> isn't scheduler enabled or the scheduler hasn't implement .bio_merge(),
> and this way actually moves merging before plugging just
> like what blk_queue_bio() does, then the performance regression
> is fixed.

This looks generally reasonable, but can you split the move of
blk_mq_attempt_merge into a separate patch (or just skip it for now)?
This clutters up the diff a lot and makes it much harder to read.

>  bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio)
>  {
>  	struct elevator_queue *e = q->elevator;
> +	struct blk_mq_ctx *ctx = blk_mq_get_ctx(q);
> +	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, ctx->cpu);
> +	bool ret = false;
>  
> +	if (e && e->type->ops.mq.bio_merge) {
>  		blk_mq_put_ctx(ctx);
>  		return e->type->ops.mq.bio_merge(hctx, bio);
> +	} else if (hctx->flags & BLK_MQ_F_SHOULD_MERGE) {

No need for the relse here given the return.  Also both mq-deadline
and cfq don't need the hctx at all and just the queue, so we could even
skip it for that case.

	if (e && e->type->ops.mq.bio_merge)
		return e->type->ops.mq.bio_merge(q, bio);

	ctx = blk_mq_get_ctx(q);
	hctx = blk_mq_map_queue(q, ctx->cpu);
	if (hctx->flags & BLK_MQ_F_SHOULD_MERGE) {
		...
	}

(and we only need the hctx for the flags, sigh..)