[PATCH 0/2] blk-mq: two improvemens on slow MQ devices

[PATCH 0/2] blk-mq: two improvemens on slow MQ devices

[Ming Lei]

Hi,

The 1st patch always applies io scheduler path if 'none' isn't used,
so that sequential IO performance can be improved on slow MQ device.
Also write order for zone device can be maintained becasue zone device
requires mq-dealine to do that.

The 2nd patch applies normal plugging for MQ HDD. 


Ming Lei (2):
  blk-mq: respect io scheduler
  blk-mq: apply normal plugging for HDD

 block/blk-mq.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
Cc: Dave Chinner <dchinner@redhat.com>

-- 
2.20.1

[PATCH 1/2] blk-mq: respect io scheduler

[Ming Lei]

Now in case of real MQ, io scheduler may be bypassed, and not only this
way may hurt performance for some slow MQ device, but also break zoned
device which depends on mq-deadline for respecting the write order in
one zone.

So don't bypass io scheduler if we have one setup.

This patch can double sequential write performance basically on MQ
scsi_debug when mq-deadline is applied.

Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
Cc: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 20a49be536b5..d7aed6518e62 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2003,6 +2003,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
 		}
 
 		blk_add_rq_to_plug(plug, rq);
+	} else if (q->elevator) {
+		blk_mq_sched_insert_request(rq, false, true, true);
 	} else if (plug && !blk_queue_nomerges(q)) {
 		/*
 		 * We do limited plugging. If the bio can be merged, do that.
@@ -2026,8 +2028,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
 			blk_mq_try_issue_directly(data.hctx, same_queue_rq,
 					&cookie);
 		}
-	} else if ((q->nr_hw_queues > 1 && is_sync) || (!q->elevator &&
-			!data.hctx->dispatch_busy)) {
+	} else if ((q->nr_hw_queues > 1 && is_sync) ||
+			!data.hctx->dispatch_busy) {
 		blk_mq_try_issue_directly(data.hctx, rq, &cookie);
 	} else {
 		blk_mq_sched_insert_request(rq, false, true, true);
-- 
2.20.1

[PATCH 2/2] blk-mq: apply normal plugging for HDD

[Ming Lei]

Some HDD drive may expose multiple hw queue, such as MegraRaid, so
still apply the normal plugging for such devices because sequential IO
may benefit a lot from plug merging.

Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
Cc: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index d7aed6518e62..969dfe02fa7c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1983,10 +1983,14 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
 		/* bypass scheduler for flush rq */
 		blk_insert_flush(rq);
 		blk_mq_run_hw_queue(data.hctx, true);
-	} else if (plug && (q->nr_hw_queues == 1 || q->mq_ops->commit_rqs)) {
+	} else if (plug && (q->nr_hw_queues == 1 || q->mq_ops->commit_rqs ||
+				!blk_queue_nonrot(q))) {
 		/*
 		 * Use plugging if we have a ->commit_rqs() hook as well, as
 		 * we know the driver uses bd->last in a smart fashion.
+		 *
+		 * Use normal plugging if this disk is slow HDD, as sequential
+		 * IO may benefit a lot from plug merging.
 		 */
 		unsigned int request_count = plug->rq_count;
 		struct request *last = NULL;
-- 
2.20.1

Re: [PATCH 1/2] blk-mq: respect io scheduler

[Javier González]

[-- Attachment #1: Type: text/plain, Size: 1957 bytes --]

> On 27 Sep 2019, at 09.24, Ming Lei <ming.lei@redhat.com> wrote:
> 
> Now in case of real MQ, io scheduler may be bypassed, and not only this
> way may hurt performance for some slow MQ device, but also break zoned
> device which depends on mq-deadline for respecting the write order in
> one zone.
> 
> So don't bypass io scheduler if we have one setup.
> 
> This patch can double sequential write performance basically on MQ
> scsi_debug when mq-deadline is applied.
> 
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Damien Le Moal <damien.lemoal@wdc.com>
> Cc: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> block/blk-mq.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 20a49be536b5..d7aed6518e62 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2003,6 +2003,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
> 		}
> 
> 		blk_add_rq_to_plug(plug, rq);
> +	} else if (q->elevator) {
> +		blk_mq_sched_insert_request(rq, false, true, true);
> 	} else if (plug && !blk_queue_nomerges(q)) {
> 		/*
> 		 * We do limited plugging. If the bio can be merged, do that.
> @@ -2026,8 +2028,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
> 			blk_mq_try_issue_directly(data.hctx, same_queue_rq,
> 					&cookie);
> 		}
> -	} else if ((q->nr_hw_queues > 1 && is_sync) || (!q->elevator &&
> -			!data.hctx->dispatch_busy)) {
> +	} else if ((q->nr_hw_queues > 1 && is_sync) ||
> +			!data.hctx->dispatch_busy) {
> 		blk_mq_try_issue_directly(data.hctx, rq, &cookie);
> 	} else {
> 		blk_mq_sched_insert_request(rq, false, true, true);
> --
> 2.20.1


Looks good to me. Fixes a couple issues we have seen with zoned devices too.

Reviewed-by: Javier González <javier@javigon.com>


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] blk-mq: respect io scheduler

[Hans Holmberg]

On Fri, Sep 27, 2019 at 9:25 AM Ming Lei <ming.lei@redhat.com> wrote:
>
> Now in case of real MQ, io scheduler may be bypassed, and not only this
> way may hurt performance for some slow MQ device, but also break zoned
> device which depends on mq-deadline for respecting the write order in
> one zone.
>
> So don't bypass io scheduler if we have one setup.
>
> This patch can double sequential write performance basically on MQ
> scsi_debug when mq-deadline is applied.
>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Damien Le Moal <damien.lemoal@wdc.com>
> Cc: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 20a49be536b5..d7aed6518e62 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2003,6 +2003,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>                 }
>
>                 blk_add_rq_to_plug(plug, rq);
> +       } else if (q->elevator) {
> +               blk_mq_sched_insert_request(rq, false, true, true);
>         } else if (plug && !blk_queue_nomerges(q)) {
>                 /*
>                  * We do limited plugging. If the bio can be merged, do that.
> @@ -2026,8 +2028,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>                         blk_mq_try_issue_directly(data.hctx, same_queue_rq,
>                                         &cookie);
>                 }
> -       } else if ((q->nr_hw_queues > 1 && is_sync) || (!q->elevator &&
> -                       !data.hctx->dispatch_busy)) {
> +       } else if ((q->nr_hw_queues > 1 && is_sync) ||
> +                       !data.hctx->dispatch_busy) {
>                 blk_mq_try_issue_directly(data.hctx, rq, &cookie);
>         } else {
>                 blk_mq_sched_insert_request(rq, false, true, true);
> --
> 2.20.1
>

I've verified that the issue I saw with zone locking being skipped due
to scheduler bypass is resolved with this patch.

Thanks,
Hans

Re: [PATCH 1/2] blk-mq: respect io scheduler

[Damien Le Moal]

On 2019/09/27 0:25, Ming Lei wrote:
> Now in case of real MQ, io scheduler may be bypassed, and not only this
> way may hurt performance for some slow MQ device, but also break zoned
> device which depends on mq-deadline for respecting the write order in
> one zone.
> 
> So don't bypass io scheduler if we have one setup.
> 
> This patch can double sequential write performance basically on MQ
> scsi_debug when mq-deadline is applied.
> 
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Damien Le Moal <damien.lemoal@wdc.com>
> Cc: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 20a49be536b5..d7aed6518e62 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2003,6 +2003,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>  		}
>  
>  		blk_add_rq_to_plug(plug, rq);
> +	} else if (q->elevator) {
> +		blk_mq_sched_insert_request(rq, false, true, true);>  	} else if (plug && !blk_queue_nomerges(q)) {
>  		/*
>  		 * We do limited plugging. If the bio can be merged, do that.
> @@ -2026,8 +2028,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>  			blk_mq_try_issue_directly(data.hctx, same_queue_rq,
>  					&cookie);
>  		}
> -	} else if ((q->nr_hw_queues > 1 && is_sync) || (!q->elevator &&
> -			!data.hctx->dispatch_busy)) {
> +	} else if ((q->nr_hw_queues > 1 && is_sync) ||
> +			!data.hctx->dispatch_busy) {
>  		blk_mq_try_issue_directly(data.hctx, rq, &cookie);
>  	} else {
>  		blk_mq_sched_insert_request(rq, false, true, true);
> 

I think this patch should have a Cc: stable@vger.kernel.org
This fixes a problem existing since we added deadline zone write-locking with
commit 5700f69178e9 ("mq-deadline: Introduce zone locking support").

Otherwise:

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] blk-mq: apply normal plugging for HDD

[Damien Le Moal]

On 2019/09/27 0:25, Ming Lei wrote:
> Some HDD drive may expose multiple hw queue, such as MegraRaid, so
> still apply the normal plugging for such devices because sequential IO
> may benefit a lot from plug merging.
> 
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Damien Le Moal <damien.lemoal@wdc.com>
> Cc: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index d7aed6518e62..969dfe02fa7c 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1983,10 +1983,14 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>  		/* bypass scheduler for flush rq */
>  		blk_insert_flush(rq);
>  		blk_mq_run_hw_queue(data.hctx, true);
> -	} else if (plug && (q->nr_hw_queues == 1 || q->mq_ops->commit_rqs)) {
> +	} else if (plug && (q->nr_hw_queues == 1 || q->mq_ops->commit_rqs ||
> +				!blk_queue_nonrot(q))) {
>  		/*
>  		 * Use plugging if we have a ->commit_rqs() hook as well, as
>  		 * we know the driver uses bd->last in a smart fashion.
> +		 *
> +		 * Use normal plugging if this disk is slow HDD, as sequential
> +		 * IO may benefit a lot from plug merging.
>  		 */
>  		unsigned int request_count = plug->rq_count;
>  		struct request *last = NULL;
> 

Cc stable needed ?

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/2] blk-mq: two improvemens on slow MQ devices

