[PATCH V2 1/2] block: immediately dispatch big size request

[PATCH V2 1/2] block: immediately dispatch big size request

[Shaohua Li]

Currently block plug holds up to 16 non-mergeable requests. This makes
sense if the request size is small, eg, reduce lock contention. But if
request size is big enough, we don't need to worry about lock
contention. Holding such request makes no sense and it lows the disk
utilization.

In practice, this improves 10% throughput for my raid5 sequential write
workload.

The size (128k) is arbitrary right now, but it makes sure lock
contention is small. This probably could be more intelligent, eg, check
average request size holded. Since this is mainly for sequential IO,
probably not worthy.

V2: check the last request instead of the first request, so as long as
there is one big size request we flush the plug.

Signed-off-by: Shaohua Li <shli@fb.com>
---
 block/blk-core.c       | 4 +++-
 include/linux/blkdev.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 14d7c07..6be6378 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1763,7 +1763,9 @@ static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
 		if (!request_count)
 			trace_block_plug(q);
 		else {
-			if (request_count >= BLK_MAX_REQUEST_COUNT) {
+			struct request *last = list_entry_rq(plug->list.prev);
+			if (request_count >= BLK_MAX_REQUEST_COUNT ||
+			    blk_rq_bytes(last) >= BLK_PLUG_FLUSH_SIZE) {
 				blk_flush_plug_list(plug, false);
 				trace_block_plug(q);
 			}
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c47c358..72fa505 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1078,6 +1078,7 @@ struct blk_plug {
 	struct list_head cb_list; /* md requires an unplug callback */
 };
 #define BLK_MAX_REQUEST_COUNT 16
+#define BLK_PLUG_FLUSH_SIZE (128 * 1024)
 
 struct blk_plug_cb;
 typedef void (*blk_plug_cb_fn)(struct blk_plug_cb *, bool);
-- 
2.9.3

[PATCH V2 2/2] blk-mq: immediately dispatch big size request

[Shaohua Li]

This is corresponding part for blk-mq. Disk with multiple hardware
queues doesn't need this as we only hold 1 request at most.

Signed-off-by: Shaohua Li <shli@fb.com>
---
 block/blk-mq.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index f3d27a6..a72538a 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1401,13 +1401,18 @@ static blk_qc_t blk_sq_make_request(struct request_queue *q, struct bio *bio)
 	 */
 	plug = current->plug;
 	if (plug) {
+		struct request *last = NULL;
+
 		blk_mq_bio_to_request(rq, bio);
 		if (!request_count)
 			trace_block_plug(q);
+		else
+			last = list_entry_rq(plug->mq_list.prev);
 
 		blk_mq_put_ctx(data.ctx);
 
-		if (request_count >= BLK_MAX_REQUEST_COUNT) {
+		if (request_count >= BLK_MAX_REQUEST_COUNT || (last &&
+		    blk_rq_bytes(last) >= BLK_PLUG_FLUSH_SIZE)) {
 			blk_flush_plug_list(plug, false);
 			trace_block_plug(q);
 		}
-- 
2.9.3

Re: [PATCH V2 2/2] blk-mq: immediately dispatch big size request

[Christoph Hellwig]

On Thu, Nov 03, 2016 at 05:03:54PM -0700, Shaohua Li wrote:
> This is corresponding part for blk-mq. Disk with multiple hardware
> queues doesn't need this as we only hold 1 request at most.

Any reason you only do this for the SQ and not the MQ case?

Re: [PATCH V2 2/2] blk-mq: immediately dispatch big size request

[Shaohua Li]

On Thu, Nov 03, 2016 at 05:09:54PM -0700, Christoph Hellwig wrote:
> On Thu, Nov 03, 2016 at 05:03:54PM -0700, Shaohua Li wrote:
> > This is corresponding part for blk-mq. Disk with multiple hardware
> > queues doesn't need this as we only hold 1 request at most.
> 
> Any reason you only do this for the SQ and not the MQ case?

Right above:
Disk with multiple hardware queues doesn't need this as we only hold 1
request at most.

Thanks,
Shaohua

Re: [PATCH V2 2/2] blk-mq: immediately dispatch big size request

[Jens Axboe]

On 11/03/2016 06:13 PM, Shaohua Li wrote:
> On Thu, Nov 03, 2016 at 05:09:54PM -0700, Christoph Hellwig wrote:
>> On Thu, Nov 03, 2016 at 05:03:54PM -0700, Shaohua Li wrote:
>>> This is corresponding part for blk-mq. Disk with multiple hardware
>>> queues doesn't need this as we only hold 1 request at most.
>>
>> Any reason you only do this for the SQ and not the MQ case?
>
> Right above:
> Disk with multiple hardware queues doesn't need this as we only hold 1
> request at most.

I've applied 1-2 for 4.10, but we probably should look into unifying
those parts of sq and mq in general. For instance, it doesn't seem to
make a lot of sense why we'd depth limit sq and not mq.

-- 
Jens Axboe

Re: [PATCH V2 2/2] blk-mq: immediately dispatch big size request

[Christoph Hellwig]

On Thu, Nov 03, 2016 at 10:00:58PM -0600, Jens Axboe wrote:
> I've applied 1-2 for 4.10, but we probably should look into unifying
> those parts of sq and mq in general. For instance, it doesn't seem to
> make a lot of sense why we'd depth limit sq and not mq.

I've spent some time looking the the make_request_fn and to be honest
I think that whole sq vs mq split is pointless.  They are about 70-80%
the same anyway, and I think everyone would be served much better
by merging them.

Re: [PATCH V2 2/2] blk-mq: immediately dispatch big size request

[Jens Axboe]

On 11/04/2016 08:46 AM, Christoph Hellwig wrote:
> On Thu, Nov 03, 2016 at 10:00:58PM -0600, Jens Axboe wrote:
>> I've applied 1-2 for 4.10, but we probably should look into unifying
>> those parts of sq and mq in general. For instance, it doesn't seem to
>> make a lot of sense why we'd depth limit sq and not mq.
>
> I've spent some time looking the the make_request_fn and to be honest
> I think that whole sq vs mq split is pointless.  They are about 70-80%
> the same anyway, and I think everyone would be served much better
> by merging them.

Yeah, that was my point, at least if we can do it without having too
many extra conditionals. Or at least split some of it into helpers.

-- 
Jens Axboe