[PATCH v2 blktests 0/5] add new tests for blk-throttle

[PATCH v2 blktests 0/5] add new tests for blk-throttle

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

changes in v2:
 - move lots of functions to rc

Yu Kuai (5):
  tests/throtl: add first test for blk-throttle
  tests/throtl: add a new test 002
  tests/throtl: add a new test 003
  tests/throtl: add a new test 004
  tests/throtl: add a new test 005

 tests/throtl/001     | 39 ++++++++++++++++++
 tests/throtl/001.out |  6 +++
 tests/throtl/002     | 30 ++++++++++++++
 tests/throtl/002.out |  4 ++
 tests/throtl/003     | 32 +++++++++++++++
 tests/throtl/003.out |  4 ++
 tests/throtl/004     | 37 +++++++++++++++++
 tests/throtl/004.out |  4 ++
 tests/throtl/005     | 37 +++++++++++++++++
 tests/throtl/005.out |  3 ++
 tests/throtl/rc      | 95 ++++++++++++++++++++++++++++++++++++++++++++
 11 files changed, 291 insertions(+)
 create mode 100755 tests/throtl/001
 create mode 100644 tests/throtl/001.out
 create mode 100755 tests/throtl/002
 create mode 100644 tests/throtl/002.out
 create mode 100755 tests/throtl/003
 create mode 100644 tests/throtl/003.out
 create mode 100755 tests/throtl/004
 create mode 100644 tests/throtl/004.out
 create mode 100755 tests/throtl/005
 create mode 100644 tests/throtl/005.out
 create mode 100644 tests/throtl/rc

-- 
2.39.2

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

[Yu Kuai]

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

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
+}
-- 
2.39.2

[PATCH v2 blktests 2/5] tests/throtl: add a new test 002

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Test iops limit over IO split, regression tests for:

commit 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 tests/throtl/002     | 30 ++++++++++++++++++++++++++++++
 tests/throtl/002.out |  4 ++++
 2 files changed, 34 insertions(+)
 create mode 100755 tests/throtl/002
 create mode 100644 tests/throtl/002.out

diff --git a/tests/throtl/002 b/tests/throtl/002
new file mode 100755
index 0000000..83cd27f
--- /dev/null
+++ b/tests/throtl/002
@@ -0,0 +1,30 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Yu Kuai
+#
+# Test iops limit work correctly for big IO of blk-throttle, regression test
+# for commit 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
+
+. tests/throtl/rc
+
+DESCRIPTION="iops limit over IO split"
+QUICK=1
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	if ! _set_up_test max_sectors=8; then
+		return 1;
+	fi
+
+	_set_limits wiops=256
+	_test_io write 1M 1
+	_remove_limits
+
+	_set_limits riops=256
+	_test_io read 1M 1
+	_remove_limits
+
+	_clean_up_test
+	echo "Test complete"
+}
diff --git a/tests/throtl/002.out b/tests/throtl/002.out
new file mode 100644
index 0000000..7e1ae85
--- /dev/null
+++ b/tests/throtl/002.out
@@ -0,0 +1,4 @@
+Running throtl/002
+1
+1
+Test complete
-- 
2.39.2

[PATCH v2 blktests 3/5] tests/throtl: add a new test 003

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Test bps limit over IO split, regression tests for:

commit 111be8839817 ("block-throttle: avoid double charge")

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 tests/throtl/003     | 32 ++++++++++++++++++++++++++++++++
 tests/throtl/003.out |  4 ++++
 2 files changed, 36 insertions(+)
 create mode 100755 tests/throtl/003
 create mode 100644 tests/throtl/003.out

