[PATCH blktests] loop/010: do not assume /dev/loop0

[PATCH blktests] loop/010: do not assume /dev/loop0

[Shin'ichiro Kawasaki]

The current implementation of the test case loop/010 assumes that the
prepared loop device is /dev/loop0, which is not always true. When other
loop devices are set up before the test case run, the assumption is
wrong and the test case fails.

To avoid the failure, use the prepared loop device name stored in
$loop_device instead of /dev/loop0. Adjust the grep string to meet the
device name. Also use "losetup --detach" instead of
"losetup --detach-all" to not detach the loop devices which existed
before the test case runs.

Fixes: 1c4ae4fed9b4 ("loop: Detect a race condition between loop detach and open")
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/loop/010 | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/tests/loop/010 b/tests/loop/010
index ea396ec..f8c6f2c 100755
--- a/tests/loop/010
+++ b/tests/loop/010
@@ -16,18 +16,26 @@ requires() {
 }
 
 create_loop() {
+	local dev
+
 	while true
 	do
-		loop_device="$(losetup --partscan --find --show "${image_file}")"
-		blkid /dev/loop0p1 >& /dev/null
+		dev="$(losetup --partscan --find --show "${image_file}")"
+		if [[ $dev != "$1" ]]; then
+			echo "Unepxected loop device set up: $dev"
+			return
+		fi
+		blkid "$dev" >& /dev/null
 	done
 }
 
 detach_loop() {
+	local dev=$1
+
 	while true
 	do
-		if [ -e /dev/loop0 ]; then
-			losetup --detach /dev/loop0 >& /dev/null
+		if [[ -e "$dev" ]]; then
+			losetup --detach "$dev" >& /dev/null
 		fi
 	done
 }
@@ -38,6 +46,7 @@ test() {
 	local create_pid
 	local detach_pid
 	local image_file="$TMPDIR/loopImg"
+	local grep_str
 
 	truncate --size 1G "${image_file}"
 	parted --align none --script "${image_file}" mklabel gpt
@@ -53,9 +62,9 @@ test() {
 	mkfs.xfs --force "${loop_device}p1" >& /dev/null
 	losetup --detach "${loop_device}" >&  /dev/null
 
-	create_loop &
+	create_loop "${loop_device}" &
 	create_pid=$!
-	detach_loop &
+	detach_loop "${loop_device}" &
 	detach_pid=$!
 
 	sleep "${TIMEOUT:-90}"
@@ -66,8 +75,9 @@ test() {
 		sleep 1
 	} 2>/dev/null
 
-	losetup --detach-all >& /dev/null
-	if _dmesg_since_test_start | grep --quiet "partition scan of loop0 failed (rc=-16)"; then
+	losetup --detach "${loop_device}" >& /dev/null
+	grep_str="partition scan of ${loop_device##*/} failed (rc=-16)"
+	if _dmesg_since_test_start | grep --quiet "$grep_str"; then
 		echo "Fail"
 	fi
 	echo "Test complete"
-- 
2.45.0

Re: [PATCH blktests] loop/010: do not assume /dev/loop0

[Chaitanya Kulkarni]

On 6/20/24 03:41, Shin'ichiro Kawasaki wrote:
> The current implementation of the test case loop/010 assumes that the
> prepared loop device is /dev/loop0, which is not always true. When other
> loop devices are set up before the test case run, the assumption is
> wrong and the test case fails.
>
> To avoid the failure, use the prepared loop device name stored in
> $loop_device instead of /dev/loop0. Adjust the grep string to meet the
> device name. Also use "losetup --detach" instead of
> "losetup --detach-all" to not detach the loop devices which existed
> before the test case runs.
>
> Fixes: 1c4ae4fed9b4 ("loop: Detect a race condition between loop detach and open")
> Signed-off-by: Shin'ichiro Kawasaki<shinichiro.kawasaki@wdc.com>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

RE: [PATCH blktests] loop/010: do not assume /dev/loop0

[Gulam Mohamed]

Hi Shinichiro,

> -----Original Message-----
> From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Sent: Thursday, June 20, 2024 4:12 PM
> To: linux-block@vger.kernel.org
> Cc: Gulam Mohamed <gulam.mohamed@oracle.com>; Shin'ichiro Kawasaki
> <shinichiro.kawasaki@wdc.com>
> Subject: [PATCH blktests] loop/010: do not assume /dev/loop0
> 
> The current implementation of the test case loop/010 assumes that the
> prepared loop device is /dev/loop0, which is not always true. When other
> loop devices are set up before the test case run, the assumption is wrong and
> the test case fails.
> 
> To avoid the failure, use the prepared loop device name stored in
> $loop_device instead of /dev/loop0. Adjust the grep string to meet the device
> name. Also use "losetup --detach" instead of "losetup --detach-all" to not
> detach the loop devices which existed before the test case runs.
> 
> Fixes: 1c4ae4fed9b4 ("loop: Detect a race condition between loop detach and
> open")
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  tests/loop/010 | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/loop/010 b/tests/loop/010 index ea396ec..f8c6f2c 100755
> --- a/tests/loop/010
> +++ b/tests/loop/010
> @@ -16,18 +16,26 @@ requires() {
>  }
> 
>  create_loop() {
> +	local dev
> +
>  	while true
>  	do
> -		loop_device="$(losetup --partscan --find --show
> "${image_file}")"
> -		blkid /dev/loop0p1 >& /dev/null
> +		dev="$(losetup --partscan --find --show "${image_file}")"
> +		if [[ $dev != "$1" ]]; then
> +			echo "Unepxected loop device set up: $dev"
> +			return
> +		fi
> +		blkid "$dev" >& /dev/null
>  	done
>  }
> 
>  detach_loop() {
> +	local dev=$1
> +
>  	while true
>  	do
> -		if [ -e /dev/loop0 ]; then
> -			losetup --detach /dev/loop0 >& /dev/null
> +		if [[ -e "$dev" ]]; then
> +			losetup --detach "$dev" >& /dev/null
>  		fi
>  	done
>  }
> @@ -38,6 +46,7 @@ test() {
>  	local create_pid
>  	local detach_pid
>  	local image_file="$TMPDIR/loopImg"
> +	local grep_str
> 
>  	truncate --size 1G "${image_file}"
>  	parted --align none --script "${image_file}" mklabel gpt @@ -53,9
> +62,9 @@ test() {
>  	mkfs.xfs --force "${loop_device}p1" >& /dev/null
>  	losetup --detach "${loop_device}" >&  /dev/null
> 
> -	create_loop &
> +	create_loop "${loop_device}" &
>  	create_pid=$!
> -	detach_loop &
> +	detach_loop "${loop_device}" &
>  	detach_pid=$!
> 
>  	sleep "${TIMEOUT:-90}"
> @@ -66,8 +75,9 @@ test() {
>  		sleep 1
>  	} 2>/dev/null
> 
> -	losetup --detach-all >& /dev/null
> -	if _dmesg_since_test_start | grep --quiet "partition scan of loop0
> failed (rc=-16)"; then
> +	losetup --detach "${loop_device}" >& /dev/null
> +	grep_str="partition scan of ${loop_device##*/} failed (rc=-16)"
> +	if _dmesg_since_test_start | grep --quiet "$grep_str"; then
>  		echo "Fail"
>  	fi
>  	echo "Test complete"
> --
> 2.45.0

Thanks for working on improving this test case. I tried to test this but I am getting the following errors:

     Running loop/010
    +Unepxected loop device set up: /dev/loop1
     Test complete

This error is from the following "if" condition in function create_loop():

	if [[ $dev != "$1" ]]; then
                        echo "Unepxected loop device set up: $dev"
                        return
                fi

I was trying to understand the reason for this "if" condition. Without this "if" check, its working fine (With kernel fix the test case passes and without kernel fix, the test case fails which is expected).

Re: [PATCH blktests] loop/010: do not assume /dev/loop0

[Shinichiro Kawasaki]

On Jun 24, 2024 / 05:25, Gulam Mohamed wrote:
> Hi Shinichiro,
> 
> > -----Original Message-----
> > From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > Sent: Thursday, June 20, 2024 4:12 PM
> > To: linux-block@vger.kernel.org
> > Cc: Gulam Mohamed <gulam.mohamed@oracle.com>; Shin'ichiro Kawasaki
> > <shinichiro.kawasaki@wdc.com>
> > Subject: [PATCH blktests] loop/010: do not assume /dev/loop0
> > 
> > The current implementation of the test case loop/010 assumes that the
> > prepared loop device is /dev/loop0, which is not always true. When other
> > loop devices are set up before the test case run, the assumption is wrong and
> > the test case fails.
> > 
> > To avoid the failure, use the prepared loop device name stored in
> > $loop_device instead of /dev/loop0. Adjust the grep string to meet the device
> > name. Also use "losetup --detach" instead of "losetup --detach-all" to not
> > detach the loop devices which existed before the test case runs.
> > 
> > Fixes: 1c4ae4fed9b4 ("loop: Detect a race condition between loop detach and
> > open")
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> >  tests/loop/010 | 26 ++++++++++++++++++--------
> >  1 file changed, 18 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tests/loop/010 b/tests/loop/010 index ea396ec..f8c6f2c 100755
> > --- a/tests/loop/010
> > +++ b/tests/loop/010
> > @@ -16,18 +16,26 @@ requires() {
> >  }
> > 
> >  create_loop() {
> > +	local dev
> > +
> >  	while true
> >  	do
> > -		loop_device="$(losetup --partscan --find --show
> > "${image_file}")"
> > -		blkid /dev/loop0p1 >& /dev/null
> > +		dev="$(losetup --partscan --find --show "${image_file}")"
> > +		if [[ $dev != "$1" ]]; then
> > +			echo "Unepxected loop device set up: $dev"
> > +			return
> > +		fi
> > +		blkid "$dev" >& /dev/null
> >  	done
> >  }
> > 
> >  detach_loop() {
> > +	local dev=$1
> > +
> >  	while true
> >  	do
> > -		if [ -e /dev/loop0 ]; then
> > -			losetup --detach /dev/loop0 >& /dev/null
> > +		if [[ -e "$dev" ]]; then
> > +			losetup --detach "$dev" >& /dev/null
> >  		fi
> >  	done
> >  }
> > @@ -38,6 +46,7 @@ test() {
> >  	local create_pid
> >  	local detach_pid
> >  	local image_file="$TMPDIR/loopImg"
> > +	local grep_str
> > 
> >  	truncate --size 1G "${image_file}"
> >  	parted --align none --script "${image_file}" mklabel gpt @@ -53,9
> > +62,9 @@ test() {
> >  	mkfs.xfs --force "${loop_device}p1" >& /dev/null
> >  	losetup --detach "${loop_device}" >&  /dev/null
> > 
> > -	create_loop &
> > +	create_loop "${loop_device}" &
> >  	create_pid=$!
> > -	detach_loop &
> > +	detach_loop "${loop_device}" &
> >  	detach_pid=$!
> > 
> >  	sleep "${TIMEOUT:-90}"
> > @@ -66,8 +75,9 @@ test() {
> >  		sleep 1
> >  	} 2>/dev/null
> > 
> > -	losetup --detach-all >& /dev/null
> > -	if _dmesg_since_test_start | grep --quiet "partition scan of loop0
> > failed (rc=-16)"; then
> > +	losetup --detach "${loop_device}" >& /dev/null
> > +	grep_str="partition scan of ${loop_device##*/} failed (rc=-16)"
> > +	if _dmesg_since_test_start | grep --quiet "$grep_str"; then
> >  		echo "Fail"
> >  	fi
> >  	echo "Test complete"
> > --
> > 2.45.0
> 
> Thanks for working on improving this test case. I tried to test this but I am getting the following errors:
> 
>      Running loop/010
>     +Unepxected loop device set up: /dev/loop1
>      Test complete
> 
> This error is from the following "if" condition in function create_loop():
> 
> 	if [[ $dev != "$1" ]]; then
>                         echo "Unepxected loop device set up: $dev"
>                         return
>                 fi
> 
> I was trying to understand the reason for this "if" condition. Without this "if" check, its working fine (With kernel fix the test case passes and without kernel fix, the test case fails which is expected).

Hi Gulam, I added the if statement to check that create_loop() always creates
the same loop device. My patch replaced "lopsetup --detach-all" with
"losetup --detache ${loop_device}", then if create_loop() creates loop devices
different from ${loop_device}, the test case will leave those devices. This is
not good.

But opposed to my expectation, create_loop() creates some different loop
devices. I guess in your system, /dev/loop0 is created in most cases. But
sometimes /dev/loop1 is created. Then the if block was executed. I repeated
the test case run on my system, and observed the same symptom as yours.

To not create loop devices other than ${loop_device}, I came up with another
solution idea. I think the losetup --find option is not needed. Just specifying
${loop_device} instead of --find will work. I tried this idea on my system and
it looks working. Will post v2 with this fix.

Re: [PATCH blktests] loop/010: do not assume /dev/loop0

[Shinichiro Kawasaki]

On Jun 20, 2024 / 19:41, Shin'ichiro Kawasaki wrote:
> The current implementation of the test case loop/010 assumes that the
> prepared loop device is /dev/loop0, which is not always true. When other
> loop devices are set up before the test case run, the assumption is
> wrong and the test case fails.
> 
> To avoid the failure, use the prepared loop device name stored in
> $loop_device instead of /dev/loop0. Adjust the grep string to meet the
> device name. Also use "losetup --detach" instead of
> "losetup --detach-all" to not detach the loop devices which existed
> before the test case runs.
> 
> Fixes: 1c4ae4fed9b4 ("loop: Detect a race condition between loop detach and open")
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  tests/loop/010 | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/loop/010 b/tests/loop/010
> index ea396ec..f8c6f2c 100755
> --- a/tests/loop/010
> +++ b/tests/loop/010
> @@ -16,18 +16,26 @@ requires() {
>  }
>  
>  create_loop() {
> +	local dev
> +
>  	while true
>  	do
> -		loop_device="$(losetup --partscan --find --show "${image_file}")"
> -		blkid /dev/loop0p1 >& /dev/null
> +		dev="$(losetup --partscan --find --show "${image_file}")"
> +		if [[ $dev != "$1" ]]; then
> +			echo "Unepxected loop device set up: $dev"
> +			return
> +		fi
> +		blkid "$dev" >& /dev/null

To correspond to "/dev/loop0p1", the "$dev" above should be "$dev"p1.
I forgot to add "p1" and this missing p1 looks slightly lowers the failure
detection ratio. Will fix it in v2.