[PATCH] block: remove plug based merging

[PATCH] block: remove plug based merging

[Jens Axboe]

It's expensive to browse the whole plug list for merge opportunities at
the IOPS rates that modern storage can do. For sequential IO, the one-hit
cached merge should suffice on fast drives, and for rotational storage the
IO scheduler will do a more exhaustive lookup based merge anyway.

Just remove the plug based O(N) traversal merging.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 762da71f9fde..e10f49decd06 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -1043,62 +1043,6 @@ static enum bio_merge_status blk_attempt_bio_merge(struct request_queue *q,
 	return BIO_MERGE_FAILED;
 }
 
-/**
- * blk_attempt_plug_merge - try to merge with %current's plugged list
- * @q: request_queue new bio is being queued at
- * @bio: new bio being queued
- * @nr_segs: number of segments in @bio
- * @same_queue_rq: pointer to &struct request that gets filled in when
- * another request associated with @q is found on the plug list
- * (optional, may be %NULL)
- *
- * Determine whether @bio being queued on @q can be merged with a request
- * on %current's plugged list.  Returns %true if merge was successful,
- * otherwise %false.
- *
- * Plugging coalesces IOs from the same issuer for the same purpose without
- * going through @q->queue_lock.  As such it's more of an issuing mechanism
- * than scheduling, and the request, while may have elvpriv data, is not
- * added on the elevator at this point.  In addition, we don't have
- * reliable access to the elevator outside queue lock.  Only check basic
- * merging parameters without querying the elevator.
- *
- * Caller must ensure !blk_queue_nomerges(q) beforehand.
- */
-bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
-		unsigned int nr_segs, struct request **same_queue_rq)
-{
-	struct blk_plug *plug;
-	struct request *rq;
-	struct list_head *plug_list;
-
-	plug = blk_mq_plug(q, bio);
-	if (!plug)
-		return false;
-
-	plug_list = &plug->mq_list;
-
-	list_for_each_entry_reverse(rq, plug_list, queuelist) {
-		if (rq->q == q && same_queue_rq) {
-			/*
-			 * Only blk-mq multiple hardware queues case checks the
-			 * rq in the same queue, there should be only one such
-			 * rq in a queue
-			 **/
-			*same_queue_rq = rq;
-		}
-
-		if (rq->q != q)
-			continue;
-
-		if (blk_attempt_bio_merge(q, rq, bio, nr_segs, false) ==
-		    BIO_MERGE_OK)
-			return true;
-	}
-
-	return false;
-}
-
 /*
  * Iterate list of requests and see if we can merge this bio with any
  * of them.
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5777064f1174..4c5b34787bbf 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2323,10 +2323,6 @@ void blk_mq_submit_bio(struct bio *bio)
 	if (!bio_integrity_prep(bio))
 		goto queue_exit;
 
-	if (!is_flush_fua && !blk_queue_nomerges(q) &&
-	    blk_attempt_plug_merge(q, bio, nr_segs, &same_queue_rq))
-		goto queue_exit;
-
 	if (blk_mq_sched_bio_merge(q, bio, nr_segs))
 		goto queue_exit;
 
diff --git a/block/blk.h b/block/blk.h
index fa23338449ed..0afee3e6a7c1 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -214,8 +214,6 @@ static inline void blk_integrity_del(struct gendisk *disk)
 unsigned long blk_rq_timeout(unsigned long timeout);
 void blk_add_timer(struct request *req);
 
-bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
-		unsigned int nr_segs, struct request **same_queue_rq);
 bool blk_bio_list_merge(struct request_queue *q, struct list_head *list,
 			struct bio *bio, unsigned int nr_segs);
 
-- 
Jens Axboe

Re: [PATCH] block: remove plug based merging

[Christoph Hellwig]

On Tue, Oct 12, 2021 at 12:12:39PM -0600, Jens Axboe wrote:
> It's expensive to browse the whole plug list for merge opportunities at
> the IOPS rates that modern storage can do. For sequential IO, the one-hit
> cached merge should suffice on fast drives, and for rotational storage the
> IO scheduler will do a more exhaustive lookup based merge anyway.

I don't really want to argue, but maybe some actual measurements to
support the above claims would be useful in the commit log?

Re: [PATCH] block: remove plug based merging

[Jens Axboe]

On 10/13/21 11:20 PM, Christoph Hellwig wrote:
> On Tue, Oct 12, 2021 at 12:12:39PM -0600, Jens Axboe wrote:
>> It's expensive to browse the whole plug list for merge opportunities at
>> the IOPS rates that modern storage can do. For sequential IO, the one-hit
>> cached merge should suffice on fast drives, and for rotational storage the
>> IO scheduler will do a more exhaustive lookup based merge anyway.
> 
> I don't really want to argue, but maybe some actual measurements to
> support the above claims would be useful in the commit log?

Sure, I do need to pad the commit messages a bit. One example - running
an IOPS bound worload caps out at ~5.6M IOPS and if you check the profile
this is top of the list:

  Overhead  Command   Shared Object     Symbol                                         
+   20.89%  io_uring  [kernel.vmlinux]  [k] blk_attempt_plug_merge
+    4.98%  io_uring  [kernel.vmlinux]  [k] io_submit_sqes
+    4.78%  io_uring  [kernel.vmlinux]  [k] blkdev_direct_IO
+    4.61%  io_uring  [kernel.vmlinux]  [k] blk_mq_submit_bio
+    3.67%  io_uring  [nvme]            [k] nvme_queue_rq

Disabling merging or applying the patch, we run at ~7.4M IOPS instead.
That's about a 33% improvement from killing a silly merge loop that
isn't even marked as an expensive merge.

I'll rework the patch, we can probably retain it is a last-insert kind
of merge point. But browsing the whole plug list for a merge candidate
is stupidity, regardless of the class of workload and storage.

-- 
Jens Axboe