linux-kernel - Re: [PATCH v2 1/4] md/raid10: cleanup wait

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: <6a1060e1-990c-b068-b515-b43c62595a8b@molgen.mpg.de>
Date:   Thu, 15 Sep 2022 11:12:29 +0200
From:   Paul Menzel <pmenzel@...gen.mpg.de>
To:     Yu Kuai <yukuai1@...weicloud.com>
Cc:     Logan Gunthorpe <logang@...tatee.com>, song@...nel.org,
        guoqing.jiang@...ux.dev, linux-raid@...r.kernel.org,
        linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
        "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH v2 1/4] md/raid10: cleanup wait_barrier()

Dear Yu,


Am 15.09.22 um 09:21 schrieb Yu Kuai:

> 在 2022/09/15 0:16, Logan Gunthorpe 写道:

>> On 2022-09-13 19:49, Yu Kuai wrote:
>>> From: Yu Kuai <yukuai3@...wei.com>
>>>
>>> Currently the nasty condition is wait_barrier() is hard to read. This
>>> patch factor out the condition into a function.
>>>
>>> There are no functional changes.
>>>
>>> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
>>> ---
>>>   drivers/md/raid10.c | 56 ++++++++++++++++++++++++++-------------------
>>>   1 file changed, 32 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>>> index 64d6e4cd8a3a..56458a53043d 100644
>>> --- a/drivers/md/raid10.c
>>> +++ b/drivers/md/raid10.c
>>> @@ -957,44 +957,52 @@ static void lower_barrier(struct r10conf *conf)
>>>       wake_up(&conf->wait_barrier);
>>>   }
>>> +static bool stop_waiting_barrier(struct r10conf *conf)
>>> +{
>>> +    /* barrier is dropped */
>>> +    if (!conf->barrier)
>>> +        return true;
>>> +
>>> +    /*
>>> +     * If there are already pending requests (preventing the barrier 
>>> from
>>> +     * rising completely), and the pre-process bio queue isn't 
>>> empty, then
>>> +     * don't wait, as we need to empty that queue to get the nr_pending
>>> +     * count down.
>>> +     */
>>> +    if (atomic_read(&conf->nr_pending)) {
>>> +        struct bio_list *bio_list = current->bio_list;
>>
>> I'd probably just put the bio_list declaration at the top of this
>> function, then the nested if statements are not necessary. The compiler
>> should be able to optimize the access just fine.
>>
>>>       if (conf->barrier) {
>>> -        struct bio_list *bio_list = current->bio_list;
>>> -        conf->nr_waiting++;
>>> -        /* Wait for the barrier to drop.
>>> -         * However if there are already pending
>>> -         * requests (preventing the barrier from
>>> -         * rising completely), and the
>>> -         * pre-process bio queue isn't empty,
>>> -         * then don't wait, as we need to empty
>>> -         * that queue to get the nr_pending
>>> -         * count down.
>>> -         */
>>>           /* Return false when nowait flag is set */
>>>           if (nowait) {
>>>               ret = false;
>>>           } else {
>>> +            conf->nr_waiting++;
>>
>> Technically speaking, I think moving nr_waiting counts as a functional
>> change. As best as I can see it is correct, but it should probably be at
>> least mentioned in the commit message, or maybe done as a separate
>> commit with it's own justification. That way if it causes problems down
>> the road, a bisect will make the issue clearer.
> 
> Thanks for your advice, I just think increase and decrease nr_waiting in
> the case 'nowait' is pointless, and I move it incidentally.
> 
> I'll post a separate clean up patch to do that.
> 
> Paul, can I still add your Acked-by for this patch?

Yes, sure.


Kind regards,

Paul