diff mbox series

[v2,blktests,1/5] tests/throtl: add first test for blk-throttle

Message ID 20240417022005.1410525-2-yukuai1@huaweicloud.com (mailing list archive)
State New, archived
Headers show
Series add new tests for blk-throttle | expand

Commit Message

Yu Kuai April 17, 2024, 2:20 a.m. UTC 
From: Yu Kuai <yukuai3@huawei.com>

Test basic functionality.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 tests/throtl/001     | 39 ++++++++++++++++++++++++
 tests/throtl/001.out |  6 ++++
 tests/throtl/rc      | 71 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+)
 create mode 100755 tests/throtl/001
 create mode 100644 tests/throtl/001.out
 create mode 100644 tests/throtl/rc

Comments

Shinichiro Kawasaki April 19, 2024, 4:29 a.m. UTC | #1 
On Apr 17, 2024 / 10:20, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>

Hi Yu, thank you for this series. It is great to expand the test coverage and
cover a number of regressions!

Please find my comments in line, and consider if they make sense for you or not.
I ran the test cases on my test machine and observed that e new test cases
passed except throtl/004. I will note the failure for the 4th patch.

> 
> Test basic functionality.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  tests/throtl/001     | 39 ++++++++++++++++++++++++
>  tests/throtl/001.out |  6 ++++
>  tests/throtl/rc      | 71 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 116 insertions(+)
>  create mode 100755 tests/throtl/001
>  create mode 100644 tests/throtl/001.out
>  create mode 100644 tests/throtl/rc
> 
> diff --git a/tests/throtl/001 b/tests/throtl/001
> new file mode 100755
> index 0000000..739efe2
> --- /dev/null
> +++ b/tests/throtl/001
> @@ -0,0 +1,39 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024 Yu Kuai
> +#
> +# Test basic functionality of blk-throttle
> +
> +. tests/throtl/rc
> +
> +DESCRIPTION="basic functionality"
> +QUICK=1
> +
> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	if ! _set_up_test nr_devices=1; then
> +		return 1;
> +	fi
> +
> +	bps_limit=$((1024 * 1024))
> +
> +	_set_limits wbps=$bps_limit
> +	_test_io write 4k 256
> +	_remove_limits
> +
> +	_set_limits wiops=256
> +	_test_io write 4k 256
> +	_remove_limits
> +
> +	_set_limits rbps=$bps_limit
> +	_test_io read 4k 256
> +	_remove_limits
> +
> +	_set_limits riops=256
> +	_test_io read 4k 256
> +	_remove_limits
> +
> +	_clean_up_test
> +	echo "Test complete"
> +}
> diff --git a/tests/throtl/001.out b/tests/throtl/001.out
> new file mode 100644
> index 0000000..a3edfdd
> --- /dev/null
> +++ b/tests/throtl/001.out
> @@ -0,0 +1,6 @@
> +Running throtl/001
> +1
> +1
> +1
> +1
> +Test complete
> diff --git a/tests/throtl/rc b/tests/throtl/rc
> new file mode 100644
> index 0000000..871102c
> --- /dev/null
> +++ b/tests/throtl/rc
> @@ -0,0 +1,71 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024 Yu Kuai
> +#
> +# Tests for blk-throttle
> +
> +. common/rc
> +. common/null_blk
> +
> +CG=/sys/fs/cgroup
> +TEST_DIR=$CG/blktests_throtl

The names of these two global variables sounds too geneic for me. I think they
have risk to have name conflict in the future. I suggest to drop them and use
the function and the global variable defined in common/cgroup, as follows.

  $CG -> "$(_cgroup2_base_dir)"
  $TEST_DIR -> "$CGROUP2_DIR"

For this change, "mkdir $TEST_DIR" in _set_up_test() needs to be replaced with
_init_cgroup2 call. Same for "rmdir $TEST_DIR" in _clean_up_test() with
_exit_cgroup2. This change will add some new shellcheck warns then some more
double quotations will be required around $(_cgroup2_base_dir) and $CGROUP2_DIR.