diff --git a/tests/throtl/003 b/tests/throtl/003
new file mode 100755
index 0000000..806b602
--- /dev/null
+++ b/tests/throtl/003
@@ -0,0 +1,32 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Yu Kuai
+#
+# Test bps limit work correctly for big IO of blk-throttle, regression test for
+# commit 111be8839817 ("block-throttle: avoid double charge")
+
+. tests/throtl/rc
+
+DESCRIPTION="bps limit over IO split"
+QUICK=1
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	if ! _set_up_test max_sectors=8; then
+		return 1;
+	fi
+
+	limit=$((1024 * 1024))
+
+	_set_limits wbps=$limit
+	_test_io write 1M 1
+	_remove_limits
+
+	_set_limits rbps=$limit
+	_test_io read 1M 1
+	_remove_limits
+
+	_clean_up_test
+	echo "Test complete"
+}
diff --git a/tests/throtl/003.out b/tests/throtl/003.out
new file mode 100644
index 0000000..07a80b3
--- /dev/null
+++ b/tests/throtl/003.out
@@ -0,0 +1,4 @@
+Running throtl/003
+1
+1
+Test complete
-- 
2.39.2

[PATCH v2 blktests 4/5] tests/throtl: add a new test 004

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Test delete the disk while IO is throttled, regerssion test for:

commit 884f0e84f1e3 ("blk-throttle: fix UAF by deleteing timer in blk_throtl_exit()")
commit 8f9e7b65f833 ("block: cancel all throttled bios in del_gendisk()")

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 tests/throtl/004     | 37 +++++++++++++++++++++++++++++++++++
 tests/throtl/004.out |  4 ++++
 tests/throtl/rc      | 46 +++++++++++++++++++++++++++++++++-----------
 3 files changed, 76 insertions(+), 11 deletions(-)
 create mode 100755 tests/throtl/004
 create mode 100644 tests/throtl/004.out

diff --git a/tests/throtl/004 b/tests/throtl/004
new file mode 100755
index 0000000..f2567c0
--- /dev/null
+++ b/tests/throtl/004
@@ -0,0 +1,37 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Yu Kuai
+#
+# Test delete the disk while IO is throttled, regerssion test for
+# commit 884f0e84f1e3 ("blk-throttle: fix UAF by deleteing timer in blk_throtl_exit()")
+# commit 8f9e7b65f833 ("block: cancel all throttled bios in del_gendisk()")
+
+. tests/throtl/rc
+
+DESCRIPTION="delete disk while IO is throttled"
+QUICK=1
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	if ! _set_up_test_by_configfs power=1; then
+		return 1;
+	fi
+
+	_set_limits wbps=$((1024 * 1024))
+
+	{
+		sleep 0.1
+		_issue_io write 10M 1
+	} &
+
+	pid=$!
+	echo "$pid" > $TEST_DIR/cgroup.procs
+
+	sleep 1
+	echo 0 > /sys/kernel/config/nullb/$devname/power
+	wait "$pid"
+
+	_clean_up_test
+	echo "Test complete"
+}
diff --git a/tests/throtl/004.out b/tests/throtl/004.out
new file mode 100644
index 0000000..03331fe
--- /dev/null
+++ b/tests/throtl/004.out
@@ -0,0 +1,4 @@
+Running throtl/004
+dd: error writing '/dev/nullb0': Input/output error
+1
+Test complete
diff --git a/tests/throtl/rc b/tests/throtl/rc
index 871102c..f70e250 100644
--- a/tests/throtl/rc
+++ b/tests/throtl/rc
@@ -30,6 +30,21 @@ _set_up_test() {
 	return 0;
 }
 
+_set_up_test_by_configfs() {
+	if ! _init_null_blk nr_devices=0; then
+		return 1;
+	fi
+
+	if ! _configure_null_blk $devname "$*"; 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
@@ -46,23 +61,32 @@ _remove_limits() {
 	echo "$dev rbps=max wbps=max riops=max wiops=max" > $TEST_DIR/io.max
 }
 
+_issue_io()
+{
+	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"
+}
+
 # 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)
+		rw=$1
+		bs=$2
+		count=$3
 
-		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"
+		sleep 0.1
+		_issue_io "$rw" "$bs" "$count"
 	} &
 
 	pid=$!
-- 
2.39.2

