Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH] raid10: avoid spin_lock from fastpath from
 raid10_unplug()
To:     Paul Menzel <pmenzel@molgen.mpg.de>,
        Yu Kuai <yukuai1@huaweicloud.com>
Cc:     aligrudi@gmail.com, song@kernel.org, linux-raid@vger.kernel.org,
        linux-kernel@vger.kernel.org, yi.zhang@huawei.com,
        yangerkun@huawei.com, "yukuai (C)" <yukuai3@huawei.com>
References: <20230618142520.14662-1-yukuai1@huaweicloud.com>
 <ac4c9d48-b8d2-b847-2721-11179fd922de@molgen.mpg.de>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <4c729ee3-ad63-1928-0113-19b576b09b24@huaweicloud.com>
Date:   Mon, 19 Jun 2023 21:00:00 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <ac4c9d48-b8d2-b847-2721-11179fd922de@molgen.mpg.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

在 2023/06/19 18:26, Paul Menzel 写道:
> Dear Yu,
> 
> 
> Thank you for your patch. Some minor nits from my side, you can also 
> ignore.
> 
> Am 18.06.23 um 16:25 schrieb Yu Kuai:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Commit 0c0be98bbe67 ("md/raid10: prevent unnecessary calls to wake_up()
>> in fast path") missed one place, for example, while testing with:
> 
> … one place. For example, with
> 
>>
>> fio -direct=1 -rw=write/randwrite -iodepth=1 ...
>>
>> Then plug and unplug will be called for each io, then wake_up() from
> 
> Maybe:
> 
>      fio -direct=1 -rw=write/randwrite -iodepth=1 ...
> 
> plug und unplug are called for each io, then …
> 
>> raid10_unplug() will cause lock contention as well.
> 
> Maybe paste the perf command and output?

Related test and test result and perf result can be found in the below
Link.

By the way, I'll wait for more review to send a v2 to optmize commit
message.

Thanks,
Kuai

> 
>> Avoid this contention by using wake_up_barrier() instead of wake_up(),
>> where spin_lock is not held while waitqueue is empty.
> 
> It’d be great if you added also the test results to the commit message.
> 
>> By the way, in this scenario, each blk_plug_cb() will be allocated and
>> freed for each io, this seems need to be optimized as well.
>>
>> Reported-and-tested-by: Ali Gholami Rudi <aligrudi@gmail.com>
>> Link: https://lore.kernel.org/all/20231606122233@laper.mirepesht/
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   drivers/md/raid10.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index d0de8c9fb3cf..fbaaa5e05edc 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -1118,7 +1118,7 @@ static void raid10_unplug(struct blk_plug_cb 
>> *cb, bool from_schedule)
>>           spin_lock_irq(&conf->device_lock);
>>           bio_list_merge(&conf->pending_bio_list, &plug->pending);
>>           spin_unlock_irq(&conf->device_lock);
>> -        wake_up(&conf->wait_barrier);
>> +        wake_up_barrier(conf);
>>           md_wakeup_thread(mddev->thread);
>>           kfree(plug);
>>           return;
>> @@ -1127,7 +1127,7 @@ static void raid10_unplug(struct blk_plug_cb 
>> *cb, bool from_schedule)
>>       /* we aren't scheduling, so we can do the write-out directly. */
>>       bio = bio_list_get(&plug->pending);
>>       raid1_prepare_flush_writes(mddev->bitmap);
>> -    wake_up(&conf->wait_barrier);
>> +    wake_up_barrier(conf);
>>       while (bio) { /* submit pending writes */
>>           struct bio *next = bio->bi_next;
> 
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> 
> 
> Kind regards,
> 
> Paul
> 
> .
>