> +devname=nullb0

The global variable name devname is too generic, too. I suggest to use prefix
"_thr" or "THR" for the global variables and functions defined in
tests/throtl/rc. This devname can be "THR_DEV" or something like that.

Also, I suggest to use "thr_nullb" as the device name because null_blk device
name nullb0 can not be used when the null_blk driver is built-in. In short,
I suggest,

   THR_DEV=dev_nullb

> +
> +group_requires() {
> +	_have_root
> +	_have_null_blk
> +	_have_kernel_option BLK_DEV_THROTTLING
> +	_have_cgroup2_controller io

This rc file introduces the dependency to the bc command. I suggest to check the
requirement by adding one more check here:

    _have_proggram bc

> +}
> +
> +# Create a new null_blk device, and create a new blk-cgroup for test.
> +_set_up_test() {
> +	if ! _init_null_blk "$*"; then

_configure_null_blk is the better than _init_null_blk, because _init_null_blk
requires loadable null_blk and does not work with built-in null_blk. Some
blktests users run tests with built-in modules, so it is the better to avoid
dependencies to built-in drivers. Then I suggest as follows.

       if ! _configure_null_blk $THR_DEV "$@" power=1; then

Please note that "$@" should be used in place of "$*" to pass positional
parameters correctly. With this change, _set_up_test_by_configfs() that the 4th
patch introduced will not be required. nr_device=1 and power=1 options in
throtl/00x will not be required either.

Regarding other functions, I can think of renames as follows:

    _set_up_test -> _setup_thr
    _clean_up_test -> _cleanup_thr (or _exit_thr ?)
    _set_limits -> _thr_set_limits
    _remove_limits -> _thr_remove_limits
    _issue_io -> _thr_issue_io
    _test_io -> _thr_test_io

> +		return 1;
> +	fi
> +
> +	echo +io > $CG/cgroup.subtree_control
> +	mkdir $TEST_DIR
> +
> +	return 0;
> +}
> +
> +_clean_up_test() {
> +	rmdir $TEST_DIR
> +	echo -io > $CG/cgroup.subtree_control
> +	_exit_null_blk
> +}
> +
> +_set_limits() {

Nit: local variable declaration "local dev" will make the code a bit more
robust. Same for other functions in this file.

> +	dev=$(cat /sys/block/$devname/dev)
> +	echo "$dev $*" > $TEST_DIR/io.max
> +}
> +
> +_remove_limits() {
> +	dev=$(cat /sys/block/$devname/dev)
> +	echo "$dev rbps=max wbps=max riops=max wiops=max" > $TEST_DIR/io.max
> +}
> +
> +# Create an asynchronous thread and bind it to the specified blk-cgroup, issue
> +# IO and then print time elapsed to the second, blk-throttle limits should be
> +# set before this function.
> +_test_io() {
> +	{
> +		sleep 0.1
> +		start_time=$(date +%s.%N)
> +
> +		if [ "$1" == "read" ]; then
> +			dd if=/dev/$devname of=/dev/null bs="$2" count="$3" iflag=direct status=none
> +		elif [ "$1" == "write" ]; then
> +			dd of=/dev/$devname if=/dev/zero bs="$2" count="$3" oflag=direct status=none
> +		fi
> +
> +		end_time=$(date +%s.%N)
> +		elapsed=$(echo "$end_time - $start_time" | bc)
> +		printf "%.0f\n" "$elapsed"
> +	} &
> +
> +	pid=$!
> +	echo "$pid" > $TEST_DIR/cgroup.procs
> +	wait $pid
> +}
> -- 
> 2.39.2
>
Yu Kuai April 19, 2024, 8:01 a.m. UTC | #2 
Hi,

