Re: [PATCH blktests v2 2/2] block/033: add test to cover gendisk leak

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

On Feb 17, 2023 / 09:38, Ming Lei wrote:
> So far only sync ublk removal is supported, and the device's
> last reference is dropped in gendisk's ->free_disk(), so it
> can be used to test gendisk leak issue.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  common/ublk         | 34 ++++++++++++++++++++++++++++++++++
>  tests/block/033     | 40 ++++++++++++++++++++++++++++++++++++++++
>  tests/block/033.out |  2 ++
>  3 files changed, 76 insertions(+)
>  create mode 100644 common/ublk
>  create mode 100755 tests/block/033
>  create mode 100644 tests/block/033.out
> 
> diff --git a/common/ublk b/common/ublk
> new file mode 100644
> index 0000000..a7b442a
> --- /dev/null
> +++ b/common/ublk
> @@ -0,0 +1,34 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2023 Ming Lei
> +#
> +# ublk_drv helper functions.
> +
> +. common/shellcheck
> +
> +_have_ublk() {
> +	_have_driver ublk_drv
> +	_have_src_program ublk/miniublk
> +}
> +
> +_remove_ublk_devices() {
> +	src/ublk/miniublk del -a
> +}
> +
> +_init_ublk() {
> +	_remove_ublk_devices
> +
> +	if ! modprobe -r ublk_drv || ! modprobe ublk_drv; then

When ublk_drv is built as a built-in module, modprobe -r fails and prints out an
error. I suggest to modify the line above as follows:

	modprobe -rq ublk_drv
	if ! modprobe ublk_drv; then

With this, I confirmed that the test case can be run with built-in ublk_drv.
I suggest to remove the word "modular" in the message below.

> +		SKIP_REASONS+=("requires modular ublk_drv")
> +		return 1
> +	fi
> +
> +	udevadm settle
> +	return 0
> +}
> +
> +_exit_ublk() {
> +	_remove_ublk_devices
> +	udevadm settle
> +	modprobe -r -q ublk_drv
> +}
> diff --git a/tests/block/033 b/tests/block/033
> new file mode 100755
> index 0000000..eaba599
> --- /dev/null
> +++ b/tests/block/033
> @@ -0,0 +1,40 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2023 Ming Lei
> +#
> +# Test if gendisk is leaked, and regression in the following commit
> +# c43332fe028c ("blk-cgroup: delay calling blkcg_exit_disk until disk_release")
> +# can be covered
> +
> +. tests/block/rc
> +. common/ublk
> +
> +DESCRIPTION="add & delete ublk device and test if gendisk is leaked"
> +QUICK=1
> +
> +requires() {
> +	_have_ublk
> +}
> +
> +test() {
> +	local ublk_prog="src/ublk/miniublk"
> +
> +	echo "Running ${TEST_NAME}"
> +
> +	if ! _init_ublk; then
> +		return 1
> +	fi
> +
> +	${ublk_prog} add -t null -n 0 --quiet > /dev/null 2>&1
> +	if ! ${ublk_prog} list -n 0 > /dev/null 2>&1; then
> +		echo "fail to list dev"
> +	fi

When I repeated to run this test case, I often observed failure. The dd below
reported an error: "failed to open '/dev/ublkb0': No such file or directory". I
added "udevadm settle" here, then the failure disappeared.

> +	if ! dd if=/dev/ublkb0 iflag=direct of=/dev/null bs=1M count=512 > /dev/null 2>&1; then
> +		echo "fail io"
> +	fi
> +	${ublk_prog} del -n 0 > /dev/null 2>&1

The outputs of ${ublk_prog} and dd are all redirected to /dev/null, so it was a
bit difficult for me to debug the error. I suggest to redirect and append to
"$FULL" instead.

> +
> +	_exit_ublk
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/block/033.out b/tests/block/033.out
> new file mode 100644
> index 0000000..067846a
> --- /dev/null
> +++ b/tests/block/033.out
> @@ -0,0 +1,2 @@
> +Running block/033
> +Test complete
> -- 
> 2.31.1
> 

-- 
Shin'ichiro Kawasaki