block loopback driver possible regression since next-20220211

block loopback driver possible regression since next-20220211

[Luis Chamberlain]

I noticed that since next-20220211 losetup fails at something stupid
simple:

losetup $LOOPDEV $DISK

I can't see how the changes on drivers/block/loop.c would cause this,
I even tried to revert what I thought would be the only commit which
would seem to do a functional change "loop: revert "make autoclear
operation asynchronous" but that didn't fix it.

I proceeded to bisecting... but I did this on today's linux-next,
and well today's linux-next is hosed even at boot. My bisection then
was completley inconclusive since linux-next is pure poop today.

Any ideas though?

Fortunately Linus' tree is fine.

I'm quit afraid that we wouldn't have caught this issue. Seems pretty
straight forward. It would seem we don't have such a basic thing on
blktests, so I'll go add that...

  Luis

Re: block loopback driver possible regression since next-20220211

[Luis Chamberlain]

On Fri, Feb 18, 2022 at 7:10 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> I noticed that since next-20220211 losetup fails at something stupid
> simple:
>
> losetup $LOOPDEV $DISK

I can bisect through linux-next, next-20220201 works. I'll let you
know what I find.

 Luis

Re: block loopback driver possible regression since next-20220211

[Jens Axboe]

On 2/18/22 8:10 PM, Luis Chamberlain wrote:
> I noticed that since next-20220211 losetup fails at something stupid
> simple:
> 
> losetup $LOOPDEV $DISK
> 
> I can't see how the changes on drivers/block/loop.c would cause this,
> I even tried to revert what I thought would be the only commit which
> would seem to do a functional change "loop: revert "make autoclear
> operation asynchronous" but that didn't fix it.
> 
> I proceeded to bisecting... but I did this on today's linux-next,
> and well today's linux-next is hosed even at boot. My bisection then
> was completley inconclusive since linux-next is pure poop today.
> 
> Any ideas though?
> 
> Fortunately Linus' tree is fine.
> 
> I'm quit afraid that we wouldn't have caught this issue. Seems pretty
> straight forward. It would seem we don't have such a basic thing on
> blktests, so I'll go add that...

My guess would be that it's:

commit fbdee71bb5d8d054e1bdb5af4c540f2cb86fe296
Author: Christoph Hellwig <hch@lst.de>
Date:   Tue Jan 4 08:16:47 2022 +0100

    block: deprecate autoloading based on dev_t

-- 
Jens Axboe

Re: block loopback driver possible regression since next-20220211

[Luis Chamberlain]

On Sat, Feb 19, 2022 at 09:18:58AM -0700, Jens Axboe wrote:
> On 2/18/22 8:10 PM, Luis Chamberlain wrote:
> > I noticed that since next-20220211 losetup fails at something stupid
> > simple:
> > 
> > losetup $LOOPDEV $DISK
> > 
> > I can't see how the changes on drivers/block/loop.c would cause this,
> > I even tried to revert what I thought would be the only commit which
> > would seem to do a functional change "loop: revert "make autoclear
> > operation asynchronous" but that didn't fix it.
> > 
> > I proceeded to bisecting... but I did this on today's linux-next,
> > and well today's linux-next is hosed even at boot. My bisection then
> > was completley inconclusive since linux-next is pure poop today.
> > 
> > Any ideas though?
> > 
> > Fortunately Linus' tree is fine.
> > 
> > I'm quit afraid that we wouldn't have caught this issue. Seems pretty
> > straight forward. It would seem we don't have such a basic thing on
> > blktests, so I'll go add that...
> 
> My guess would be that it's:
> 
> commit fbdee71bb5d8d054e1bdb5af4c540f2cb86fe296
> Author: Christoph Hellwig <hch@lst.de>
> Date:   Tue Jan 4 08:16:47 2022 +0100
> 
>     block: deprecate autoloading based on dev_t
> 

Indeed. The issue is that dropping that also does not allow the
association of extra custom higher number loop block nodes created manually
even if you *do* load a respective module before use. That is by design
by the commit, since we're stuffing the nasty old probe logic now under the new
CONFIG_BLOCK_LEGACY_AUTOLOAD. Subtle difference, but same deprecation
effect.

I agree with the approach to stuff all this nasty cruft under
BLOCK_LEGACY_AUTOLOAD however I suspect v5.19 might be too soon to tell if we
can nuke it safely by then though.

I'd go so far as to say that we should sadly make
BLOCK_LEGACY_AUTOLOAD=y for a while before going with an axe to kill it.
I think we have a few hidden gems we'll soon discover might need a bit
more time to adjust.

FWIW below is a simple test, which now fails to explain what I mean with
the above.

root@kdevops ~ # cat loop-high-devs.sh 
#!/bin/bash

NUM="8"
LOOPDEV="/dev/loop${NUM}"

modprobe loop
sleep 2

if [[ ! -e $LOOPDEV ]]; then
	mknod $LOOPDEV b 7 $NUM
fi

losetup -d $LOOPDEV 2>/dev/null

