Re: [PATCH] bfq: do try insert merge before bfq_init_rq() call

From: Paolo Valente
Date: Sun Oct 16 2022 - 06:01:54 EST




> Il giorno 15 ott 2022, alle ore 05:32, Yuwei Guan <ssawgyw@xxxxxxxxx> 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@xxxxxxxxxxxxx>
>> 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