Re: bug in 2.6.22-rc2: loop mount limited to one single iso image

On Sunday 20 May 2007, Uwe Bugla wrote:
Am Samstag, 19. Mai 2007 21:17 schrieben Sie:
Ray Lee wrote:
Hey there,

On 5/19/07, Uwe Bugla <uwe.bugla@xxxxxx> wrote:
> I am running Debian Etch 4.0 stable in connection with udev.
>
> In kernel 2.6.22-rc2 the number of available loop mounts is reduced
> from 8 (conventional standard of preceding 2.6 kernels) to 1.
>
> Symptom: If you try to mount more than one single iso-image with
> the loop parameter you are receiving the following message during
> system boot:
>
> "mount: could not find any free loop device"
>
> Kernel 2.6.20.11 does not show that problem.
>
> Question: Can someone reading this confirm and reproduce that
> problem?

I can reproduce the problem pretty trivially with:

mkdir a m1 m2
touch a/1
genisoimage -o cd1.iso a || mkisofs -o cd1.iso a
cp cd1.iso cd2.iso
sudo mount -o loop cd1.iso m1
sudo mount -o loop cd2.iso m2

...and the last mount fails. That used to work, at least as of 2.6.15,
which is the only other 2.6 kernel I have running on a machine that I
can log into at the moment.

This is unfortunate case of user-space breakage.

Now when we do not have specific loop device limit, we also do not
pre-register loop devices:

{pts/0}% LC_ALL=C ll /dev/loop*
brw-rw---- 1 root disk 7, 0 May 19 20:33 /dev/loop0

(I wonder where loop0 comes from at all :) but losetup & Co unfortunately
need preexisting device node :(

Trivial workaround for those who need it now - wrapper that creates node
and calls real losetup. I am not sure this actually works with loop mount
though.

Real fix is to add special control node and change losetup to use it
instead (or in addition) to opening /dev/loop%d. But as it stands now I'd
call it big regression.

-andrey

Looking at the changelogs between 2.6.20 and current tip shows at least
three significant patches to loop.c. The first was supposedly tested
heavily, so we'll leave that alone for now. (They all were applied
after 2.6.21 was released, so it's probably fine.)

Anyway, could you try reverting the two patches below (in order: the
first, then the second) and see if that fixes it? The second one looks
iffy to me, but if this fixes it then I'm sure Al can sort out why.

Ray



commit 705962ccc9d21a08b74b6b6e1d3cf10f98968a67
Author: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Date: Sun May 13 05:52:32 2007 -0400

fix deadlock in loop.c

... doh

Jeremy Fitzhardinge noted that the recent loop.c cleanups worked,
but cause lockdep to complain.

Ouch. OK, the deadlock is real and yes, I'm an idiot. Speaking
of which, we probably want to s/lock/pin/ in drivers/base/map.c to
avoid such
brainos again. And yes, this stuff needs clear documentation.
Will try to put one together once I get some sleep...

Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Cc: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index e2fc4b6..5526ead 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1399,6 +1399,11 @@ static struct loop_device *loop_init_one(int i)
struct loop_device *lo;
struct gendisk *disk;

+ list_for_each_entry(lo, &loop_devices, lo_list) {
+ if (lo->lo_number == i)
+ return lo;
+ }
+
lo = kzalloc(sizeof(*lo), GFP_KERNEL);
if (!lo)
goto out;
@@ -1443,17 +1448,13 @@ static void loop_del_one(struct loop_device
*lo) kfree(lo);
}

-static int loop_lock(dev_t dev, void *data)
-{
- mutex_lock(&loop_devices_mutex);
- return 0;
-}
-
static struct kobject *loop_probe(dev_t dev, int *part, void *data)
{
- struct loop_device *lo = loop_init_one(dev & MINORMASK);
+ struct loop_device *lo;
struct kobject *kobj;

+ mutex_lock(&loop_devices_mutex);
+ lo = loop_init_one(dev & MINORMASK);
kobj = lo ? get_disk(lo->lo_disk) : ERR_PTR(-ENOMEM);
mutex_unlock(&loop_devices_mutex);

@@ -1466,7 +1467,7 @@ static int __init loop_init(void)
if (register_blkdev(LOOP_MAJOR, "loop"))
return -EIO;
blk_register_region(MKDEV(LOOP_MAJOR, 0), 1UL << MINORBITS,
- THIS_MODULE, loop_probe, loop_lock,
NULL); + THIS_MODULE, loop_probe, NULL,
NULL);

