Re: [PATCH tests 1/5] tests: add a new test to check if pluged bio is unlimited for raid10

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Tue, 23 May 2023 21:38:56 +0800
Yu Kuai <yukuai1@huaweicloud.com> wrote:

> From: Yu Kuai <yukuai3@huawei.com>
> 
> Pluged bio is unlimited means that all submitted bio will be pluged, and
> those bio won't be issued to underlaying disks until blk_finish_plug() or
> blk_flush_plug(). In this case, a lot memory will be used for
> raid10_bio and io latency will be very bad.
> 
> This test do some dirty pages writeback for raid10, where plug is used, and
> check if device inflight counter exceed threshold.
> 
> This problem is supposed to be fixed by [1].

The test here is for md, mdadm has nothing to do here. I'm not against it
but please extract it to separate directory because like "md_tests".
We need to start grouping tests.
> 
> [1]
> https://lore.kernel.org/linux-raid/20230420112946.2869956-9-yukuai1@huaweicloud.com/
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  tests/22raid10plug | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 tests/22raid10plug
> 
> diff --git a/tests/22raid10plug b/tests/22raid10plug
> new file mode 100644
> index 00000000..fde4ce80
> --- /dev/null
> +++ b/tests/22raid10plug
> @@ -0,0 +1,41 @@
> +devs="$dev0 $dev1 $dev2 $dev3 $dev4 $dev5"
> +
> +# test will fail if inflight is observed to be greater
> +threshold=4096
> +
> +# create a simple raid10
> +mdadm --create --run --level=raid10 --raid-disks 6 $md0 $devs
> --bitmap=internal --assume-clean
You don't need 6 drives, 4 is enough (unless I miss something).
 
> +if [ $? -ne 0 ]; then
> +        die "create raid10 failed"
> +fi
> +
> +old_background=`cat /proc/sys/vm/dirty_background_ratio`
> +old=`cat /proc/sys/vm/dirty_ratio`
> +
> +# trigger background writeback
> +echo 0 > /proc/sys/vm/dirty_background_ratio
> +echo 60 > /proc/sys/vm/dirty_ratio
> +
> +# io pressure with buffer write
> +fio -filename=$md0 -ioengine=libaio -rw=write -bs=4k -numjobs=1 -iodepth=128
> -name=test -runtime=10 & +
> +pid=$!
> +
> +sleep 2
> +
> +# check if inflight exceed threshold
> +while true; do
> +        tmp=`cat /sys/block/md0/inflight | awk '{printf("%d\n", $1 + $2);}'`
> +        if [ $tmp -gt $threshold ]; then
> +                die "inflight is greater than 4096"

The message here is not meaningful, what 4096 is? Please add comment describing
why value above 4096 causes an error. We need to understand how the future
changes in md may affect this setting (I think that there is a correlation
between the value and MAX_PLUG_BIO).

> +                break

the break is dead condition because die has `exit` inside.

> +        elif [ $tmp -eq 0 ]; then
> +                break
> +        fi

I would prefer to make verification independent from user
environment and md device inflight state. Simply, we should rely on fio. If
there is a fio in background we should check if inflight doesn't exceeded
expected value. we should finish when fio ends. You set runtime to 10, please
think if we can make this shorter.

Thanks,
Mariusz

> +        sleep 0.1
> +done
> +
> +kill -9 $pid
> +mdadm -S $md0
> +echo $old_background > /proc/sys/vm/dirty_background_ratio
> +echo $old > /proc/sys/vm/dirty_ratio