| Message ID | 20240522085056.54818-5-xni@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mdadm/tests: enhance/fix regression cases | expand |
On Wed, 22 May 2024 16:50:41 +0800 Xiao Ni <xni@redhat.com> wrote: > There are two changes. > First, if md module is not loaded, it gives error when reading > speed_limit_max. So read the value after loading md module which > is done in do_setup > > Second, sometimes the test reports error sync action doesn't > happen. But dmesg shows sync action is done. So limit the sync > speed before test. It doesn't affect the test run time. Because > check wait sets the max speed before waiting sync action. And > recording speed_limit_max/min in do_setup. > > Fixes: 4c12714d1ca0 ('test: run tests on system level mdadm') > Signed-off-by: Xiao Ni <xni@redhat.com> > --- > test | 10 +++++----- > tests/func.sh | 26 +++++++++++++++++++++++--- > 2 files changed, 28 insertions(+), 8 deletions(-) > > diff --git a/test b/test > index 338c2db44fa7..ff403293d60b 100755 > --- a/test > +++ b/test > @@ -6,7 +6,10 @@ targetdir="/var/tmp" > logdir="$targetdir" > config=/tmp/mdadm.conf > testdir=$PWD/tests > -system_speed_limit=`cat /proc/sys/dev/raid/speed_limit_max` > +system_speed_limit_max=0 > +system_speed_limit_min=0 > +test_speed_limit_min=100 > +test_speed_limit_max=500 > devlist= > > savelogs=0 > @@ -39,10 +42,6 @@ ctrl_c() { > ctrl_c_error=1 > } > > -restore_system_speed_limit() { > - echo $system_speed_limit > /proc/sys/dev/raid/speed_limit_max > -} > - > mdadm() { > rm -f $targetdir/stderr > case $* in > @@ -103,6 +102,7 @@ do_test() { > do_clean > # source script in a subshell, so it has access to our > # namespace, but cannot change it. > + control_system_speed_limit > echo -ne "$_script... " > if ( set -ex ; . $_script ) &> $targetdir/log > then > diff --git a/tests/func.sh b/tests/func.sh > index b474442b6abe..221cff158f8c 100644 > --- a/tests/func.sh > +++ b/tests/func.sh > @@ -136,6 +136,23 @@ check_env() { > fi > } > > +record_system_speed_limit() { > + system_speed_limit_max=`cat /proc/sys/dev/raid/speed_limit_max` > + system_speed_limit_min=`cat /proc/sys/dev/raid/speed_limit_min` > +} > + > +# To avoid sync action finishes before checking it, it needs to limit > +# the sync speed > +control_system_speed_limit() { > + echo $test_speed_limit_min > /proc/sys/dev/raid/speed_limit_min > + echo $test_speed_limit_max > /proc/sys/dev/raid/speed_limit_max > +} > + > +restore_system_speed_limit() { > + echo $system_speed_limit_min > /proc/sys/dev/raid/speed_limit_max > + echo $system_speed_limit_max > /proc/sys/dev/raid/speed_limit_max > +} > + Hi Xiao, control and restore functions are not used or in this patch. if you want to use them in next patches please highlight it in description. Beside that LGTM. I will loop over the patches and take as much I can. Looks like there are no conflicts. Thanks, Mariusz
On Thu, May 23, 2024 at 10:26 PM Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote: > > On Wed, 22 May 2024 16:50:41 +0800 > Xiao Ni <xni@redhat.com> wrote: > > > There are two changes. > > First, if md module is not loaded, it gives error when reading > > speed_limit_max. So read the value after loading md module which > > is done in do_setup > > > > Second, sometimes the test reports error sync action doesn't > > happen. But dmesg shows sync action is done. So limit the sync > > speed before test. It doesn't affect the test run time. Because > > check wait sets the max speed before waiting sync action. And > > recording speed_limit_max/min in do_setup. > > > > Fixes: 4c12714d1ca0 ('test: run tests on system level mdadm') > > Signed-off-by: Xiao Ni <xni@redhat.com> > > --- > > test | 10 +++++----- > > tests/func.sh | 26 +++++++++++++++++++++++--- > > 2 files changed, 28 insertions(+), 8 deletions(-) > > > > diff --git a/test b/test > > index 338c2db44fa7..ff403293d60b 100755 > > --- a/test > > +++ b/test > > @@ -6,7 +6,10 @@ targetdir="/var/tmp" > > logdir="$targetdir" > > config=/tmp/mdadm.conf > > testdir=$PWD/tests > > -system_speed_limit=`cat /proc/sys/dev/raid/speed_limit_max` > > +system_speed_limit_max=0 > > +system_speed_limit_min=0 > > +test_speed_limit_min=100 > > +test_speed_limit_max=500 > > devlist= > > > > savelogs=0 > > @@ -39,10 +42,6 @@ ctrl_c() { > > ctrl_c_error=1 > > } > > > > -restore_system_speed_limit() { > > - echo $system_speed_limit > /proc/sys/dev/raid/speed_limit_max > > -} > > - > > mdadm() { > > rm -f $targetdir/stderr > > case $* in > > @@ -103,6 +102,7 @@ do_test() { > > do_clean > > # source script in a subshell, so it has access to our > > # namespace, but cannot change it. > > + control_system_speed_limit > > echo -ne "$_script... " > > if ( set -ex ; . $_script ) &> $targetdir/log > > then > > diff --git a/tests/func.sh b/tests/func.sh > > index b474442b6abe..221cff158f8c 100644 > > --- a/tests/func.sh > > +++ b/tests/func.sh > > @@ -136,6 +136,23 @@ check_env() { > > fi > > } > > > > +record_system_speed_limit() { > > + system_speed_limit_max=`cat /proc/sys/dev/raid/speed_limit_max` > > + system_speed_limit_min=`cat /proc/sys/dev/raid/speed_limit_min` > > +} > > + > > +# To avoid sync action finishes before checking it, it needs to limit > > +# the sync speed > > +control_system_speed_limit() { > > + echo $test_speed_limit_min > /proc/sys/dev/raid/speed_limit_min > > + echo $test_speed_limit_max > /proc/sys/dev/raid/speed_limit_max > > +} > > + > > +restore_system_speed_limit() { > > + echo $system_speed_limit_min > /proc/sys/dev/raid/speed_limit_max > > + echo $system_speed_limit_max > /proc/sys/dev/raid/speed_limit_max > > +} > > + > > Hi Xiao, > control and restore functions are not used or in this patch. if you want to use > them in next patches please highlight it in description. > Beside that LGTM. Hi Mariusz The record_system_speed_limit and control_system_speed_limit are used in this patch. restore_system_speed_limit has been used before my patch set already. So I don't need to change it. Regards Xiao > > I will loop over the patches and take as much I can. Looks like there are no > conflicts. > > Thanks, > Mariusz >
On Thu, 23 May 2024 22:39:16 +0800 Xiao Ni <xni@redhat.com> wrote: > > Hi Xiao, > > control and restore functions are not used or in this patch. if you want to > > use them in next patches please highlight it in description. > > Beside that LGTM. > > Hi Mariusz > > The record_system_speed_limit and control_system_speed_limit are used > in this patch. restore_system_speed_limit has been used before my > patch set already. So I don't need to change it. > Thanks for fast response. The patches are not pushed yet, I will wait. I looped the code and don't see usages. You just declared them,where they are used? Thanks, Mariusz
On Wed, May 22, 2024 at 4:51 PM Xiao Ni <xni@redhat.com> wrote: > > There are two changes. > First, if md module is not loaded, it gives error when reading > speed_limit_max. So read the value after loading md module which > is done in do_setup > > Second, sometimes the test reports error sync action doesn't > happen. But dmesg shows sync action is done. So limit the sync > speed before test. It doesn't affect the test run time. Because > check wait sets the max speed before waiting sync action. And > recording speed_limit_max/min in do_setup. > > Fixes: 4c12714d1ca0 ('test: run tests on system level mdadm') > Signed-off-by: Xiao Ni <xni@redhat.com> > --- > test | 10 +++++----- > tests/func.sh | 26 +++++++++++++++++++++++--- > 2 files changed, 28 insertions(+), 8 deletions(-) > > diff --git a/test b/test > index 338c2db44fa7..ff403293d60b 100755 > --- a/test > +++ b/test > @@ -6,7 +6,10 @@ targetdir="/var/tmp" > logdir="$targetdir" > config=/tmp/mdadm.conf > testdir=$PWD/tests > -system_speed_limit=`cat /proc/sys/dev/raid/speed_limit_max` > +system_speed_limit_max=0 > +system_speed_limit_min=0 > +test_speed_limit_min=100 > +test_speed_limit_max=500 > devlist= > > savelogs=0 > @@ -39,10 +42,6 @@ ctrl_c() { > ctrl_c_error=1 > } > > -restore_system_speed_limit() { > - echo $system_speed_limit > /proc/sys/dev/raid/speed_limit_max > -} > - > mdadm() { > rm -f $targetdir/stderr > case $* in > @@ -103,6 +102,7 @@ do_test() { > do_clean > # source script in a subshell, so it has access to our > # namespace, but cannot change it. > + control_system_speed_limit It controls the system speed here. You can see restore_system_speed_limit in the source code. It was added in 4c12714d1ca06533fe7a887966df2558fd2f96b2 > echo -ne "$_script... " > if ( set -ex ; . $_script ) &> $targetdir/log > then > diff --git a/tests/func.sh b/tests/func.sh > index b474442b6abe..221cff158f8c 100644 > --- a/tests/func.sh > +++ b/tests/func.sh > @@ -136,6 +136,23 @@ check_env() { > fi > } > > +record_system_speed_limit() { > + system_speed_limit_max=`cat /proc/sys/dev/raid/speed_limit_max` > + system_speed_limit_min=`cat /proc/sys/dev/raid/speed_limit_min` > +} > + > +# To avoid sync action finishes before checking it, it needs to limit > +# the sync speed > +control_system_speed_limit() { > + echo $test_speed_limit_min > /proc/sys/dev/raid/speed_limit_min > + echo $test_speed_limit_max > /proc/sys/dev/raid/speed_limit_max > +} > + > +restore_system_speed_limit() { > + echo $system_speed_limit_min > /proc/sys/dev/raid/speed_limit_max > + echo $system_speed_limit_max > /proc/sys/dev/raid/speed_limit_max > +} > + > do_setup() { > trap cleanup 0 1 3 15 > trap ctrl_c 2 > @@ -214,6 +231,7 @@ do_setup() { > ulimit -c unlimited > [ -f /proc/mdstat ] || modprobe md_mod > echo 0 > /sys/module/md_mod/parameters/start_ro > + record_system_speed_limit And it records the system speed here. Best Regards Xiao > } > > # check various things > @@ -265,15 +283,17 @@ check() { > fi > ;; > wait ) > - p=`cat /proc/sys/dev/raid/speed_limit_max` > - echo 2000000 > /proc/sys/dev/raid/speed_limit_max > + min=`cat /proc/sys/dev/raid/speed_limit_min` > + max=`cat /proc/sys/dev/raid/speed_limit_max` > + echo 200000 > /proc/sys/dev/raid/speed_limit_max > sleep 0.1 > while grep -Eq '(resync|recovery|reshape|check|repair) *=' /proc/mdstat || > grep -v idle > /dev/null /sys/block/md*/md/sync_action > do > sleep 0.5 > done > - echo $p > /proc/sys/dev/raid/speed_limit_max > + echo $min > /proc/sys/dev/raid/speed_limit_min > + echo $max > /proc/sys/dev/raid/speed_limit_max > ;; > state ) > grep -sq "blocks.*\[$2\]\$" /proc/mdstat || > -- > 2.32.0 (Apple Git-132) > >
On Wed, 22 May 2024 16:50:41 +0800 Xiao Ni <xni@redhat.com> wrote: > There are two changes. > First, if md module is not loaded, it gives error when reading > speed_limit_max. So read the value after loading md module which > is done in do_setup > > Second, sometimes the test reports error sync action doesn't > happen. But dmesg shows sync action is done. So limit the sync > speed before test. It doesn't affect the test run time. Because > check wait sets the max speed before waiting sync action. And > recording speed_limit_max/min in do_setup. > > Fixes: 4c12714d1ca0 ('test: run tests on system level mdadm') > Signed-off-by: Xiao Ni <xni@redhat.com> > --- Applied! Thanks, Mariusz
diff --git a/test b/test index 338c2db44fa7..ff403293d60b 100755 --- a/test +++ b/test @@ -6,7 +6,10 @@ targetdir="/var/tmp" logdir="$targetdir" config=/tmp/mdadm.conf testdir=$PWD/tests -system_speed_limit=`cat /proc/sys/dev/raid/speed_limit_max` +system_speed_limit_max=0 +system_speed_limit_min=0 +test_speed_limit_min=100 +test_speed_limit_max=500 devlist= savelogs=0 @@ -39,10 +42,6 @@ ctrl_c() { ctrl_c_error=1 } -restore_system_speed_limit() { - echo $system_speed_limit > /proc/sys/dev/raid/speed_limit_max -} - mdadm() { rm -f $targetdir/stderr case $* in @@ -103,6 +102,7 @@ do_test() { do_clean # source script in a subshell, so it has access to our # namespace, but cannot change it. + control_system_speed_limit echo -ne "$_script... " if ( set -ex ; . $_script ) &> $targetdir/log then diff --git a/tests/func.sh b/tests/func.sh index b474442b6abe..221cff158f8c 100644 --- a/tests/func.sh +++ b/tests/func.sh @@ -136,6 +136,23 @@ check_env() { fi } +record_system_speed_limit() { + system_speed_limit_max=`cat /proc/sys/dev/raid/speed_limit_max` + system_speed_limit_min=`cat /proc/sys/dev/raid/speed_limit_min` +} + +# To avoid sync action finishes before checking it, it needs to limit +# the sync speed +control_system_speed_limit() { + echo $test_speed_limit_min > /proc/sys/dev/raid/speed_limit_min + echo $test_speed_limit_max > /proc/sys/dev/raid/speed_limit_max +} + +restore_system_speed_limit() { + echo $system_speed_limit_min > /proc/sys/dev/raid/speed_limit_max + echo $system_speed_limit_max > /proc/sys/dev/raid/speed_limit_max +} + do_setup() { trap cleanup 0 1 3 15 trap ctrl_c 2 @@ -214,6 +231,7 @@ do_setup() { ulimit -c unlimited [ -f /proc/mdstat ] || modprobe md_mod echo 0 > /sys/module/md_mod/parameters/start_ro + record_system_speed_limit } # check various things @@ -265,15 +283,17 @@ check() { fi ;; wait ) - p=`cat /proc/sys/dev/raid/speed_limit_max` - echo 2000000 > /proc/sys/dev/raid/speed_limit_max + min=`cat /proc/sys/dev/raid/speed_limit_min` + max=`cat /proc/sys/dev/raid/speed_limit_max` + echo 200000 > /proc/sys/dev/raid/speed_limit_max sleep 0.1 while grep -Eq '(resync|recovery|reshape|check|repair) *=' /proc/mdstat || grep -v idle > /dev/null /sys/block/md*/md/sync_action do sleep 0.5 done - echo $p > /proc/sys/dev/raid/speed_limit_max + echo $min > /proc/sys/dev/raid/speed_limit_min + echo $max > /proc/sys/dev/raid/speed_limit_max ;; state ) grep -sq "blocks.*\[$2\]\$" /proc/mdstat ||
There are two changes. First, if md module is not loaded, it gives error when reading speed_limit_max. So read the value after loading md module which is done in do_setup Second, sometimes the test reports error sync action doesn't happen. But dmesg shows sync action is done. So limit the sync speed before test. It doesn't affect the test run time. Because check wait sets the max speed before waiting sync action. And recording speed_limit_max/min in do_setup. Fixes: 4c12714d1ca0 ('test: run tests on system level mdadm') Signed-off-by: Xiao Ni <xni@redhat.com> --- test | 10 +++++----- tests/func.sh | 26 +++++++++++++++++++++++--- 2 files changed, 28 insertions(+), 8 deletions(-)