在 2024/04/19 12:29, Shinichiro Kawasaki 写道:
> On Apr 17, 2024 / 10:20, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
> 
> Hi Yu, thank you for this series. It is great to expand the test coverage and
> cover a number of regressions!
> 
> Please find my comments in line, and consider if they make sense for you or not.
> I ran the test cases on my test machine and observed that e new test cases
> passed except throtl/004. I will note the failure for the 4th patch.
> 
>>
>> Test basic functionality.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   tests/throtl/001     | 39 ++++++++++++++++++++++++
>>   tests/throtl/001.out |  6 ++++
>>   tests/throtl/rc      | 71 ++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 116 insertions(+)
>>   create mode 100755 tests/throtl/001
>>   create mode 100644 tests/throtl/001.out
>>   create mode 100644 tests/throtl/rc
>>
>> diff --git a/tests/throtl/001 b/tests/throtl/001
>> new file mode 100755
>> index 0000000..739efe2
>> --- /dev/null
>> +++ b/tests/throtl/001
>> @@ -0,0 +1,39 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2024 Yu Kuai
>> +#
>> +# Test basic functionality of blk-throttle
>> +
>> +. tests/throtl/rc
>> +
>> +DESCRIPTION="basic functionality"
>> +QUICK=1
>> +
>> +test() {
>> +	echo "Running ${TEST_NAME}"
>> +
>> +	if ! _set_up_test nr_devices=1; then
>> +		return 1;
>> +	fi
>> +
>> +	bps_limit=$((1024 * 1024))
>> +
>> +	_set_limits wbps=$bps_limit
>> +	_test_io write 4k 256
>> +	_remove_limits
>> +
>> +	_set_limits wiops=256
>> +	_test_io write 4k 256
>> +	_remove_limits
>> +
>> +	_set_limits rbps=$bps_limit
>> +	_test_io read 4k 256
>> +	_remove_limits
>> +
>> +	_set_limits riops=256
>> +	_test_io read 4k 256
>> +	_remove_limits
>> +
>> +	_clean_up_test
>> +	echo "Test complete"
>> +}
>> diff --git a/tests/throtl/001.out b/tests/throtl/001.out
>> new file mode 100644
>> index 0000000..a3edfdd
>> --- /dev/null
>> +++ b/tests/throtl/001.out
>> @@ -0,0 +1,6 @@
>> +Running throtl/001
>> +1
>> +1
>> +1
>> +1
>> +Test complete
>> diff --git a/tests/throtl/rc b/tests/throtl/rc
>> new file mode 100644
>> index 0000000..871102c
>> --- /dev/null
>> +++ b/tests/throtl/rc
>> @@ -0,0 +1,71 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2024 Yu Kuai
>> +#
>> +# Tests for blk-throttle
>> +
>> +. common/rc
>> +. common/null_blk
>> +
>> +CG=/sys/fs/cgroup
>> +TEST_DIR=$CG/blktests_throtl
> 
> The names of these two global variables sounds too geneic for me. I think they
> have risk to have name conflict in the future. I suggest to drop them and use
> the function and the global variable defined in common/cgroup, as follows.
> 
>    $CG -> "$(_cgroup2_base_dir)"
>    $TEST_DIR -> "$CGROUP2_DIR"
> 
> For this change, "mkdir $TEST_DIR" in _set_up_test() needs to be replaced with
> _init_cgroup2 call. Same for "rmdir $TEST_DIR" in _clean_up_test() with
> _exit_cgroup2. This change will add some new shellcheck warns then some more
> double quotations will be required around $(_cgroup2_base_dir) and $CGROUP2_DIR.

Ok, sounds good.
> 
>> +devname=nullb0
> 
> The global variable name devname is too generic, too. I suggest to use prefix
> "_thr" or "THR" for the global variables and functions defined in
> tests/throtl/rc. This devname can be "THR_DEV" or something like that.
> 
> Also, I suggest to use "thr_nullb" as the device name because null_blk device
> name nullb0 can not be used when the null_blk driver is built-in. In short,
> I suggest,

I'll try with built-in null_blk, and switch the name.

