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

[Brian Foster <bfoster@redhat.com>]

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