All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] block: only check previous entry for plug merge attempt
@ 2021-10-14 18:34 Jens Axboe
  2021-10-15 14:18 ` Pankaj Raghav
  2021-10-16  4:31 ` Christoph Hellwig
  0 siblings, 2 replies; 4+ messages in thread
From: Jens Axboe @ 2021-10-14 18:34 UTC (permalink / raw)
  To: linux-block

Currently we scan the entire plug list, which is potentially very
expensive. In an IOPS bound workload, we can drive about 5.6M IOPS with
merging enabled, and profiling shows that the plug merge check is the
(by far) most expensive thing we're doing:

  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

Instead of browsing the whole list, just check the previously inserted
entry. That is enough for a naive merge check and will catch most cases,
and for devices that need full merging, the IO scheduler attached to
such devices will do that anyway. The plug merge is meant to be an
inexpensive check to avoid getting a request, but if we repeatedly
scan the list for every single insert, it is very much not a cheap
check.

With this patch, the workload instead runs at ~7.0M IOPS, providing
a 25% improvement. Disabling merging entirely yields another 5%
improvement.

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

---

v3: retain merge list, but only check last addition.

diff --git a/block/blk-merge.c b/block/blk-merge.c
index f390a8753268..575080ad0617 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -1089,32 +1089,22 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
 {
 	struct blk_plug *plug;
 	struct request *rq;
-	struct list_head *plug_list;
 
 	plug = blk_mq_plug(q, bio);
-	if (!plug)
+	if (!plug || list_empty(&plug->mq_list))
 		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;
+	/* check the previously added entry for a quick merge attempt */
+	rq = list_last_entry(&plug->mq_list, struct request, 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 (blk_attempt_bio_merge(q, rq, bio, nr_segs, false) == BIO_MERGE_OK)
+		return true;
 	return false;
 }
 
-- 
Jens Axboe


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] block: only check previous entry for plug merge attempt
  2021-10-14 18:34 [PATCH v3] block: only check previous entry for plug merge attempt Jens Axboe
@ 2021-10-15 14:18 ` Pankaj Raghav
  2021-10-16 22:13   ` Jens Axboe
  2021-10-16  4:31 ` Christoph Hellwig
  1 sibling, 1 reply; 4+ messages in thread
From: Pankaj Raghav @ 2021-10-15 14:18 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-block, Pankaj Raghav

On Thu, Oct 14, 2021 at 12:34:21PM -0600, Jens Axboe wrote:
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index f390a8753268..575080ad0617 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c

Even though the code is self-documenting, the current description on top
of this function is now outdated with this change. 

- * Determine whether @bio being queued on @q can be merged with a request
- * on %current's plugged list.  Returns %true if merge was successful,
+ * Determine whether @bio being queued on @q can be merged with the previous
+ * request on %current's plugged list.  Returns %true if merge was successful,
  * otherwise %false.

> @@ -1089,32 +1089,22 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
>  {
>  	struct blk_plug *plug;
>  	struct request *rq;
> -	struct list_head *plug_list;
>  
>  	plug = blk_mq_plug(q, bio);
> -	if (!plug)
> +	if (!plug || list_empty(&plug->mq_list))
>  		return false;
>  
Regards,
Pankaj Raghav

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] block: only check previous entry for plug merge attempt
  2021-10-14 18:34 [PATCH v3] block: only check previous entry for plug merge attempt Jens Axboe
  2021-10-15 14:18 ` Pankaj Raghav
@ 2021-10-16  4:31 ` Christoph Hellwig
  1 sibling, 0 replies; 4+ messages in thread
From: Christoph Hellwig @ 2021-10-16  4:31 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-block

Looks good module the needed comment update:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] block: only check previous entry for plug merge attempt
  2021-10-15 14:18 ` Pankaj Raghav
@ 2021-10-16 22:13   ` Jens Axboe
  0 siblings, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2021-10-16 22:13 UTC (permalink / raw)
  To: Pankaj Raghav; +Cc: linux-block, Pankaj Raghav

On 10/15/21 8:18 AM, Pankaj Raghav wrote:
> On Thu, Oct 14, 2021 at 12:34:21PM -0600, Jens Axboe wrote:
>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index f390a8753268..575080ad0617 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
> 
> Even though the code is self-documenting, the current description on top
> of this function is now outdated with this change. 
> 
> - * Determine whether @bio being queued on @q can be merged with a request
> - * on %current's plugged list.  Returns %true if merge was successful,
> + * Determine whether @bio being queued on @q can be merged with the previous
> + * request on %current's plugged list.  Returns %true if merge was successful,
>   * otherwise %false.

Fixed up, thanks.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-10-16 22:13 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-14 18:34 [PATCH v3] block: only check previous entry for plug merge attempt Jens Axboe
2021-10-15 14:18 ` Pankaj Raghav
2021-10-16 22:13   ` Jens Axboe
2021-10-16  4:31 ` Christoph Hellwig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.