On Sat, May 20, 2017 at 11:30:59PM -0700, Christoph Hellwig wrote:
> 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.
OK, will do it in v2.
>
> > 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);
That need to change current elevator interface(pass q instead of hctx), so
I think better to not do that in this patch.
>
> 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..)
The check may need to be removed in the future, since every blk-mq driver
sets BLK_MQ_F_SHOULD_MERGE now.
Thanks,
Ming
