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