[PATCH v2 blktests 5/5] tests/throtl: add a new test 005

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Test change config while IO is throttled, regression test for:

commit a880ae93e5b5 ("blk-throttle: fix io hung due to configuration updates")

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 tests/throtl/005     | 37 +++++++++++++++++++++++++++++++++++++
 tests/throtl/005.out |  3 +++
 2 files changed, 40 insertions(+)
 create mode 100755 tests/throtl/005
 create mode 100644 tests/throtl/005.out

diff --git a/tests/throtl/005 b/tests/throtl/005
new file mode 100755
index 0000000..5ee5e7d
--- /dev/null
+++ b/tests/throtl/005
@@ -0,0 +1,37 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Yu Kuai
+#
+# Test change config while IO is throttled, regression test for
+# commit a880ae93e5b5 ("blk-throttle: fix io hung due to configuration updates")
+
+. tests/throtl/rc
+
+DESCRIPTION="change config with throttled IO"
+QUICK=1
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	if ! _set_up_test nr_devices=1; then
+		return 1;
+	fi
+
+	_set_limits wbps=$((512 * 1024))
+
+	{
+		sleep 0.1
+		_issue_io write 1M 1
+	} &
+
+	pid=$!
+	echo "$pid" > $TEST_DIR/cgroup.procs
+
+	sleep 1
+	_set_limits wbps=$((256 * 1024))
+	wait $pid
+	_remove_limits
+
+	_clean_up_test
+	echo "Test complete"
+}
diff --git a/tests/throtl/005.out b/tests/throtl/005.out
new file mode 100644
index 0000000..1d23210
--- /dev/null
+++ b/tests/throtl/005.out
@@ -0,0 +1,3 @@
+Running throtl/005
+3
+Test complete
-- 
2.39.2

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

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

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

Re: [PATCH v2 blktests 4/5] tests/throtl: add a new test 004

[Shinichiro Kawasaki]

On Apr 17, 2024 / 10:20, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Test delete the disk while IO is throttled, regerssion test for:

Nit: s/regerssion/regression/

> 
> commit 884f0e84f1e3 ("blk-throttle: fix UAF by deleteing timer in blk_throtl_exit()")
> commit 8f9e7b65f833 ("block: cancel all throttled bios in del_gendisk()")
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  tests/throtl/004     | 37 +++++++++++++++++++++++++++++++++++
>  tests/throtl/004.out |  4 ++++
>  tests/throtl/rc      | 46 +++++++++++++++++++++++++++++++++-----------
>  3 files changed, 76 insertions(+), 11 deletions(-)
>  create mode 100755 tests/throtl/004
>  create mode 100644 tests/throtl/004.out
> 
> diff --git a/tests/throtl/004 b/tests/throtl/004
> new file mode 100755
> index 0000000..f2567c0
> --- /dev/null
> +++ b/tests/throtl/004
> @@ -0,0 +1,37 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024 Yu Kuai
> +#
> +# Test delete the disk while IO is throttled, regerssion test for

Nit: s/regerssion/regression/

> +# commit 884f0e84f1e3 ("blk-throttle: fix UAF by deleteing timer in blk_throtl_exit()")
> +# commit 8f9e7b65f833 ("block: cancel all throttled bios in del_gendisk()")
> +
> +. tests/throtl/rc
> +
> +DESCRIPTION="delete disk while IO is throttled"
> +QUICK=1
> +
> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	if ! _set_up_test_by_configfs power=1; then
> +		return 1;
> +	fi
> +
> +	_set_limits wbps=$((1024 * 1024))
> +
> +	{
> +		sleep 0.1
> +		_issue_io write 10M 1
> +	} &
> +
> +	pid=$!
> +	echo "$pid" > $TEST_DIR/cgroup.procs
> +
> +	sleep 1

When I ran this test case on my QEMU test machine, it failed with the message
below. When I repeat the test case, it sometimes passes but fails in most
cases. I guess this is because my machine is slow and takes some time from
the disk deletion to write process exit.