[Jens Axboe]

On 9/27/19 9:24 AM, Ming Lei wrote:
> Hi,
> 
> The 1st patch always applies io scheduler path if 'none' isn't used,
> so that sequential IO performance can be improved on slow MQ device.
> Also write order for zone device can be maintained becasue zone device
> requires mq-dealine to do that.
> 
> The 2nd patch applies normal plugging for MQ HDD.

Both look clean and fine to me. I will wait with queueing them up
until I've done some testing.

-- 
Jens Axboe

Re: [PATCH 1/2] blk-mq: respect io scheduler

[Jens Axboe]

On 9/27/19 7:25 PM, Damien Le Moal wrote:
> On 2019/09/27 0:25, Ming Lei wrote:
>> Now in case of real MQ, io scheduler may be bypassed, and not only this
>> way may hurt performance for some slow MQ device, but also break zoned
>> device which depends on mq-deadline for respecting the write order in
>> one zone.
>>
>> So don't bypass io scheduler if we have one setup.
>>
>> This patch can double sequential write performance basically on MQ
>> scsi_debug when mq-deadline is applied.
>>
>> Cc: Bart Van Assche <bvanassche@acm.org>
>> Cc: Hannes Reinecke <hare@suse.com>
>> Cc: Damien Le Moal <damien.lemoal@wdc.com>
>> Cc: Dave Chinner <dchinner@redhat.com>
>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>> ---
>>   block/blk-mq.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 20a49be536b5..d7aed6518e62 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -2003,6 +2003,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>>   		}
>>   
>>   		blk_add_rq_to_plug(plug, rq);
>> +	} else if (q->elevator) {
>> +		blk_mq_sched_insert_request(rq, false, true, true);>  	} else if (plug && !blk_queue_nomerges(q)) {
>>   		/*
>>   		 * We do limited plugging. If the bio can be merged, do that.
>> @@ -2026,8 +2028,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>>   			blk_mq_try_issue_directly(data.hctx, same_queue_rq,
>>   					&cookie);
>>   		}
>> -	} else if ((q->nr_hw_queues > 1 && is_sync) || (!q->elevator &&
>> -			!data.hctx->dispatch_busy)) {
>> +	} else if ((q->nr_hw_queues > 1 && is_sync) ||
>> +			!data.hctx->dispatch_busy) {
>>   		blk_mq_try_issue_directly(data.hctx, rq, &cookie);
>>   	} else {
>>   		blk_mq_sched_insert_request(rq, false, true, true);
>>
> 
> I think this patch should have a Cc: stable@vger.kernel.org
> This fixes a problem existing since we added deadline zone write-locking with
> commit 5700f69178e9 ("mq-deadline: Introduce zone locking support").

I'd rather not mark it for stable until it's been in the kernel for some
weeks at least, since we are potentially dealing with behavioral change
for everyone. We've been burnt by stuff like this in the past.

That said, this patch could be a candidate. Let's revisit in a few weeks.

-- 
Jens Axboe

Re: [PATCH 1/2] blk-mq: respect io scheduler

[Damien Le Moal]

On 2019/09/27 10:32, Jens Axboe wrote:
> On 9/27/19 7:25 PM, Damien Le Moal wrote:
>> On 2019/09/27 0:25, Ming Lei wrote:
>>> Now in case of real MQ, io scheduler may be bypassed, and not only this
>>> way may hurt performance for some slow MQ device, but also break zoned
>>> device which depends on mq-deadline for respecting the write order in
>>> one zone.
>>>
>>> So don't bypass io scheduler if we have one setup.
>>>
>>> This patch can double sequential write performance basically on MQ
>>> scsi_debug when mq-deadline is applied.
>>>
>>> Cc: Bart Van Assche <bvanassche@acm.org>
>>> Cc: Hannes Reinecke <hare@suse.com>
>>> Cc: Damien Le Moal <damien.lemoal@wdc.com>
>>> Cc: Dave Chinner <dchinner@redhat.com>
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> ---
>>>   block/blk-mq.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index 20a49be536b5..d7aed6518e62 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -2003,6 +2003,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>>>   		}
>>>   
>>>   		blk_add_rq_to_plug(plug, rq);
>>> +	} else if (q->elevator) {
>>> +		blk_mq_sched_insert_request(rq, false, true, true);>  	} else if (plug && !blk_queue_nomerges(q)) {
>>>   		/*
>>>   		 * We do limited plugging. If the bio can be merged, do that.
>>> @@ -2026,8 +2028,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>>>   			blk_mq_try_issue_directly(data.hctx, same_queue_rq,
>>>   					&cookie);
>>>   		}
>>> -	} else if ((q->nr_hw_queues > 1 && is_sync) || (!q->elevator &&
>>> -			!data.hctx->dispatch_busy)) {
>>> +	} else if ((q->nr_hw_queues > 1 && is_sync) ||
>>> +			!data.hctx->dispatch_busy) {
>>>   		blk_mq_try_issue_directly(data.hctx, rq, &cookie);
>>>   	} else {
>>>   		blk_mq_sched_insert_request(rq, false, true, true);
>>>
>>
>> I think this patch should have a Cc: stable@vger.kernel.org
>> This fixes a problem existing since we added deadline zone write-locking with
>> commit 5700f69178e9 ("mq-deadline: Introduce zone locking support").
> 
> I'd rather not mark it for stable until it's been in the kernel for some
> weeks at least, since we are potentially dealing with behavioral change
> for everyone. We've been burnt by stuff like this in the past.
> 
> That said, this patch could be a candidate. Let's revisit in a few weeks.
> 

OK. Thanks !

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] blk-mq: respect io scheduler

[André Almeida]

Hello Ming Lei,

On 9/27/19 4:24 AM, Ming Lei wrote:
> Now in case of real MQ, io scheduler may be bypassed, and not only this
> way may hurt performance for some slow MQ device, but also break zoned
> device which depends on mq-deadline for respecting the write order in
> one zone.
> 
> So don't bypass io scheduler if we have one setup.
> 
> This patch can double sequential write performance basically on MQ
> scsi_debug when mq-deadline is applied.
> 
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Damien Le Moal <damien.lemoal@wdc.com>
> Cc: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 20a49be536b5..d7aed6518e62 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2003,6 +2003,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>  		}
>  
>  		blk_add_rq_to_plug(plug, rq);
> +	} else if (q->elevator) {
> +		blk_mq_sched_insert_request(rq, false, true, true);
>  	} else if (plug && !blk_queue_nomerges(q)) {
>  		/*
>  		 * We do limited plugging. If the bio can be merged, do that.
> @@ -2026,8 +2028,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>  			blk_mq_try_issue_directly(data.hctx, same_queue_rq,
>  					&cookie);
>  		}
> -	} else if ((q->nr_hw_queues > 1 && is_sync) || (!q->elevator &&
> -			!data.hctx->dispatch_busy)) {
> +	} else if ((q->nr_hw_queues > 1 && is_sync) ||
> +			!data.hctx->dispatch_busy) {
>  		blk_mq_try_issue_directly(data.hctx, rq, &cookie);
>  	} else {
>  		blk_mq_sched_insert_request(rq, false, true, true);
> 

This patch introduces identical branches, both "else if (q->elevator)"
and "else" do the same thing. Wouldn't be enough to just remove the
condition "!q->elevator" without adding a new if?

Thanks,
	André