diff mbox series

bfq: do try insert merge before bfq_init_rq() call

Message ID 20221013135321.174-1-Yuwei.Guan@zeekrlife.com
State New
Headers
Series bfq: do try insert merge before bfq_init_rq() call |

Commit Message

Yuwei Guan Oct. 13, 2022, 1:53 p.m. UTC 
  It's useless to do bfq_init_rq(rq), if the rq can do merge first.

In the patch 5f550ede5edf8, it moved to bfq_init_rq() before
blk_mq_sched_try_insert_merge(), but it's pointless,
as the fifo_time of next is not set yet,
and !list_empty(&next->queuelist) is 0, so it does not
need to reposition rq's fifo_time.

And for the "hash lookup, try again" situation, as follow,
bfq_requests_merged() call can work normally.

blk_mq_sched_try_insert_merge
  elv_attempt_insert_merge
    elv_rqhash_find

Signed-off-by: Yuwei Guan <Yuwei.Guan@zeekrlife.com>
---
 block/bfq-iosched.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jan Kara Oct. 14, 2022, 2:50 p.m. UTC | #1 
On Thu 13-10-22 21:53:21, Yuwei Guan wrote:
> It's useless to do bfq_init_rq(rq), if the rq can do merge first.
> 
> In the patch 5f550ede5edf8, it moved to bfq_init_rq() before
> blk_mq_sched_try_insert_merge(), but it's pointless,
> as the fifo_time of next is not set yet,
> and !list_empty(&next->queuelist) is 0, so it does not
> need to reposition rq's fifo_time.
> 
> And for the "hash lookup, try again" situation, as follow,
> bfq_requests_merged() call can work normally.
> 
> blk_mq_sched_try_insert_merge
>   elv_attempt_insert_merge
>     elv_rqhash_find
> 
> Signed-off-by: Yuwei Guan <Yuwei.Guan@zeekrlife.com>

OK, after some thinking I agree. How much testing has this patch got?
Because I'd like to verify we didn't overlook something.

							Honza