DISK="test_loop_${NUM}.img"
rm -f $DISK
truncate -s 10M $DISK
losetup $LOOPDEV $DISK
if [ $? -eq 0 ]; then
	echo "$LOOPDEV ready"
else
	echo "$LOOPDEV failed"
fi

  Luis

Re: block loopback driver possible regression since next-20220211

[Chaitanya Kulkarni]

On 2/19/22 11:05 AM, Luis Chamberlain wrote:
> On Sat, Feb 19, 2022 at 09:18:58AM -0700, Jens Axboe wrote:
>> On 2/18/22 8:10 PM, Luis Chamberlain wrote:
>>> I noticed that since next-20220211 losetup fails at something stupid
>>> simple:
>>>
>>> losetup $LOOPDEV $DISK
>>>
>>> I can't see how the changes on drivers/block/loop.c would cause this,
>>> I even tried to revert what I thought would be the only commit which
>>> would seem to do a functional change "loop: revert "make autoclear
>>> operation asynchronous" but that didn't fix it.
>>>
>>> I proceeded to bisecting... but I did this on today's linux-next,
>>> and well today's linux-next is hosed even at boot. My bisection then
>>> was completley inconclusive since linux-next is pure poop today.
>>>
>>> Any ideas though?
>>>
>>> Fortunately Linus' tree is fine.
>>>
>>> I'm quit afraid that we wouldn't have caught this issue. Seems pretty
>>> straight forward. It would seem we don't have such a basic thing on
>>> blktests, so I'll go add that...
>>
>> My guess would be that it's:
>>
>> commit fbdee71bb5d8d054e1bdb5af4c540f2cb86fe296
>> Author: Christoph Hellwig <hch@lst.de>
>> Date:   Tue Jan 4 08:16:47 2022 +0100
>>
>>      block: deprecate autoloading based on dev_t
>>
> 
> Indeed. The issue is that dropping that also does not allow the
> association of extra custom higher number loop block nodes created manually
> even if you *do* load a respective module before use. That is by design
> by the commit, since we're stuffing the nasty old probe logic now under the new
> CONFIG_BLOCK_LEGACY_AUTOLOAD. Subtle difference, but same deprecation
> effect.
> 
> I agree with the approach to stuff all this nasty cruft under
> BLOCK_LEGACY_AUTOLOAD however I suspect v5.19 might be too soon to tell if we
> can nuke it safely by then though.
> 
> I'd go so far as to say that we should sadly make
> BLOCK_LEGACY_AUTOLOAD=y for a while before going with an axe to kill it.
> I think we have a few hidden gems we'll soon discover might need a bit
> more time to adjust.
> 
> FWIW below is a simple test, which now fails to explain what I mean with
> the above.
> 
> root@kdevops ~ # cat loop-high-devs.sh
> #!/bin/bash
> 
> NUM="8"
> LOOPDEV="/dev/loop${NUM}"
> 
> modprobe loop
> sleep 2
> 
> if [[ ! -e $LOOPDEV ]]; then
> 	mknod $LOOPDEV b 7 $NUM
> fi
> 
> losetup -d $LOOPDEV 2>/dev/null
> 
> DISK="test_loop_${NUM}.img"
> rm -f $DISK
> truncate -s 10M $DISK
> losetup $LOOPDEV $DISK
> if [ $? -eq 0 ]; then
> 	echo "$LOOPDEV ready"
> else
> 	echo "$LOOPDEV failed"
> fi
> 
>    Luis
> 


thanks a log for bisecting, I'll review if you send out blktests for
loop, it is been on my todo list for a while, I can also send you few
scripts that I've written on the top of your list and we can add them in
the final series...

-ck

Re: block loopback driver possible regression since next-20220211

[Christoph Hellwig]

On Sat, Feb 19, 2022 at 11:05:23AM -0800, Luis Chamberlain wrote:
> Indeed. The issue is that dropping that also does not allow the
> association of extra custom higher number loop block nodes created manually
> even if you *do* load a respective module before use. That is by design
> by the commit, since we're stuffing the nasty old probe logic now under the new
> CONFIG_BLOCK_LEGACY_AUTOLOAD. Subtle difference, but same deprecation
> effect.
> 
> I agree with the approach to stuff all this nasty cruft under
> BLOCK_LEGACY_AUTOLOAD however I suspect v5.19 might be too soon to tell if we
> can nuke it safely by then though.

Yeah.  5.19 was planned when I submitted this for 5.17, but with it
appearing in 5.18 it is far too early anyway.

> I'd go so far as to say that we should sadly make
> BLOCK_LEGACY_AUTOLOAD=y for a while before going with an axe to kill it.
> I think we have a few hidden gems we'll soon discover might need a bit
> more time to adjust.

Probably.

> 
> FWIW below is a simple test, which now fails to explain what I mean with
> the above.
> 
> root@kdevops ~ # cat loop-high-devs.sh 

The interesting part is that the script works if I remove this mknod:

> if [[ ! -e $LOOPDEV ]]; then
> 	mknod $LOOPDEV b 7 $NUM
> fi

so it seems like losetup is trying to be smart here and skip the manual
creation if the device exists.  I'll take a look at the code and wіll
prepare a patch for that.