> 
>     THR_DEV=dev_nullb
> 
>> +
>> +group_requires() {
>> +	_have_root
>> +	_have_null_blk
>> +	_have_kernel_option BLK_DEV_THROTTLING
>> +	_have_cgroup2_controller io
> 
> This rc file introduces the dependency to the bc command. I suggest to check the
> requirement by adding one more check here:
> 
>      _have_proggram bc

Ok, and perhaps also check dd as well?

> 
>> +}
>> +
>> +# Create a new null_blk device, and create a new blk-cgroup for test.
>> +_set_up_test() {
>> +	if ! _init_null_blk "$*"; then
> 
> _configure_null_blk is the better than _init_null_blk, because _init_null_blk
> requires loadable null_blk and does not work with built-in null_blk. Some
> blktests users run tests with built-in modules, so it is the better to avoid
> dependencies to built-in drivers. Then I suggest as follows.
> 
>         if ! _configure_null_blk $THR_DEV "$@" power=1; then
> 
> Please note that "$@" should be used in place of "$*" to pass positional
> parameters correctly. With this change, _set_up_test_by_configfs() that the 4th
> patch introduced will not be required. nr_device=1 and power=1 options in
> throtl/00x will not be required either.
> 
> Regarding other functions, I can think of renames as follows:
> 
>      _set_up_test -> _setup_thr
>      _clean_up_test -> _cleanup_thr (or _exit_thr ?)
>      _set_limits -> _thr_set_limits
>      _remove_limits -> _thr_remove_limits
>      _issue_io -> _thr_issue_io
>      _test_io -> _thr_test_io

Will update in the next version.

Thanks,
Kuai
> 
>> +		return 1;
>> +	fi
>> +
>> +	echo +io > $CG/cgroup.subtree_control
>> +	mkdir $TEST_DIR
>> +
>> +	return 0;
>> +}
>> +
>> +_clean_up_test() {
>> +	rmdir $TEST_DIR
>> +	echo -io > $CG/cgroup.subtree_control
>> +	_exit_null_blk
>> +}
>> +
>> +_set_limits() {
> 
> Nit: local variable declaration "local dev" will make the code a bit more
> robust. Same for other functions in this file.
> 
>> +	dev=$(cat /sys/block/$devname/dev)
>> +	echo "$dev $*" > $TEST_DIR/io.max
>> +}
>> +
>> +_remove_limits() {
>> +	dev=$(cat /sys/block/$devname/dev)
>> +	echo "$dev rbps=max wbps=max riops=max wiops=max" > $TEST_DIR/io.max
>> +}
>> +
>> +# Create an asynchronous thread and bind it to the specified blk-cgroup, issue
>> +# IO and then print time elapsed to the second, blk-throttle limits should be
>> +# set before this function.
>> +_test_io() {
>> +	{
>> +		sleep 0.1
>> +		start_time=$(date +%s.%N)
>> +
>> +		if [ "$1" == "read" ]; then
>> +			dd if=/dev/$devname of=/dev/null bs="$2" count="$3" iflag=direct status=none
>> +		elif [ "$1" == "write" ]; then
>> +			dd of=/dev/$devname if=/dev/zero bs="$2" count="$3" oflag=direct status=none
>> +		fi
>> +
>> +		end_time=$(date +%s.%N)
>> +		elapsed=$(echo "$end_time - $start_time" | bc)
>> +		printf "%.0f\n" "$elapsed"
>> +	} &
>> +
>> +	pid=$!
>> +	echo "$pid" > $TEST_DIR/cgroup.procs
>> +	wait $pid
>> +}
>> -- 
>> 2.39.2
>> .
>
Shinichiro Kawasaki April 19, 2024, 8:50 a.m. UTC | #3 
On Apr 19, 2024 / 16:01, Yu Kuai wrote:
> Hi,
> 
> 在 2024/04/19 12:29, Shinichiro Kawasaki 写道:
> > On Apr 17, 2024 / 10:20, Yu Kuai wrote:
> > > From: Yu Kuai <yukuai3@huawei.com>
...
> > > +group_requires() {
> > > +	_have_root
> > > +	_have_null_blk
> > > +	_have_kernel_option BLK_DEV_THROTTLING
> > > +	_have_cgroup2_controller io
> > 
> > This rc file introduces the dependency to the bc command. I suggest to check the
> > requirement by adding one more check here:
> > 
> >      _have_proggram bc
> 
> Ok, and perhaps also check dd as well?

Not really, because README describes that blktests depends on GNU coreutils
which includes dd. There are many other test cases which use dd without
checking dd, then it will be odd to check it only for the throtl group.
diff mbox series

Patch

diff --git a/tests/throtl/001 b/tests/throtl/001
new file mode 100755
index 0000000..739efe2
--- /dev/null
+++ b/tests/throtl/001
@@ -0,0 +1,39 @@ 
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Yu Kuai
+#
+# Test basic functionality of blk-throttle
+
+. tests/throtl/rc
+
+DESCRIPTION="basic functionality"
+QUICK=1
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	if ! _set_up_test nr_devices=1; then
+		return 1;
+	fi
+
+	bps_limit=$((1024 * 1024))
+
+	_set_limits wbps=$bps_limit
+	_test_io write 4k 256
+	_remove_limits
+
+	_set_limits wiops=256
+	_test_io write 4k 256
+	_remove_limits
+
+	_set_limits rbps=$bps_limit
+	_test_io read 4k 256
+	_remove_limits
+
+	_set_limits riops=256
+	_test_io read 4k 256
+	_remove_limits
+
+	_clean_up_test
+	echo "Test complete"
+}
diff --git a/tests/throtl/001.out b/tests/throtl/001.out
new file mode 100644
index 0000000..a3edfdd
--- /dev/null
+++ b/tests/throtl/001.out
@@ -0,0 +1,6 @@ 
+Running throtl/001
+1
+1
+1
+1
+Test complete
diff --git a/tests/throtl/rc b/tests/throtl/rc
new file mode 100644
index 0000000..871102c
--- /dev/null
+++ b/tests/throtl/rc
@@ -0,0 +1,71 @@ 
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Yu Kuai
+#
+# Tests for blk-throttle
+
+. common/rc
+. common/null_blk
+
+CG=/sys/fs/cgroup
+TEST_DIR=$CG/blktests_throtl
+devname=nullb0
+
+group_requires() {
+	_have_root
+	_have_null_blk
+	_have_kernel_option BLK_DEV_THROTTLING
+	_have_cgroup2_controller io
+}
+
+# Create a new null_blk device, and create a new blk-cgroup for test.
+_set_up_test() {
+	if ! _init_null_blk "$*"; then
+		return 1;
+	fi
+
+	echo +io > $CG/cgroup.subtree_control
+	mkdir $TEST_DIR
+
+	return 0;
+}
+
+_clean_up_test() {
+	rmdir $TEST_DIR
+	echo -io > $CG/cgroup.subtree_control
+	_exit_null_blk
+}
+
+_set_limits() {
+	dev=$(cat /sys/block/$devname/dev)
+	echo "$dev $*" > $TEST_DIR/io.max
+}
+
+_remove_limits() {
+	dev=$(cat /sys/block/$devname/dev)
+	echo "$dev rbps=max wbps=max riops=max wiops=max" > $TEST_DIR/io.max
+}
+
+# Create an asynchronous thread and bind it to the specified blk-cgroup, issue
+# IO and then print time elapsed to the second, blk-throttle limits should be
+# set before this function.
+_test_io() {
+	{
+		sleep 0.1
+		start_time=$(date +%s.%N)
+
+		if [ "$1" == "read" ]; then
+			dd if=/dev/$devname of=/dev/null bs="$2" count="$3" iflag=direct status=none
+		elif [ "$1" == "write" ]; then
+			dd of=/dev/$devname if=/dev/zero bs="$2" count="$3" oflag=direct status=none
+		fi
+
+		end_time=$(date +%s.%N)
+		elapsed=$(echo "$end_time - $start_time" | bc)
+		printf "%.0f\n" "$elapsed"
+	} &
+
+	pid=$!
+	echo "$pid" > $TEST_DIR/cgroup.procs
+	wait $pid
+}