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

Hi,

在 2023/05/24 15:53, Mariusz Tkaczyk 写道:

On Tue, 23 May 2023 21:38:56 +0800
Yu Kuai [off-list ref] wrote:

quoted

From: Yu Kuai <redacted>

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.

Sorry I don't understand this, currently the test for md is here, I
don't see why need a seperate directory, is there a plan to move all
these tests into new directory?

quoted

[1]
https://lore.kernel.org/linux-raid/20230420112946.2869956-9-yukuai1@huaweicloud.com/ (local)

Signed-off-by: Yu Kuai <redacted>
---
  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).

Yes, 4 is enough.

quoted

+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).

MAX_PLUG_BIO is just limit for one task, I'm not sure if there will only
be one task issuing io, that why I choose a much larger value 4096.

quoted

+                break

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

Yes, I found that while writing other tests, I'll use trap to make sure
everthing is cleaned up.

quoted

+        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.

I'll change that check inflight when fio is in backgroud. But I don't
10s can be shorter, in my VM, it can take 1-5s to trigger the problem, I
was worried that 10s might not be enough for someone...

Thanks,
Kuai

Thanks,
Mariusz

quoted

+        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

.