From: Yu Kuai <[email protected]>
Commit 0c0be98bbe67 ("md/raid10: prevent unnecessary calls to wake_up()
in fast path") missed one place, for example, while testing with:
fio -direct=1 -rw=write/randwrite -iodepth=1 ...
Then plug and unplug will be called for each io, then wake_up() from
raid10_unplug() will cause lock contention as well.
Avoid this contention by using wake_up_barrier() instead of wake_up(),
where spin_lock is not held while waitqueue is empty.
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 <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Yu Kuai <[email protected]>
---
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;
--
2.39.2
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 <[email protected]>
>
> 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?
> 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 <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Yu Kuai <[email protected]>
> ---
> 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 <[email protected]>
Kind regards,
Paul
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Link: https://lore.kernel.org/all/[email protected]/
>> Signed-off-by: Yu Kuai <[email protected]>
>> ---
>> 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 <[email protected]>
>
>
> Kind regards,
>
> Paul
>
> .
>
On Sat, Jun 17, 2023 at 11:26 PM Yu Kuai <[email protected]> wrote:
>
> From: Yu Kuai <[email protected]>
>
> Commit 0c0be98bbe67 ("md/raid10: prevent unnecessary calls to wake_up()
> in fast path") missed one place, for example, while testing with:
>
> fio -direct=1 -rw=write/randwrite -iodepth=1 ...
>
> Then plug and unplug will be called for each io, then wake_up() from
> raid10_unplug() will cause lock contention as well.
>
> Avoid this contention by using wake_up_barrier() instead of wake_up(),
> where spin_lock is not held while waitqueue is empty.
>
> 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 <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
I think ./scripts/checkpatch.pl would recommend using "Closes" instead of
"Link" after "Reported-by". Please follow this pattern.
Thanks,
Song
> Signed-off-by: Yu Kuai <[email protected]>
> ---
> 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;
> --
> 2.39.2
>
On Mon, Jun 19, 2023 at 6:00 AM Yu Kuai <[email protected]> wrote:
>
> 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 <[email protected]>
> >>
> >> 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.
We don't need a lot of details of the test results in the commit log. But a
quick summary of the performance result can be really helpful here.
Something like: on array X, fio job Y, before the patch we got AAA, with
the patch, we got BBB.
Other than this, the patch looks good to me.
Thanks,
Song