throtl/004 (delete disk while IO is throttled)               [failed]
    runtime  1.085s  ...  1.997s
    --- tests/throtl/004.out    2024-04-19 11:26:56.507007360 +0900
    +++ /home/shin/Blktests/blktests/results/nodev/throtl/004.out.bad   2024-04-19 13:34:03.766045990 +0900
    @@ -1,4 +1,4 @@
     Running throtl/004
     dd: error writing '/dev/thr_nullb': Input/output error
    -1
    +2
     Test complete

When I change the "sleep 1" in line above to "sleep .6", then the test case
passed even when I repeat the test case run several times. This changes adds
some margin and will make the result elapsed time to be round up to "1" not to
"2". Still 0.6 second delay on the wait process and 0.1 second delay on the
write process has the gap 0.5 second, then it is ensured to be rounded to "1".
So I guess the sleep time 0.6 is the valid number, probably.

> +	echo 0 > /sys/kernel/config/nullb/$devname/power
> +	wait "$pid"
> +
> +	_clean_up_test
> +	echo "Test complete"
> +}
> diff --git a/tests/throtl/004.out b/tests/throtl/004.out
> new file mode 100644
> index 0000000..03331fe
> --- /dev/null
> +++ b/tests/throtl/004.out
> @@ -0,0 +1,4 @@
> +Running throtl/004
> +dd: error writing '/dev/nullb0': Input/output error
> +1
> +Test complete
> diff --git a/tests/throtl/rc b/tests/throtl/rc
> index 871102c..f70e250 100644
> --- a/tests/throtl/rc
> +++ b/tests/throtl/rc
> @@ -30,6 +30,21 @@ _set_up_test() {
>  	return 0;
>  }
>  
> +_set_up_test_by_configfs() {
> +	if ! _init_null_blk nr_devices=0; then
> +		return 1;
> +	fi
> +
> +	if ! _configure_null_blk $devname "$*"; 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
> @@ -46,23 +61,32 @@ _remove_limits() {
>  	echo "$dev rbps=max wbps=max riops=max wiops=max" > $TEST_DIR/io.max
>  }
>  
> +_issue_io()
> +{
> +	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"
> +}
> +
>  # 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)
> +		rw=$1
> +		bs=$2
> +		count=$3
>  
> -		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"
> +		sleep 0.1
> +		_issue_io "$rw" "$bs" "$count"
>  	} &
>  
>  	pid=$!
> -- 
> 2.39.2
> 

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

[Yu Kuai]

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

Re: [PATCH v2 blktests 4/5] tests/throtl: add a new test 004

[Yu Kuai]

Hi,

