Hi,
在 2024/04/19 12:29, Shinichiro Kawasaki 写道:
On Apr 17, 2024 / 10:20, Yu Kuai wrote:
From: Yu Kuai <yukuai3@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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
.
