Re: [PATCH blktests] loop/009: add test for loop partition uvents

From: Shin'ichiro Kawasaki <shinichiro@fastmail.com>
To: Alyssa Ross <hi@alyssa.is>
Cc: chaitanyak@nvidia.com,
	Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>,
	axboe@kernel.dk, hch@lst.de, linux-block@vger.kernel.org
Subject: Re: [PATCH blktests] loop/009: add test for loop partition uvents
Date: Thu, 6 Apr 2023 19:30:03 +0900	[thread overview]
Message-ID: <20230406103003.u5j7m6xqflcli7y2@shinhome> (raw)
In-Reply-To: <20230330160247.16030-1-hi@alyssa.is>

Hello Alyssa, thanks for the patch and sorry for this late response.

Please find one comment in line. Other than that, this patch looks good to me.
I also ran the test case in my environment and confirmed that it passes with
the kernel fix. Good.

On Mar 30, 2023 / 16:02, Alyssa Ross wrote:
> Link: https://lore.kernel.org/r/20230320125430.55367-1-hch@lst.de/
> Suggested-by: Chaitanya Kulkarni <chaitanyak@nvidia.com>
> Signed-off-by: Alyssa Ross <hi@alyssa.is>
> ---
>  tests/loop/009     | 62 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/loop/009.out |  3 +++
>  2 files changed, 65 insertions(+)
>  create mode 100755 tests/loop/009
>  create mode 100644 tests/loop/009.out
> 
> diff --git a/tests/loop/009 b/tests/loop/009
> new file mode 100755
> index 0000000..dfa9de3
> --- /dev/null
> +++ b/tests/loop/009
> @@ -0,0 +1,62 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright 2023 Alyssa Ross
> +#
> +# Regression test for patch "loop: LOOP_CONFIGURE: send uevents for partitions".
> +
> +. tests/loop/rc
> +
> +DESCRIPTION="check that LOOP_CONFIGURE sends uevents for partitions"
> +
> +QUICK=1
> +
> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	# Make a disk image with a partition.
> +	truncate -s 3MiB "$TMPDIR/img"
> +	sfdisk "$TMPDIR/img" >"$FULL" <<EOF
> +label: gpt
> +size=1MiB
> +EOF
> +
> +	mkfifo "$TMPDIR/mon"
> +	timeout 5 udevadm monitor -ks block/partition >"$TMPDIR/mon" &
> +
> +	# Open the fifo for reading, and wait for udevadm monitor to start.
> +	exec 3< "$TMPDIR/mon"
> +	read -r _ <&3
> +	read -r _ <&3
> +	read -r _ <&3
> +
> +	dev="$(losetup -f)"
> +
> +	# The default udev behavior is to watch loop devices, which means that
> +	# udevd will explicitly prompt the kernel to rescan the partitions with
> +	# ioctl(BLKRRPART).  We want to make sure we're getting uevents from
> +	# ioctl(LOOP_CONFIGURE), so disable this udev behavior for our device to
> +	# avoid false positives.
> +	echo "ACTION!=\"remove\", KERNEL==\"${dev#/dev/}\", OPTIONS+=\"nowatch\"" \
> +		>/run/udev/rules.d/99-blktests-$$.rules

On Fedora Server 37, the line above prints a failure message because the
directory /run/udev/rules.d/ does not exist. To avoid it, I needed the change
below. I suggest to apply this change.

diff --git a/tests/loop/009 b/tests/loop/009
index dfa9de3..2b7a042 100755
--- a/tests/loop/009
+++ b/tests/loop/009
@@ -36,6 +36,7 @@ EOF
        # ioctl(BLKRRPART).  We want to make sure we're getting uevents from
        # ioctl(LOOP_CONFIGURE), so disable this udev behavior for our device to
        # avoid false positives.
+       [[ ! -d /run/udev/rules.d ]] && mkdir -p /run/udev/rules.d
        echo "ACTION!=\"remove\", KERNEL==\"${dev#/dev/}\", OPTIONS+=\"nowatch\"" \
                >/run/udev/rules.d/99-blktests-$$.rules
        udevadm control -R

> +	udevadm control -R
> +
> +	# Open and close the loop device for writing, to trigger the inotify
> +	# event udevd had already started listening for.
> +	: > "$dev"
> +
> +	# Wait for udev to have processed the inotify event.
> +	udevadm control --ping
> +
> +	losetup -P "$dev" "$TMPDIR/img"
> +
> +	# Wait for at most 1 add event so we don't need to wait for timeout if
> +	# we get what we're looking for.
> +	<&3 grep -m 1 '^KERNEL\[.*\] add' |
> +		sed -e 's/\[.*\]//' -e 's/loop[0-9]\+/loop_/g'
> +
> +	rm /run/udev/rules.d/99-blktests-$$.rules
> +	udevadm control -R
> +	losetup -d "$dev"
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/loop/009.out b/tests/loop/009.out
> new file mode 100644
> index 0000000..658dcff
> --- /dev/null
> +++ b/tests/loop/009.out
> @@ -0,0 +1,3 @@
> +Running loop/009
> +KERNEL add      /devices/virtual/block/loop_/loop_p1 (block)
> +Test complete
> -- 
> 2.37.1
> 
