linux-kernel - Re: [PATCH] bfq: do try insert merge before bfq_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <2EA88BDB-9C3B-4EF6-BF8B-3CCF7DB304C1@linaro.org>
Date:   Sun, 16 Oct 2022 12:01:39 +0200
From:   Paolo Valente <paolo.valente@...aro.org>
To:     Yuwei Guan <ssawgyw@...il.com>
Cc:     Jan Kara <jack@...e.cz>, Jens Axboe <axboe@...nel.dk>,
        linux-block <linux-block@...r.kernel.org>,
        linux-kernel@...r.kernel.org, Yuwei.Guan@...krlife.com
Subject: Re: [PATCH] bfq: do try insert merge before bfq_init_rq() call



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