if (max_loop) {
printk(KERN_INFO "loop: the max_loop option is obsolete "





commit 07002e995638b83a6987180f43722a0eb39d4932
Author: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Date: Sat May 12 16:23:15 2007 -0400

fix the dynamic allocation and probe in loop.c

Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Acked-by: Ken Chen <kenchen@xxxxxxxxxx>
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 18cdd8c..e2fc4b6 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1317,18 +1317,6 @@ static long lo_compat_ioctl(struct file *file,
unsigned int cmd, unsigned long a
}
#endif

-static struct loop_device *loop_find_dev(int number)
-{
- struct loop_device *lo;
-
- list_for_each_entry(lo, &loop_devices, lo_list) {
- if (lo->lo_number == number)
- return lo;
- }
- return NULL;
-}
-
-static struct loop_device *loop_init_one(int i);
static int lo_open(struct inode *inode, struct file *file)
{
struct loop_device *lo = inode->i_bdev->bd_disk->private_data;
@@ -1337,11 +1325,6 @@ static int lo_open(struct inode *inode, struct
file *file)
lo->lo_refcnt++;
mutex_unlock(&lo->lo_ctl_mutex);

- mutex_lock(&loop_devices_mutex);
- if (!loop_find_dev(lo->lo_number + 1))
- loop_init_one(lo->lo_number + 1);
- mutex_unlock(&loop_devices_mutex);
-
return 0;
}

@@ -1448,7 +1431,7 @@ out_free_queue:
out_free_dev:
kfree(lo);
out:
- return ERR_PTR(-ENOMEM);
+ return NULL;
}

static void loop_del_one(struct loop_device *lo)
@@ -1460,36 +1443,30 @@ static void loop_del_one(struct loop_device
*lo) kfree(lo);
}

+static int loop_lock(dev_t dev, void *data)
+{
+ mutex_lock(&loop_devices_mutex);
+ return 0;
+}
+
static struct kobject *loop_probe(dev_t dev, int *part, void *data)
{
- unsigned int number = dev & MINORMASK;
- struct loop_device *lo;
+ struct loop_device *lo = loop_init_one(dev & MINORMASK);
+ struct kobject *kobj;

- mutex_lock(&loop_devices_mutex);
- lo = loop_find_dev(number);
- if (lo == NULL)
- lo = loop_init_one(number);
+ kobj = lo ? get_disk(lo->lo_disk) : ERR_PTR(-ENOMEM);
mutex_unlock(&loop_devices_mutex);

*part = 0;
- if (IS_ERR(lo))
- return (void *)lo;
- else
- return &lo->lo_disk->kobj;
+ return kobj;
}

static int __init loop_init(void)
{
- struct loop_device *lo;
-
if (register_blkdev(LOOP_MAJOR, "loop"))
return -EIO;
blk_register_region(MKDEV(LOOP_MAJOR, 0), 1UL << MINORBITS,
- THIS_MODULE, loop_probe, NULL, NULL);
-
- lo = loop_init_one(0);
- if (IS_ERR(lo))
- goto out;
+ THIS_MODULE, loop_probe, loop_lock,
NULL);

if (max_loop) {
printk(KERN_INFO "loop: the max_loop option is obsolete "
@@ -1498,11 +1475,6 @@ static int __init loop_init(void)
}
printk(KERN_INFO "loop: module loaded\n");
return 0;
-
-out:
- unregister_blkdev(LOOP_MAJOR, "loop");
- printk(KERN_ERR "loop: ran out of memory\n");
- return -ENOMEM;
}

static void __exit loop_exit(void)

Hello Ray, hello Andrey,

First of all, thank you deeply for your contributions / reproduction /
ideas!

unfortunately it does not make any difference if I simply rip out the
changes of patch-2.6.22-rc2 or patch-2.6.21 in connection with patch
2.6.22-rc2 regarding module loop.c.


because they are just followup to the real cause. This is causes by commit

commit 73285082745045bcd64333c1fbaa88f8490f2626
Author: Ken Chen <kenchen@xxxxxxxxxx>
Date: Tue May 8 00:28:20 2007 -0700

remove artificial software max_loop limit


look what it did (abridged):

-static int __init loop_init(void)
[...]
- for (i = 0; i < max_loop; i++) {
- disks[i] = alloc_disk(1);
- if (!disks[i])
- goto out_mem3;
}
-
- for (i = 0; i < max_loop; i++) {
- struct loop_device *lo = &loop_dev[i];
- struct gendisk *disk = disks[i];
-
- memset(lo, 0, sizeof(*lo));
- lo->lo_queue = blk_alloc_queue(GFP_KERNEL);
- if (!lo->lo_queue)
- goto out_mem4;
- mutex_init(&lo->lo_ctl_mutex);
- lo->lo_number = i;
- lo->lo_thread = NULL;
- init_waitqueue_head(&lo->lo_event);
- spin_lock_init(&lo->lo_lock);
- disk->major = LOOP_MAJOR;
- disk->first_minor = i;
- disk->fops = &lo_fops;
- sprintf(disk->disk_name, "loop%d", i);
- disk->private_data = lo;
- disk->queue = lo->lo_queue;
- }
-
- /* We cannot fail after we call this, so another loop!*/
- for (i = 0; i < max_loop; i++)
- add_disk(disks[i]);

So before this commit we got /dev/loop%d up to max_loop when loop was loaded.
After this commit we get nothing (I still wonder wheher lone loop0 comes from
after reboot, because reloading module leaves me without /dev/loop%n
alltogether).

This is a real regression because on udev-enabled system (probably 99% of
distributions now) losetup as available in current util-linux simply stops
working.

-andrey

In all mentioned cases I get nothing but an incompilable kernel 2.6.22-rc2.

As I stated already the mentioned loop-mount-problem does not exist in
Kernel 2.6.20.11, who is, at least for the performance level on my machine,
nothing but a bad compromise.

I hope that Al Viro will supply a solution for this horrible kernel
regression. And I do not think that Al Viro is an idiot - he is just a
human, as all of us are.

Yours sincerely

Uwe


Attachment: pgpXoE54o1IhY.pgp
Description: PGP signature

Relevant Pages
Loading