在 2024/04/19 12:45, Shinichiro Kawasaki 写道:
> On Apr 17, 2024 / 10:20, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Test delete the disk while IO is throttled, regerssion test for:
> 
> Nit: s/regerssion/regression/
> 
>>
>> commit 884f0e84f1e3 ("blk-throttle: fix UAF by deleteing timer in blk_throtl_exit()")
>> commit 8f9e7b65f833 ("block: cancel all throttled bios in del_gendisk()")
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   tests/throtl/004     | 37 +++++++++++++++++++++++++++++++++++
>>   tests/throtl/004.out |  4 ++++
>>   tests/throtl/rc      | 46 +++++++++++++++++++++++++++++++++-----------
>>   3 files changed, 76 insertions(+), 11 deletions(-)
>>   create mode 100755 tests/throtl/004
>>   create mode 100644 tests/throtl/004.out
>>
>> diff --git a/tests/throtl/004 b/tests/throtl/004
>> new file mode 100755
>> index 0000000..f2567c0
>> --- /dev/null
>> +++ b/tests/throtl/004
>> @@ -0,0 +1,37 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2024 Yu Kuai
>> +#
>> +# Test delete the disk while IO is throttled, regerssion test for
> 
> Nit: s/regerssion/regression/
> 
>> +# commit 884f0e84f1e3 ("blk-throttle: fix UAF by deleteing timer in blk_throtl_exit()")
>> +# commit 8f9e7b65f833 ("block: cancel all throttled bios in del_gendisk()")
>> +
>> +. tests/throtl/rc
>> +
>> +DESCRIPTION="delete disk while IO is throttled"
>> +QUICK=1
>> +
>> +test() {
>> +	echo "Running ${TEST_NAME}"
>> +
>> +	if ! _set_up_test_by_configfs power=1; then
>> +		return 1;
>> +	fi
>> +
>> +	_set_limits wbps=$((1024 * 1024))
>> +
>> +	{
>> +		sleep 0.1
>> +		_issue_io write 10M 1
>> +	} &
>> +
>> +	pid=$!
>> +	echo "$pid" > $TEST_DIR/cgroup.procs
>> +
>> +	sleep 1
> 
> When I ran this test case on my QEMU test machine, it failed with the message
> below. When I repeat the test case, it sometimes passes but fails in most
> cases. I guess this is because my machine is slow and takes some time from
> the disk deletion to write process exit.
> 
> throtl/004 (delete disk while IO is throttled)               [failed]
>      runtime  1.085s  ...  1.997s
>      --- tests/throtl/004.out    2024-04-19 11:26:56.507007360 +0900
>      +++ /home/shin/Blktests/blktests/results/nodev/throtl/004.out.bad   2024-04-19 13:34:03.766045990 +0900
>      @@ -1,4 +1,4 @@
>       Running throtl/004
>       dd: error writing '/dev/thr_nullb': Input/output error
>      -1
>      +2
>       Test complete
> 
> When I change the "sleep 1" in line above to "sleep .6", then the test case
> passed even when I repeat the test case run several times. This changes adds
> some margin and will make the result elapsed time to be round up to "1" not to
> "2". Still 0.6 second delay on the wait process and 0.1 second delay on the
> write process has the gap 0.5 second, then it is ensured to be rounded to "1".
> So I guess the sleep time 0.6 is the valid number, probably.

Yes, I was worried about slow machine, that's why I creat the null_blk
with 1s IO latency. use 0.6 sounds good to me, time precision is still
one second.

Thanks,
Kuai
> 
>> +	echo 0 > /sys/kernel/config/nullb/$devname/power
>> +	wait "$pid"
>> +
>> +	_clean_up_test
>> +	echo "Test complete"
>> +}
>> diff --git a/tests/throtl/004.out b/tests/throtl/004.out
>> new file mode 100644
>> index 0000000..03331fe
>> --- /dev/null
>> +++ b/tests/throtl/004.out
>> @@ -0,0 +1,4 @@
>> +Running throtl/004
>> +dd: error writing '/dev/nullb0': Input/output error
>> +1
>> +Test complete
>> diff --git a/tests/throtl/rc b/tests/throtl/rc
>> index 871102c..f70e250 100644
>> --- a/tests/throtl/rc
>> +++ b/tests/throtl/rc
>> @@ -30,6 +30,21 @@ _set_up_test() {
>>   	return 0;
>>   }
>>   
>> +_set_up_test_by_configfs() {
>> +	if ! _init_null_blk nr_devices=0; then
>> +		return 1;
>> +	fi
>> +
>> +	if ! _configure_null_blk $devname "$*"; 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
>> @@ -46,23 +61,32 @@ _remove_limits() {
>>   	echo "$dev rbps=max wbps=max riops=max wiops=max" > $TEST_DIR/io.max
>>   }
>>   
>> +_issue_io()
>> +{
>> +	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"
>> +}
>> +
>>   # 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)
>> +		rw=$1
>> +		bs=$2
>> +		count=$3
>>   
>> -		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"
>> +		sleep 0.1
>> +		_issue_io "$rw" "$bs" "$count"
>>   	} &
>>   
>>   	pid=$!
>> -- 
>> 2.39.2
>> .
> 

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

[Shinichiro Kawasaki]

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.