> ---
>  block/bfq-iosched.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 7ea427817f7f..9845370a701c 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -6147,7 +6147,7 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>  		bfqg_stats_update_legacy_io(q, rq);
>  #endif
>  	spin_lock_irq(&bfqd->lock);
> -	bfqq = bfq_init_rq(rq);
> +
>  	if (blk_mq_sched_try_insert_merge(q, rq, &free)) {
>  		spin_unlock_irq(&bfqd->lock);
>  		blk_mq_free_requests(&free);
> @@ -6156,6 +6156,7 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>  
>  	trace_block_rq_insert(rq);
>  
> +	bfqq = bfq_init_rq(rq);
>  	if (!bfqq || at_head) {
>  		if (at_head)
>  			list_add(&rq->queuelist, &bfqd->dispatch);
> -- 
> 2.34.1
>
Yuwei Guan Oct. 15, 2022, 3:32 a.m. UTC | #2 
On 2022/10/14 22:50, Jan Kara wrote:
> On Thu 13-10-22 21:53:21, Yuwei Guan wrote:
>> It's useless to do bfq_init_rq(rq), if the rq can do merge first.
>>
>> In the patch 5f550ede5edf8, it moved to bfq_init_rq() before
>> blk_mq_sched_try_insert_merge(), but it's pointless,
>> as the fifo_time of next is not set yet,
>> and !list_empty(&next->queuelist) is 0, so it does not
>> need to reposition rq's fifo_time.
>>
>> And for the "hash lookup, try again" situation, as follow,
>> bfq_requests_merged() call can work normally.
>>
>> blk_mq_sched_try_insert_merge
>>    elv_attempt_insert_merge
>>      elv_rqhash_find
>>
>> Signed-off-by: Yuwei Guan <Yuwei.Guan@zeekrlife.com>
> OK, after some thinking I agree. How much testing has this patch got?
> Because I'd like to verify we didn't overlook something.
>
> 							Honza
Thanks for reviewing.
I tested it with fio seq read case like bellow,
then check blk trace and bfq log.

[global]
name=fio-seq-reads
filename=fio-seq-reads
rw=read
bs=16K
direct=1
numjobs=4

[file1]
size=32m
ioengine=psync

What kinds of test cases you perfer to do, I will deal with them,
or we verify this patch together, if you have free time. :)
>> ---
>>   block/bfq-iosched.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>> index 7ea427817f7f..9845370a701c 100644
>> --- a/block/bfq-iosched.c
>> +++ b/block/bfq-iosched.c
>> @@ -6147,7 +6147,7 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>>   		bfqg_stats_update_legacy_io(q, rq);
>>   #endif
>>   	spin_lock_irq(&bfqd->lock);
>> -	bfqq = bfq_init_rq(rq);
>> +
>>   	if (blk_mq_sched_try_insert_merge(q, rq, &free)) {
>>   		spin_unlock_irq(&bfqd->lock);
>>   		blk_mq_free_requests(&free);
>> @@ -6156,6 +6156,7 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>>   
>>   	trace_block_rq_insert(rq);
>>   
>> +	bfqq = bfq_init_rq(rq);
>>   	if (!bfqq || at_head) {
>>   		if (at_head)
>>   			list_add(&rq->queuelist, &bfqd->dispatch);
>> -- 
>> 2.34.1
>>
Paolo Valente Oct. 16, 2022, 10:01 a.m. UTC | #3 
> Il giorno 15 ott 2022, alle ore 05:32, Yuwei Guan <ssawgyw@gmail.com> ha scritto:
> 
> 
> On 2022/10/14 22:50, Jan Kara wrote:
>> On Thu 13-10-22 21:53:21, Yuwei Guan wrote:
>>> It's useless to do bfq_init_rq(rq), if the rq can do merge first.
>>> 
>>> In the patch 5f550ede5edf8, it moved to bfq_init_rq() before
>>> blk_mq_sched_try_insert_merge(), but it's pointless,
>>> as the fifo_time of next is not set yet,
>>> and !list_empty(&next->queuelist) is 0, so it does not
>>> need to reposition rq's fifo_time.
>>> 
>>> And for the "hash lookup, try again" situation, as follow,
>>> bfq_requests_merged() call can work normally.
>>> 
>>> blk_mq_sched_try_insert_merge
>>>   elv_attempt_insert_merge
>>>     elv_rqhash_find
>>> 
>>> Signed-off-by: Yuwei Guan <Yuwei.Guan@zeekrlife.com>
>> OK, after some thinking I agree. How much testing has this patch got?
>> Because I'd like to verify we didn't overlook something.
>> 
>> 							Honza
> Thanks for reviewing.
> I tested it with fio seq read case like bellow,
> then check blk trace and bfq log.
> 
> [global]
> name=fio-seq-reads
> filename=fio-seq-reads
> rw=read
> bs=16K
> direct=1
> numjobs=4
> 
> [file1]
> size=32m
> ioengine=psync
> 
> What kinds of test cases you perfer to do, I will deal with them,
> or we verify this patch together, if you have free time. :)


Hi guys,
thank you Yuwei for proposing this.  Yet, I'm a little doubtful, for
the case where blk_mq_sched_try_insert_merge returns true, and then to
bfq_init_rq() does not get called.  In this case, all the code for
handling bursts, splits, ioprio changes and the other stuff in to
bfq_init_rq() is not executed.  This worries me a little bit.  Can you
show me why not executing these operations is fine?

Thanks,
Paolo

>>> ---
>>>  block/bfq-iosched.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>>> index 7ea427817f7f..9845370a701c 100644
>>> --- a/block/bfq-iosched.c
>>> +++ b/block/bfq-iosched.c
>>> @@ -6147,7 +6147,7 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>>>  		bfqg_stats_update_legacy_io(q, rq);
>>>  #endif
>>>  	spin_lock_irq(&bfqd->lock);
>>> -	bfqq = bfq_init_rq(rq);
>>> +
>>>  	if (blk_mq_sched_try_insert_merge(q, rq, &free)) {
>>>  		spin_unlock_irq(&bfqd->lock);
>>>  		blk_mq_free_requests(&free);
>>> @@ -6156,6 +6156,7 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>>>    	trace_block_rq_insert(rq);
>>>  +	bfqq = bfq_init_rq(rq);
>>>  	if (!bfqq || at_head) {
>>>  		if (at_head)
>>>  			list_add(&rq->queuelist, &bfqd->dispatch);
>>> -- 
>>> 2.34.1
diff mbox series

Patch

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 7ea427817f7f..9845370a701c 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -6147,7 +6147,7 @@  static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
 		bfqg_stats_update_legacy_io(q, rq);
 #endif
 	spin_lock_irq(&bfqd->lock);
-	bfqq = bfq_init_rq(rq);
+
 	if (blk_mq_sched_try_insert_merge(q, rq, &free)) {
 		spin_unlock_irq(&bfqd->lock);
 		blk_mq_free_requests(&free);
@@ -6156,6 +6156,7 @@  static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
 
 	trace_block_rq_insert(rq);
 
+	bfqq = bfq_init_rq(rq);
 	if (!bfqq || at_head) {
 		if (at_head)
 			list_add(&rq->queuelist, &bfqd->dispatch);