From: Yu Kuai <[email protected]>
bio can be added to plug infinitely, and following writeback test can
trigger huge amount of plugged bio:
Test script:
modprobe brd rd_nr=4 rd_size=10485760
mdadm -CR /dev/md0 -l10 -n4 /dev/ram[0123] --assume-clean --bitmap=internal
echo 0 > /proc/sys/vm/dirty_background_ratio
fio -filename=/dev/md0 -ioengine=libaio -rw=write -bs=4k -numjobs=1 -iodepth=128 -name=test
Test result:
Monitor /sys/block/md0/inflight will found that inflight keep increasing
until fio finish writing, after running for about 2 minutes:
[root@fedora ~]# cat /sys/block/md0/inflight
0 4474191
Fix the problem by limiting the number of plugged bio based on the number
of copies for original bio.
Signed-off-by: Yu Kuai <[email protected]>
---
drivers/md/raid1-10.c | 9 ++++++++-
drivers/md/raid1.c | 2 +-
drivers/md/raid10.c | 2 +-
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
index 17e55c1fd5a1..bb1e23b66c45 100644
--- a/drivers/md/raid1-10.c
+++ b/drivers/md/raid1-10.c
@@ -21,6 +21,7 @@
#define IO_MADE_GOOD ((struct bio *)2)
#define BIO_SPECIAL(bio) ((unsigned long)bio <= 2)
+#define MAX_PLUG_BIO 32
/* for managing resync I/O pages */
struct resync_pages {
@@ -31,6 +32,7 @@ struct resync_pages {
struct raid1_plug_cb {
struct blk_plug_cb cb;
struct bio_list pending;
+ unsigned int count;
};
static void rbio_pool_free(void *rbio, void *data)
@@ -128,7 +130,7 @@ static inline void raid1_submit_write(struct bio *bio)
}
static inline bool raid1_add_bio_to_plug(struct mddev *mddev, struct bio *bio,
- blk_plug_cb_fn unplug)
+ blk_plug_cb_fn unplug, int copies)
{
struct raid1_plug_cb *plug = NULL;
struct blk_plug_cb *cb;
@@ -148,6 +150,11 @@ static inline bool raid1_add_bio_to_plug(struct mddev *mddev, struct bio *bio,
plug = container_of(cb, struct raid1_plug_cb, cb);
bio_list_add(&plug->pending, bio);
+ if (++plug->count / MAX_PLUG_BIO >= copies) {
+ list_del(&cb->list);
+ cb->callback(cb, false);
+ }
+
return true;
}
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 006620fed595..dc89a1c4b1f1 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1562,7 +1562,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
r1_bio->sector);
/* flush_pending_writes() needs access to the rdev so...*/
mbio->bi_bdev = (void *)rdev;
- if (!raid1_add_bio_to_plug(mddev, mbio, raid1_unplug)) {
+ if (!raid1_add_bio_to_plug(mddev, mbio, raid1_unplug, disks)) {
spin_lock_irqsave(&conf->device_lock, flags);
bio_list_add(&conf->pending_bio_list, mbio);
spin_unlock_irqrestore(&conf->device_lock, flags);
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index fb22cfe94d32..9237dbeb07ba 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1306,7 +1306,7 @@ static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
atomic_inc(&r10_bio->remaining);
- if (!raid1_add_bio_to_plug(mddev, mbio, raid10_unplug)) {
+ if (!raid1_add_bio_to_plug(mddev, mbio, raid10_unplug, conf->copies)) {
spin_lock_irqsave(&conf->device_lock, flags);
bio_list_add(&conf->pending_bio_list, mbio);
spin_unlock_irqrestore(&conf->device_lock, flags);
--
2.39.2
On Mon, May 29, 2023 at 9:14 PM Yu Kuai <[email protected]> wrote:
>
> From: Yu Kuai <[email protected]>
>
> bio can be added to plug infinitely, and following writeback test can
> trigger huge amount of plugged bio:
>
> Test script:
> modprobe brd rd_nr=4 rd_size=10485760
> mdadm -CR /dev/md0 -l10 -n4 /dev/ram[0123] --assume-clean --bitmap=internal
> echo 0 > /proc/sys/vm/dirty_background_ratio
> fio -filename=/dev/md0 -ioengine=libaio -rw=write -bs=4k -numjobs=1 -iodepth=128 -name=test
>
> Test result:
> Monitor /sys/block/md0/inflight will found that inflight keep increasing
> until fio finish writing, after running for about 2 minutes:
>
> [root@fedora ~]# cat /sys/block/md0/inflight
> 0 4474191
>
> Fix the problem by limiting the number of plugged bio based on the number
> of copies for original bio.
>
> Signed-off-by: Yu Kuai <[email protected]>
> ---
> drivers/md/raid1-10.c | 9 ++++++++-
> drivers/md/raid1.c | 2 +-
> drivers/md/raid10.c | 2 +-
> 3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
> index 17e55c1fd5a1..bb1e23b66c45 100644
> --- a/drivers/md/raid1-10.c
> +++ b/drivers/md/raid1-10.c
> @@ -21,6 +21,7 @@
> #define IO_MADE_GOOD ((struct bio *)2)
>
> #define BIO_SPECIAL(bio) ((unsigned long)bio <= 2)
> +#define MAX_PLUG_BIO 32
>
> /* for managing resync I/O pages */
> struct resync_pages {
> @@ -31,6 +32,7 @@ struct resync_pages {
> struct raid1_plug_cb {
> struct blk_plug_cb cb;
> struct bio_list pending;
> + unsigned int count;
> };
>
> static void rbio_pool_free(void *rbio, void *data)
> @@ -128,7 +130,7 @@ static inline void raid1_submit_write(struct bio *bio)
> }
>
> static inline bool raid1_add_bio_to_plug(struct mddev *mddev, struct bio *bio,
> - blk_plug_cb_fn unplug)
> + blk_plug_cb_fn unplug, int copies)
> {
> struct raid1_plug_cb *plug = NULL;
> struct blk_plug_cb *cb;
> @@ -148,6 +150,11 @@ static inline bool raid1_add_bio_to_plug(struct mddev *mddev, struct bio *bio,
>
> plug = container_of(cb, struct raid1_plug_cb, cb);
> bio_list_add(&plug->pending, bio);
> + if (++plug->count / MAX_PLUG_BIO >= copies) {
> + list_del(&cb->list);
> + cb->callback(cb, false);
> + }
> +
It doesn't need this line here.
Have you done some performance tests with this patch set?
Regards
Xiao
>
> return true;
> }
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 006620fed595..dc89a1c4b1f1 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1562,7 +1562,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
> r1_bio->sector);
> /* flush_pending_writes() needs access to the rdev so...*/
> mbio->bi_bdev = (void *)rdev;
> - if (!raid1_add_bio_to_plug(mddev, mbio, raid1_unplug)) {
> + if (!raid1_add_bio_to_plug(mddev, mbio, raid1_unplug, disks)) {
> spin_lock_irqsave(&conf->device_lock, flags);
> bio_list_add(&conf->pending_bio_list, mbio);
> spin_unlock_irqrestore(&conf->device_lock, flags);
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index fb22cfe94d32..9237dbeb07ba 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1306,7 +1306,7 @@ static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
>
> atomic_inc(&r10_bio->remaining);
>
> - if (!raid1_add_bio_to_plug(mddev, mbio, raid10_unplug)) {
> + if (!raid1_add_bio_to_plug(mddev, mbio, raid10_unplug, conf->copies)) {
> spin_lock_irqsave(&conf->device_lock, flags);
> bio_list_add(&conf->pending_bio_list, mbio);
> spin_unlock_irqrestore(&conf->device_lock, flags);
> --
> 2.39.2
>
Hi,
在 2023/05/31 23:42, Xiao Ni 写道:
> On Mon, May 29, 2023 at 9:14 PM Yu Kuai <[email protected]> wrote:
>>
>> From: Yu Kuai <[email protected]>
>>
>> bio can be added to plug infinitely, and following writeback test can
>> trigger huge amount of plugged bio:
>>
>> Test script:
>> modprobe brd rd_nr=4 rd_size=10485760
>> mdadm -CR /dev/md0 -l10 -n4 /dev/ram[0123] --assume-clean --bitmap=internal
>> echo 0 > /proc/sys/vm/dirty_background_ratio
>> fio -filename=/dev/md0 -ioengine=libaio -rw=write -bs=4k -numjobs=1 -iodepth=128 -name=test
>>
>> Test result:
>> Monitor /sys/block/md0/inflight will found that inflight keep increasing
>> until fio finish writing, after running for about 2 minutes:
>>
>> [root@fedora ~]# cat /sys/block/md0/inflight
>> 0 4474191
>>
>> Fix the problem by limiting the number of plugged bio based on the number
>> of copies for original bio.
>>
>> Signed-off-by: Yu Kuai <[email protected]>
>> ---
>> drivers/md/raid1-10.c | 9 ++++++++-
>> drivers/md/raid1.c | 2 +-
>> drivers/md/raid10.c | 2 +-
>> 3 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
>> index 17e55c1fd5a1..bb1e23b66c45 100644
>> --- a/drivers/md/raid1-10.c
>> +++ b/drivers/md/raid1-10.c
>> @@ -21,6 +21,7 @@
>> #define IO_MADE_GOOD ((struct bio *)2)
>>
>> #define BIO_SPECIAL(bio) ((unsigned long)bio <= 2)
>> +#define MAX_PLUG_BIO 32
>>
>> /* for managing resync I/O pages */
>> struct resync_pages {
>> @@ -31,6 +32,7 @@ struct resync_pages {
>> struct raid1_plug_cb {
>> struct blk_plug_cb cb;
>> struct bio_list pending;
>> + unsigned int count;
>> };
>>
>> static void rbio_pool_free(void *rbio, void *data)
>> @@ -128,7 +130,7 @@ static inline void raid1_submit_write(struct bio *bio)
>> }
>>
>> static inline bool raid1_add_bio_to_plug(struct mddev *mddev, struct bio *bio,
>> - blk_plug_cb_fn unplug)
>> + blk_plug_cb_fn unplug, int copies)
>> {
>> struct raid1_plug_cb *plug = NULL;
>> struct blk_plug_cb *cb;
>> @@ -148,6 +150,11 @@ static inline bool raid1_add_bio_to_plug(struct mddev *mddev, struct bio *bio,
>>
>> plug = container_of(cb, struct raid1_plug_cb, cb);
>> bio_list_add(&plug->pending, bio);
>> + if (++plug->count / MAX_PLUG_BIO >= copies) {
>> + list_del(&cb->list);
>> + cb->callback(cb, false);
>> + }
>> +
> It doesn't need this line here.
>
> Have you done some performance tests with this patch set?
Just a simple fio script to test 4 ramdisk/loop 16thread 4k write in my
VM, and I didn't notice regression, however, I didn't run benchmarks
yet, I don't have such physical environment to test performance for
now...
I'll definitely run some performance tests in physical evironment later.
Thanks,
Kuai
>
> Regards
> Xiao
>>
>> return true;
>> }
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index 006620fed595..dc89a1c4b1f1 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -1562,7 +1562,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>> r1_bio->sector);
>> /* flush_pending_writes() needs access to the rdev so...*/
>> mbio->bi_bdev = (void *)rdev;
>> - if (!raid1_add_bio_to_plug(mddev, mbio, raid1_unplug)) {
>> + if (!raid1_add_bio_to_plug(mddev, mbio, raid1_unplug, disks)) {
>> spin_lock_irqsave(&conf->device_lock, flags);
>> bio_list_add(&conf->pending_bio_list, mbio);
>> spin_unlock_irqrestore(&conf->device_lock, flags);
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index fb22cfe94d32..9237dbeb07ba 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -1306,7 +1306,7 @@ static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
>>
>> atomic_inc(&r10_bio->remaining);
>>
>> - if (!raid1_add_bio_to_plug(mddev, mbio, raid10_unplug)) {
>> + if (!raid1_add_bio_to_plug(mddev, mbio, raid10_unplug, conf->copies)) {
>> spin_lock_irqsave(&conf->device_lock, flags);
>> bio_list_add(&conf->pending_bio_list, mbio);
>> spin_unlock_irqrestore(&conf->device_lock, flags);
>> --
>> 2.39.2
>>
>
> .
>