[PATCH] common/rc: destroy loop dev before fallback recreation

[PATCH] common/rc: destroy loop dev before fallback recreation

[Brian Foster]

When running fstests on an s390x box I observed failure to unmount
filesystem errors due to stale loop devices being left around. This
root caused down to generic/361 leaving around an attached loop0
device. On further inspection, the test actually created two loop
devices (loop0 and loop1), and executed on and cleaned up the
latter.

The origin of the former appears to be that the initial losetup
command in _create_loop_device() fails due to $dio_args in this
environment, but still creates the loop device. For example:

 # losetup --direct-io=on -f --show /mnt/scratch/fs.img
 /dev/loop0
 losetup: /dev/loop0: set direct io failed: Invalid argument
 # losetup -a
 /dev/loop0: [64771]:131 (/mnt/scratch/fs.img)

The helper then goes on to create loop1, but it or the test never
deals with loop0. To avoid this problem, detach any old loop device
if one was set up before the fallback losetup command.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---

This appears to be fallout from recent commit aa14b84a8d1a2 ("xfs/259:
try to force loop device block size"). I'm not really sure why losetup
creates the device with bad dio settings but not with block size. Maybe
it's more of a dynamic setting or whatever and that's why this was
previously a separate losetup command..? Anyways, this seems to work for
me..

Brian

 common/rc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/rc b/common/rc
index 81587dad..891f6b7e 100644
--- a/common/rc
+++ b/common/rc
@@ -4596,6 +4596,7 @@ _create_loop_device()
 	# size to the directio alignment of the underlying fs, so if we want to
 	# use our own sector size, we need to specify that at creation time.
 	if ! dev="$(losetup $dio_args $args -f --show $file 2>/dev/null)"; then
+		test -n "$dev" && losetup -d "$dev" > /dev/null 2>&1
 		dev="$(losetup $args -f --show $file)" || \
 			_fail "Cannot assign $file to a loop device ($args)"
 	fi
-- 
2.51.0

Re: [PATCH] common/rc: destroy loop dev before fallback recreation

[Darrick J. Wong]

On Wed, Sep 24, 2025 at 02:12:35PM -0400, Brian Foster wrote:
> When running fstests on an s390x box I observed failure to unmount
> filesystem errors due to stale loop devices being left around. This
> root caused down to generic/361 leaving around an attached loop0
> device. On further inspection, the test actually created two loop
> devices (loop0 and loop1), and executed on and cleaned up the
> latter.
> 
> The origin of the former appears to be that the initial losetup
> command in _create_loop_device() fails due to $dio_args in this
> environment, but still creates the loop device. For example:
> 
>  # losetup --direct-io=on -f --show /mnt/scratch/fs.img
>  /dev/loop0
>  losetup: /dev/loop0: set direct io failed: Invalid argument

Egad following the argument parsing in losetup is awful.  I had thought
that the -f would set act == ACT_FIND_FREE which would then set up the
loop device with one configure call, but the error message clearly
indicates that we're failing here:

	case A_SET_DIRECT_IO:
		res = loopcxt_ioctl_dio(&lc, use_dio);
		if (res)
			warn(_("%s: set direct io failed"),
			        loopcxt_get_device(&lc));
		break;

In this case, we clearly don't tear down the loop device after this
failure, so yes, you've found a bug.  losetup can totally create a loop
device, fail to configure it, and return EXIT_FAILURE without tearing
down that loop device.

>  # losetup -a
>  /dev/loop0: [64771]:131 (/mnt/scratch/fs.img)
> 
> The helper then goes on to create loop1, but it or the test never
> deals with loop0. To avoid this problem, detach any old loop device
> if one was set up before the fallback losetup command.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> 
> This appears to be fallout from recent commit aa14b84a8d1a2 ("xfs/259:
> try to force loop device block size"). I'm not really sure why losetup
> creates the device with bad dio settings but not with block size. Maybe
> it's more of a dynamic setting or whatever and that's why this was
> previously a separate losetup command..? Anyways, this seems to work for
> me..

It probably has to do with the underlying fs not supporting directio or
something.  What fstype is /mnt/scratch, and which kernel version?

> Brian
> 
>  common/rc | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/common/rc b/common/rc
> index 81587dad..891f6b7e 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4596,6 +4596,7 @@ _create_loop_device()
>  	# size to the directio alignment of the underlying fs, so if we want to
>  	# use our own sector size, we need to specify that at creation time.
>  	if ! dev="$(losetup $dio_args $args -f --show $file 2>/dev/null)"; then
> +		test -n "$dev" && losetup -d "$dev" > /dev/null 2>&1

The logic looks sound, but I think there ought to be a comment
explicitly documenting this behavior of losetup:

		# losetup can create a loop device, fail to configure
		# it, and return EXIT_FAILURE without tearing down that
		# loop device.
		test -n "$dev" && losetup -d "$dev" &>/dev/null

Because I won't remember this subtlety 3 months from now. :(

--D

>  		dev="$(losetup $args -f --show $file)" || \
>  			_fail "Cannot assign $file to a loop device ($args)"
>  	fi
> -- 
> 2.51.0
> 
> 

Re: [PATCH] common/rc: destroy loop dev before fallback recreation

[Brian Foster]

On Thu, Sep 25, 2025 at 09:50:46AM -0700, Darrick J. Wong wrote:
> On Wed, Sep 24, 2025 at 02:12:35PM -0400, Brian Foster wrote:
> > When running fstests on an s390x box I observed failure to unmount
> > filesystem errors due to stale loop devices being left around. This
> > root caused down to generic/361 leaving around an attached loop0
> > device. On further inspection, the test actually created two loop
> > devices (loop0 and loop1), and executed on and cleaned up the
> > latter.
> > 
> > The origin of the former appears to be that the initial losetup
> > command in _create_loop_device() fails due to $dio_args in this
> > environment, but still creates the loop device. For example:
> > 
> >  # losetup --direct-io=on -f --show /mnt/scratch/fs.img
> >  /dev/loop0
> >  losetup: /dev/loop0: set direct io failed: Invalid argument
> 
> Egad following the argument parsing in losetup is awful.  I had thought
> that the -f would set act == ACT_FIND_FREE which would then set up the
> loop device with one configure call, but the error message clearly
> indicates that we're failing here:
> 
> 	case A_SET_DIRECT_IO:
> 		res = loopcxt_ioctl_dio(&lc, use_dio);
> 		if (res)
> 			warn(_("%s: set direct io failed"),
> 			        loopcxt_get_device(&lc));
> 		break;
> 
> In this case, we clearly don't tear down the loop device after this
> failure, so yes, you've found a bug.  losetup can totally create a loop
> device, fail to configure it, and return EXIT_FAILURE without tearing
> down that loop device.
> 

Figured it was something like that after seeing how this was previously
a separate losetup command. I would have thought the same around error
handling, fwiw.

> >  # losetup -a
> >  /dev/loop0: [64771]:131 (/mnt/scratch/fs.img)
> > 
> > The helper then goes on to create loop1, but it or the test never
> > deals with loop0. To avoid this problem, detach any old loop device
> > if one was set up before the fallback losetup command.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> > 
> > This appears to be fallout from recent commit aa14b84a8d1a2 ("xfs/259:
> > try to force loop device block size"). I'm not really sure why losetup
> > creates the device with bad dio settings but not with block size. Maybe
> > it's more of a dynamic setting or whatever and that's why this was
> > previously a separate losetup command..? Anyways, this seems to work for
> > me..
> 
> It probably has to do with the underlying fs not supporting directio or
> something.  What fstype is /mnt/scratch, and which kernel version?

Eh, this was XFS IIRC on a custom RHEL kernel on s390x. I didn't take it
as far as testing upstream and/or digging further because it clearly
toggled on the recent losetup change.

> 
> > Brian
> > 
> >  common/rc | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/common/rc b/common/rc
> > index 81587dad..891f6b7e 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -4596,6 +4596,7 @@ _create_loop_device()
> >  	# size to the directio alignment of the underlying fs, so if we want to
> >  	# use our own sector size, we need to specify that at creation time.
> >  	if ! dev="$(losetup $dio_args $args -f --show $file 2>/dev/null)"; then
> > +		test -n "$dev" && losetup -d "$dev" > /dev/null 2>&1
> 
> The logic looks sound, but I think there ought to be a comment
> explicitly documenting this behavior of losetup:
> 
> 		# losetup can create a loop device, fail to configure
> 		# it, and return EXIT_FAILURE without tearing down that
> 		# loop device.
> 		test -n "$dev" && losetup -d "$dev" &>/dev/null
> 
> Because I won't remember this subtlety 3 months from now. :(
> 

Sure, I'll add something like that. Thanks.

Brian

> --D
> 
> >  		dev="$(losetup $args -f --show $file)" || \
> >  			_fail "Cannot assign $file to a loop device ($args)"
> >  	fi
> > -- 
> > 2.51.0
> > 
> > 
>