Re: [patch] remove artificial software max_loop limit

Re: [patch] remove artificial software max_loop limit

[devzero]

>Remove artificial maximum 256 loop device that can be created due to a
>legacy device number limit.  Searching through lkml archive, there are
>several instances where users complained about the artificial limit
>that the loop driver impose.  There is no reason to have such limit.

Hey, i was one of those :)

Nice to see, that it`s solved now, thanks very much!

I never expected this to happen and put all my hope into dm-loop instead.
Did you mind that Bryn m. Reeves from redhat will suffer a serious depression now? ( ->  http://sources.redhat.com/lvm2/wiki/DMLoop ) ;)

regards
roland

_______________________________________________________________
SMS schreiben mit WEB.DE FreeMail - einfach, schnell und
kostenguenstig. Jetzt gleich testen! http://f.web.de/?mc=021192

Re: [patch] remove artificial software max_loop limit

[devzero]

> Well, the point of an upper limit might be to keep loop devices from  
> chewing up too much memory on a system.  IE: To fail allocating more  
> loopdevs before you run OOM and start killing random userspace  
> processes.

ok, sounds reasonable. 

but - not very sure here, but don`t you need to be root for creating loop devices and don`t you have many other ways to chew up too  much memory then, anyway ?


> -----Ursprüngliche Nachricht-----
> Von: Kyle Moffett <mrmacman_g4@mac.com>
> Gesendet: 01.04.07 20:44:04
> An: devzero@web.de
> CC: Ken Chen <kenchen@google.com>, linux-kernel@vger.kernel.org
> Betreff: Re: [patch] remove artificial software max_loop limit


> On Apr 01, 2007, at 14:36:11, devzero@web.de wrote:
> >> Blame on the dual meaning of max_loop that it uses currently: to
> >> initialize a set of loop devices and as a side effect, it also sets
> >> the upper limit.  People are complaining about the former constrain,
> >> isn't it?  Does anyone uses the 2nd meaning of upper limit?
> >>
> >> - Ken
> >
> > what sense would it make to set an upper limit at all?
> >
> > we`re so happy to have none anymore :)
> 
> Well, the point of an upper limit might be to keep loop devices from  
> chewing up too much memory on a system.  IE: To fail allocating more  
> loopdevs before you run OOM and start killing random userspace  
> processes.
> 
> Cheers,
> Kyle Moffett
> 
> 


_______________________________________________________________
SMS schreiben mit WEB.DE FreeMail - einfach, schnell und
kostenguenstig. Jetzt gleich testen! http://f.web.de/?mc=021192

Re: [patch] remove artificial software max_loop limit

[devzero]

>Blame on the dual meaning of max_loop that it uses currently: to
>initialize a set of loop devices and as a side effect, it also sets
>the upper limit.  People are complaining about the former constrain,
>isn't it?  Does anyone uses the 2nd meaning of upper limit?
>
>- Ken

what sense would it make to set an upper limit at all?

we`re so happy to have none anymore :)

i think andrew`s suggestion is just good:

>So if we're worried about not breaking existing setups, we should retain
>this module parameter as a do-nothing thing, maybe with a
>this-is-going-away warning printk, too.


roland



_______________________________________________________________
SMS schreiben mit WEB.DE FreeMail - einfach, schnell und
kostenguenstig. Jetzt gleich testen! http://f.web.de/?mc=021192

Re: [patch] remove artificial software max_loop limit

[Kyle Moffett]

On Apr 01, 2007, at 14:36:11, devzero@web.de wrote:
>> Blame on the dual meaning of max_loop that it uses currently: to
>> initialize a set of loop devices and as a side effect, it also sets
>> the upper limit.  People are complaining about the former constrain,
>> isn't it?  Does anyone uses the 2nd meaning of upper limit?
>>
>> - Ken
>
> what sense would it make to set an upper limit at all?
>
> we`re so happy to have none anymore :)

Well, the point of an upper limit might be to keep loop devices from  
chewing up too much memory on a system.  IE: To fail allocating more  
loopdevs before you run OOM and start killing random userspace  
processes.

Cheers,
Kyle Moffett

Re: [patch] remove artificial software max_loop limit

[devzero]

not sure if this is a real issue and if it`s UML or loop related -  but how is low-memory situations being handled when creating loop devices ?

should losetup or dmesg tell "out of memory" if there is not enough memory left ?

i fired up my 2.6.20 UML and tried to create lots of loop-devices.

this crashed my UML very soon , just around 200 devices - then i saw my "mistake" that my UML had only 32MB of RAM.

then i gave my UML mem=256M and now i can setup many more loop-devices, but still crashes in the end:

setting up loop-device 1962 with losetup
Kernel panic - not syncing: do_fork failed in kernel_thread, errno = -11

EIP: 0073:[<ffffe410>] CPU: 0 Not tainted ESP: 007b:b7e6ffb0 EFLAGS: 00000246
    Not tainted
EAX: 00000000 EBX: 000072b3 ECX: 00000013 EDX: 000072b3
ESI: 000072af EDI: 00000011 EBP: 00000000 DS: 007b ES: 007b
087e7eac:  [<0807ca7b>] notifier_call_chain+0x1d/0x33
087e7ec8:  [<08071416>] panic+0x52/0xdd
087e7ee4:  [<0805cd74>] kernel_thread+0x5d/0x5f
087e7ef4:  [<0808217f>] keventd_create_kthread+0x1a/0x48
087e7ef8:  [<080820cb>] kthread+0x0/0x9a
087e7f10:  [<0807f5fb>] run_workqueue+0x8a/0x11f
087e7f18:  [<08082165>] keventd_create_kthread+0x0/0x48
087e7f1c:  [<08068351>] set_signals+0x1d/0x32
087e7f2c:  [<0807f690>] worker_thread+0x0/0x14e
087e7f30:  [<0807f7a1>] worker_thread+0x111/0x14e
087e7f74:  [<0806e771>] default_wake_function+0x0/0x12
087e7f98:  [<0808213f>] kthread+0x74/0x9a
087e7fbc:  [<080679bf>] run_kernel_thread+0x38/0x41
087e7fd8:  [<080679a2>] run_kernel_thread+0x1b/0x41
087e7fe4:  [<0805f92f>] new_thread_handler+0x53/0x79
087e7fe8:  [<080820cb>] kthread+0x0/0x9a


regards
roland






> -----Ursprüngliche Nachricht-----
> Von: devzero@web.de
> Gesendet: 01.04.07 11:16:14
> An: linux-kernel@vger.kernel.org
> Betreff: Re: [patch] remove artificial software max_loop limit 


> >Remove artificial maximum 256 loop device that can be created due to a
> >legacy device number limit.  Searching through lkml archive, there are
> >several instances where users complained about the artificial limit
> >that the loop driver impose.  There is no reason to have such limit.
> 
> Hey, i was one of those :)
> 
> Nice to see, that it`s solved now, thanks very much!
> 
> I never expected this to happen and put all my hope into dm-loop instead.
> Did you mind that Bryn m. Reeves from redhat will suffer a serious depression now? ( ->  http://sources.redhat.com/lvm2/wiki/DMLoop ) ;)
> 
> regards
> roland
> 


_______________________________________________________________
SMS schreiben mit WEB.DE FreeMail - einfach, schnell und
kostenguenstig. Jetzt gleich testen! http://f.web.de/?mc=021192

Re: [patch] remove artificial software max_loop limit

[Ken Chen]

On 4/1/07, devzero@web.de <devzero@web.de> wrote:
> not sure if this is a real issue and if it`s UML or loop related -
> but how is low-memory situations being handled when
> creating loop devices ?

kernel returns -ENOMEM as an error code if there are no memory left to
initialize loop device.


> should losetup or dmesg tell "out of memory" if there is not
> enough memory left ?

It should, as kernel does pass that info back to app.  Does losetup
check return value of open() syscall?

Re: [patch] remove artificial software max_loop limit

[Jeff Dike]

On Sun, Apr 01, 2007 at 12:53:55PM +0200, devzero@web.de wrote:
> not sure if this is a real issue and if it`s UML or loop related -
> but how is low-memory situations being handled when creating loop
> devices ? 

It's UML-related - it's not dealing with the case of a kernel thread
failing because of a lack of memory.

				Jeff

-- 
Work email - jdike at linux dot intel dot com

[patch] remove artificial software max_loop limit

[Ken Chen]

Remove artificial maximum 256 loop device that can be created due to a
legacy device number limit.  Searching through lkml archive, there are
several instances where users complained about the artificial limit
that the loop driver impose.  There is no reason to have such limit.

This patch rid the limit entirely and make loop device and associated
block queue instantiation on demand.  With on-demand instantiation, it
also gives the benefit of not wasting memory if these devices are not
in use (compare to current implementation that always create 8 loop
devices), a net improvement in both areas.  This version is both
tested with creation of large number of loop devices and is compatible
with existing losetup/mount user land tools.

There are a number of people who worked on this and provided valuable
suggestions, in no particular order, by:

Jens Axboe
Jan Engelhardt
Christoph Hellwig
Thomas M

Signed-off-by: Ken Chen <kenchen@google.com>

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6b5b642..7db2c38 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -77,9 +77,8 @@ #include <linux/kthread.h>

 #include <asm/uaccess.h>

-static int max_loop = 8;
-static struct loop_device *loop_dev;
-static struct gendisk **disks;
+static LIST_HEAD(loop_devices);
+static DEFINE_SPINLOCK(loop_devices_lock);

 /*
  * Transfer functions
@@ -183,7 +182,7 @@ figure_loop_size(struct loop_device *lo)
 	if (unlikely((loff_t)x != size))
 		return -EFBIG;

-	set_capacity(disks[lo->lo_number], x);
+	set_capacity(lo->lo_disk, x);
 	return 0;					
 }

@@ -812,7 +811,7 @@ static int loop_set_fd(struct loop_devic
 	lo->lo_queue->queuedata = lo;
 	lo->lo_queue->unplug_fn = loop_unplug;

-	set_capacity(disks[lo->lo_number], size);
+	set_capacity(lo->lo_disk, size);
 	bd_set_size(bdev, size << 9);

 	set_blocksize(bdev, lo_blocksize);
@@ -832,7 +831,7 @@ out_clr:
 	lo->lo_device = NULL;
 	lo->lo_backing_file = NULL;
 	lo->lo_flags = 0;
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	invalidate_bdev(bdev, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
@@ -918,7 +917,7 @@ static int loop_clr_fd(struct loop_devic
 	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
 	invalidate_bdev(bdev, 0);
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
 	lo->lo_state = Lo_unbound;
@@ -1322,6 +1321,23 @@ static long lo_compat_ioctl(struct file
 }
 #endif

+static struct loop_device *loop_find_dev(int number)
+{
+	struct loop_device *lo;
+	int found = 0;
+		
+	spin_lock(&loop_devices_lock);
+	list_for_each_entry(lo, &loop_devices, lo_list) {
+		if (lo->lo_number == number) {
+			found = 1;
+			break;
+		}
+	}
+	spin_unlock(&loop_devices_lock);
+	return found ? lo : 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;
@@ -1330,6 +1346,9 @@ static int lo_open(struct inode *inode,
 	lo->lo_refcnt++;
 	mutex_unlock(&lo->lo_ctl_mutex);

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

@@ -1357,8 +1376,6 @@ #endif
 /*
  * And now the modules code and kernel interface.
  */
-module_param(max_loop, int, 0);
-MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-256)");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);

@@ -1383,7 +1400,7 @@ int loop_unregister_transfer(int number)

 	xfer_funcs[n] = NULL;

-	for (lo = &loop_dev[0]; lo < &loop_dev[max_loop]; lo++) {
+	list_for_each_entry(lo, &loop_devices, lo_list) {
 		mutex_lock(&lo->lo_ctl_mutex);

 		if (lo->lo_encryption == xfer)
@@ -1398,102 +1415,104 @@ int loop_unregister_transfer(int number)
 EXPORT_SYMBOL(loop_register_transfer);
 EXPORT_SYMBOL(loop_unregister_transfer);

-static int __init loop_init(void)
+static struct loop_device *loop_init_one(int i)
 {
-	int	i;
+	struct loop_device *lo;
+	struct gendisk *disk;

-	if (max_loop < 1 || max_loop > 256) {
-		printk(KERN_WARNING "loop: invalid max_loop (must be between"
-				    " 1 and 256), using default (8)\n");
-		max_loop = 8;
-	}
+	lo = kzalloc(sizeof(*lo), GFP_KERNEL);
+	if (!lo)
+		goto out;

-	if (register_blkdev(LOOP_MAJOR, "loop"))
-		return -EIO;
+	lo->lo_queue = blk_alloc_queue(GFP_KERNEL);
+	if (!lo->lo_queue)
+		goto out_free_dev;
+
+	disk = lo->lo_disk = alloc_disk(1);
+	if (!disk)
+		goto out_free_queue;
+
+	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;
+	disk->private_data	= lo;
+	disk->queue		= lo->lo_queue;
+	sprintf(disk->disk_name, "loop%d", i);
+	add_disk(disk);
+	spin_lock(&loop_devices_lock);
+	list_add_tail(&lo->lo_list, &loop_devices);
+	spin_unlock(&loop_devices_lock);
+	return lo;
+
+out_free_queue:
+	blk_cleanup_queue(lo->lo_queue);
+out_free_dev:
+	kfree(lo);
+out:
+	return ERR_PTR(-ENOMEM);
+}

-	loop_dev = kmalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
-	if (!loop_dev)
-		goto out_mem1;
-	memset(loop_dev, 0, max_loop * sizeof(struct loop_device));
+static void loop_del_one(struct loop_device *lo)
+{
+	del_gendisk(lo->lo_disk);
+	blk_cleanup_queue(lo->lo_queue);
+	put_disk(lo->lo_disk);
+	list_del(&lo->lo_list);
+	kfree(lo);
+}

-	disks = kmalloc(max_loop * sizeof(struct gendisk *), GFP_KERNEL);
-	if (!disks)
-		goto out_mem2;
+static struct kobject *loop_probe(dev_t dev, int *part, void *data)
+{
+	unsigned int number = dev & MINORMASK;
+	struct loop_device *lo;

-	for (i = 0; i < max_loop; i++) {
-		disks[i] = alloc_disk(1);
-		if (!disks[i])
-			goto out_mem3;
+	if ((lo = loop_find_dev(number)) == NULL) {
+		lo = loop_init_one(number);
+		*part = 0;
+		if (IS_ERR(lo))
+			return (void *)lo;
 	}
+	return &lo->lo_disk->kobj;
+}

-	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;
-	}
+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);

-	/* We cannot fail after we call this, so another loop!*/
-	for (i = 0; i < max_loop; i++)
-		add_disk(disks[i]);
-	printk(KERN_INFO "loop: loaded (max %d devices)\n", max_loop);
+	lo = loop_init_one(0);
+	if (IS_ERR(lo))
+		goto out_free;
+
+	printk(KERN_INFO "loop: loaded");
 	return 0;

-out_mem4:
-	while (i--)
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-	i = max_loop;
-out_mem3:
-	while (i--)
-		put_disk(disks[i]);
-	kfree(disks);
-out_mem2:
-	kfree(loop_dev);
-out_mem1:
+out_free:
 	unregister_blkdev(LOOP_MAJOR, "loop");
 	printk(KERN_ERR "loop: ran out of memory\n");
 	return -ENOMEM;
 }

-static void loop_exit(void)
+static void __exit loop_exit(void)
 {
-	int i;
+	struct loop_device *lo, *next;

-	for (i = 0; i < max_loop; i++) {
-		del_gendisk(disks[i]);
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-		put_disk(disks[i]);
-	}
+	list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
+		loop_del_one(lo);
+
+	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), 1UL << MINORBITS);
 	if (unregister_blkdev(LOOP_MAJOR, "loop"))
 		printk(KERN_WARNING "loop: cannot unregister blkdev\n");
-
-	kfree(disks);
-	kfree(loop_dev);
 }

 module_init(loop_init);
 module_exit(loop_exit);
-
-#ifndef MODULE
-static int __init max_loop_setup(char *str)
-{
-	max_loop = simple_strtol(str, NULL, 0);
-	return 1;
-}
-
-__setup("max_loop=", max_loop_setup);
-#endif
diff --git a/include/linux/loop.h b/include/linux/loop.h
index 191a595..0b99b31 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -64,6 +64,8 @@ struct loop_device {
 	wait_queue_head_t	lo_event;

 	request_queue_t		*lo_queue;
+	struct gendisk		*lo_disk;
+	struct list_head	lo_list;
 };

 #endif /* __KERNEL__ */

Re: [patch] remove artificial software max_loop limit

[Ken Chen]

On 3/30/07, Ken Chen <kenchen@google.com> wrote:
> Remove artificial maximum 256 loop device that can be created due to a
> legacy device number limit.  Searching through lkml archive, there are
> several instances where users complained about the artificial limit
> that the loop driver impose.  There is no reason to have such limit.
>
> This patch rid the limit entirely and make loop device and associated
> block queue instantiation on demand.  With on-demand instantiation, it
> also gives the benefit of not wasting memory if these devices are not
> in use (compare to current implementation that always create 8 loop
> devices), a net improvement in both areas.  This version is both
> tested with creation of large number of loop devices and is compatible
> with existing losetup/mount user land tools.
>
> Signed-off-by: Ken Chen <kenchen@google.com>

Spotted one bug: the sequence of loop device lookup, instantiation,
and then insert into global loop_devices_list better be in one
critical section, otherwise smp race ensues.  Fix that up and also use
mutex lock instead of spin_lock for loop_devices_list.

Signed-off-by: Ken Chen <kenchen@google.com>

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6b5b642..1e87aea 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -77,9 +77,8 @@

 #include <asm/uaccess.h>

-static int max_loop = 8;
-static struct loop_device *loop_dev;
-static struct gendisk **disks;
+static LIST_HEAD(loop_devices);
+static DEFINE_MUTEX(loop_devices_mutex);

 /*
  * Transfer functions
@@ -183,7 +182,7 @@ figure_loop_size(struct loop_device *lo)
 	if (unlikely((loff_t)x != size))
 		return -EFBIG;

-	set_capacity(disks[lo->lo_number], x);
+	set_capacity(lo->lo_disk, x);
 	return 0;					
 }

@@ -812,7 +811,7 @@ static int loop_set_fd
 	lo->lo_queue->queuedata = lo;
 	lo->lo_queue->unplug_fn = loop_unplug;

-	set_capacity(disks[lo->lo_number], size);
+	set_capacity(lo->lo_disk, size);
 	bd_set_size(bdev, size << 9);

 	set_blocksize(bdev, lo_blocksize);
@@ -832,7 +831,7 @@ out_clr:
 	lo->lo_device = NULL;
 	lo->lo_backing_file = NULL;
 	lo->lo_flags = 0;
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	invalidate_bdev(bdev, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
@@ -918,7 +917,7 @@ static int loop_clr_fd
 	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
 	invalidate_bdev(bdev, 0);
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
 	lo->lo_state = Lo_unbound;
@@ -1322,6 +1321,18 @@ static long lo_compat_ioctl
 }
 #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;
@@ -1330,6 +1341,11 @@ static int lo_open
 	lo->lo_refcnt++;
 	mutex_unlock(&lo->lo_ctl_mutex);

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

@@ -1357,8 +1373,6 @@ static struct block_device_operations lo_fops
 /*
  * And now the modules code and kernel interface.
  */
-module_param(max_loop, int, 0);
-MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-256)");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);

@@ -1383,7 +1397,7 @@ int loop_unregister_transfer(int number)

 	xfer_funcs[n] = NULL;

-	for (lo = &loop_dev[0]; lo < &loop_dev[max_loop]; lo++) {
+	list_for_each_entry(lo, &loop_devices, lo_list) {
 		mutex_lock(&lo->lo_ctl_mutex);

 		if (lo->lo_encryption == xfer)
@@ -1398,102 +1412,106 @@ int loop_unregister_transfer(int number)
 EXPORT_SYMBOL(loop_register_transfer);
 EXPORT_SYMBOL(loop_unregister_transfer);

-static int __init loop_init(void)
+static struct loop_device *loop_init_one(int i)
 {
-	int	i;
+	struct loop_device *lo;
+	struct gendisk *disk;

-	if (max_loop < 1 || max_loop > 256) {
-		printk(KERN_WARNING "loop: invalid max_loop (must be between"
-				    " 1 and 256), using default (8)\n");
-		max_loop = 8;
-	}
+	lo = kzalloc(sizeof(*lo), GFP_KERNEL);
+	if (!lo)
+		goto out;

-	if (register_blkdev(LOOP_MAJOR, "loop"))
-		return -EIO;
+	lo->lo_queue = blk_alloc_queue(GFP_KERNEL);
+	if (!lo->lo_queue)
+		goto out_free_dev;
+
+	disk = lo->lo_disk = alloc_disk(1);
+	if (!disk)
+		goto out_free_queue;
+
+	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;
+	disk->private_data	= lo;
+	disk->queue		= lo->lo_queue;
+	sprintf(disk->disk_name, "loop%d", i);
+	add_disk(disk);
+	list_add_tail(&lo->lo_list, &loop_devices);
+	return lo;
+
+out_free_queue:
+	blk_cleanup_queue(lo->lo_queue);
+out_free_dev:
+	kfree(lo);
+out:
+	return ERR_PTR(-ENOMEM);
+}

-	loop_dev = kmalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
-	if (!loop_dev)
-		goto out_mem1;
-	memset(loop_dev, 0, max_loop * sizeof(struct loop_device));
+static void loop_del_one(struct loop_device *lo)
+{
+	del_gendisk(lo->lo_disk);
+	blk_cleanup_queue(lo->lo_queue);
+	put_disk(lo->lo_disk);
+	list_del(&lo->lo_list);
+	kfree(lo);
+}

-	disks = kmalloc(max_loop * sizeof(struct gendisk *), GFP_KERNEL);
-	if (!disks)
-		goto out_mem2;
+static struct kobject *loop_probe(dev_t dev, int *part, void *data)
+{
+	unsigned int number = dev & MINORMASK;
+	struct loop_device *lo;

-	for (i = 0; i < max_loop; i++) {
-		disks[i] = alloc_disk(1);
-		if (!disks[i])
-			goto out_mem3;
-	}
+	mutex_lock(&loop_device_mutex);
+	lo = loop_find_dev(number);
+	if (lo == NULL)
+		lo = loop_init_one(number);
+	mutex_unlock(&loop_device_mutex);

-	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;
-	}
+	*part = 0;
+	if (IS_ERR(lo))
+		return (void *)lo;
+	else
+		return &lo->lo_disk->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);

-	/* We cannot fail after we call this, so another loop!*/
-	for (i = 0; i < max_loop; i++)
-		add_disk(disks[i]);
-	printk(KERN_INFO "loop: loaded (max %d devices)\n", max_loop);
+	lo = loop_init_one(0);
+	if (IS_ERR(lo))
+		goto out_free;
+
+	printk(KERN_INFO "loop: loaded");
 	return 0;

-out_mem4:
-	while (i--)
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-	i = max_loop;
-out_mem3:
-	while (i--)
-		put_disk(disks[i]);
-	kfree(disks);
-out_mem2:
-	kfree(loop_dev);
-out_mem1:
+out_free:
 	unregister_blkdev(LOOP_MAJOR, "loop");
 	printk(KERN_ERR "loop: ran out of memory\n");
 	return -ENOMEM;
 }

-static void loop_exit(void)
+static void __exit loop_exit(void)
 {
-	int i;
+	struct loop_device *lo, *next;

-	for (i = 0; i < max_loop; i++) {
-		del_gendisk(disks[i]);
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-		put_disk(disks[i]);
-	}
+	list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
+		loop_del_one(lo);
+
+	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), 1UL << MINORBITS);
 	if (unregister_blkdev(LOOP_MAJOR, "loop"))
 		printk(KERN_WARNING "loop: cannot unregister blkdev\n");
-
-	kfree(disks);
-	kfree(loop_dev);
 }

 module_init(loop_init);
 module_exit(loop_exit);
-
-#ifndef MODULE
-static int __init max_loop_setup(char *str)
-{
-	max_loop = simple_strtol(str, NULL, 0);
-	return 1;
-}
-
-__setup("max_loop=", max_loop_setup);
-#endif
diff --git a/include/linux/loop.h b/include/linux/loop.h
index 191a595..0b99b31 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -64,6 +64,8 @@ struct loop_device {
 	wait_queue_head_t	lo_event;

 	request_queue_t		*lo_queue;
+	struct gendisk		*lo_disk;
+	struct list_head	lo_list;
 };

 #endif /* __KERNEL__ */

Re: [patch] remove artificial software max_loop limit

[Jan Engelhardt]

On Mar 30 2007 01:48, Ken Chen wrote:
> On 3/30/07, Ken Chen <kenchen@google.com> wrote:

> @@ -812,7 +811,7 @@ static int loop_set_fd
> lo->lo_queue->queuedata = lo;
> lo->lo_queue->unplug_fn = loop_unplug;
>
> -	set_capacity(disks[lo->lo_number], size);
> +	set_capacity(lo->lo_disk, size);
> bd_set_size(bdev, size << 9);
>
> 	set_blocksize(bdev, lo_blocksize);
> @@ -832,7 +831,7 @@ out_clr:
> lo->lo_device = NULL;
> lo->lo_backing_file = NULL;
> lo->lo_flags = 0;
> -	set_capacity(disks[lo->lo_number], 0);
> +	set_capacity(lo->lo_disk, 0);
> invalidate_bdev(bdev, 0);
> bd_set_size(bdev, 0);
> mapping_set_gfp_mask(mapping, lo->old_gfp_mask);

Welcome to Google Mail, we eat your whitespace :-)

> +	sprintf(disk->disk_name, "loop%d", i);

Let's make this %u, no?

> +static struct kobject *loop_probe(dev_t dev, int *part, void *data)
> +{
> +	unsigned int number = dev & MINORMASK;
> +	struct loop_device *lo;
>
> -	for (i = 0; i < max_loop; i++) {
> -		disks[i] = alloc_disk(1);
> -		if (!disks[i])
> -			goto out_mem3;
> -	}

Could not we use kobject_uevent(), like I did (though perhaps without
the k.oops), to make udev create a node?


Jan
-- 

Re: [patch] remove artificial software max_loop limit

[Ken Chen]

On 3/30/07, Jan Engelhardt <jengelh@linux01.gwdg.de> wrote:
> > lo->lo_device = NULL;
> > lo->lo_backing_file = NULL;
> > lo->lo_flags = 0;
> > -     set_capacity(disks[lo->lo_number], 0);
> > +     set_capacity(lo->lo_disk, 0);
> > invalidate_bdev(bdev, 0);
> > bd_set_size(bdev, 0);
> > mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
>
> Welcome to Google Mail, we eat your whitespace :-)

Oh, crap.  Google mail is innocent.  It was me who did some ugly
copy-paste between apps.

> > +     sprintf(disk->disk_name, "loop%d", i);
>
> Let's make this %u, no?

I don't mind either way, this thing won't be bigger than 1^20 anyway.
Oh, which reminds me that we probably should explicitly test and cap
that limit (1^20 that is).

> > +static struct kobject *loop_probe(dev_t dev, int *part, void *data)
> > +{
> > +     unsigned int number = dev & MINORMASK;
> > +     struct loop_device *lo;
> >
> > -     for (i = 0; i < max_loop; i++) {
> > -             disks[i] = alloc_disk(1);
> > -             if (!disks[i])
> > -                     goto out_mem3;
> > -     }
>
> Could not we use kobject_uevent(), like I did (though perhaps without
> the k.oops), to make udev create a node?

Well, I go with a version that doesn't panic kernel, which is a
deciding factor here ;-)


Full patch attached.

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6b5b642..1e87aea 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -77,9 +77,8 @@

 #include <asm/uaccess.h>

-static int max_loop = 8;
-static struct loop_device *loop_dev;
-static struct gendisk **disks;
+static LIST_HEAD(loop_devices);
+static DEFINE_MUTEX(loop_devices_mutex);

 /*
  * Transfer functions
@@ -183,7 +182,7 @@ figure_loop_size(struct loop_device *lo)
 	if (unlikely((loff_t)x != size))
 		return -EFBIG;

-	set_capacity(disks[lo->lo_number], x);
+	set_capacity(lo->lo_disk, x);
 	return 0;					
 }

@@ -812,7 +811,7 @@ static int loop_set_fd
 	lo->lo_queue->queuedata = lo;
 	lo->lo_queue->unplug_fn = loop_unplug;

-	set_capacity(disks[lo->lo_number], size);
+	set_capacity(lo->lo_disk, size);
 	bd_set_size(bdev, size << 9);

 	set_blocksize(bdev, lo_blocksize);
@@ -832,7 +831,7 @@ out_clr:
 	lo->lo_device = NULL;
 	lo->lo_backing_file = NULL;
 	lo->lo_flags = 0;
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	invalidate_bdev(bdev, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
@@ -918,7 +917,7 @@ static int loop_clr_fd
 	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
 	invalidate_bdev(bdev, 0);
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
 	lo->lo_state = Lo_unbound;
@@ -1322,6 +1321,18 @@ static long lo_compat_ioctl
 }
 #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;
@@ -1330,6 +1341,11 @@ static int lo_open
 	lo->lo_refcnt++;
 	mutex_unlock(&lo->lo_ctl_mutex);

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

@@ -1357,8 +1373,6 @@ static struct block_device_operations lo_fops
 /*
  * And now the modules code and kernel interface.
  */
-module_param(max_loop, int, 0);
-MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-256)");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);

@@ -1383,7 +1397,7 @@ int loop_unregister_transfer(int number)

 	xfer_funcs[n] = NULL;

-	for (lo = &loop_dev[0]; lo < &loop_dev[max_loop]; lo++) {
+	list_for_each_entry(lo, &loop_devices, lo_list) {
 		mutex_lock(&lo->lo_ctl_mutex);

 		if (lo->lo_encryption == xfer)
@@ -1398,102 +1412,106 @@ int loop_unregister_transfer(int number)
 EXPORT_SYMBOL(loop_register_transfer);
 EXPORT_SYMBOL(loop_unregister_transfer);

-static int __init loop_init(void)
+static struct loop_device *loop_init_one(int i)
 {
-	int	i;
+	struct loop_device *lo;
+	struct gendisk *disk;

-	if (max_loop < 1 || max_loop > 256) {
-		printk(KERN_WARNING "loop: invalid max_loop (must be between"
-				    " 1 and 256), using default (8)\n");
-		max_loop = 8;
-	}
+	lo = kzalloc(sizeof(*lo), GFP_KERNEL);
+	if (!lo)
+		goto out;

-	if (register_blkdev(LOOP_MAJOR, "loop"))
-		return -EIO;
+	lo->lo_queue = blk_alloc_queue(GFP_KERNEL);
+	if (!lo->lo_queue)
+		goto out_free_dev;
+
+	disk = lo->lo_disk = alloc_disk(1);
+	if (!disk)
+		goto out_free_queue;
+
+	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;
+	disk->private_data	= lo;
+	disk->queue		= lo->lo_queue;
+	sprintf(disk->disk_name, "loop%d", i);
+	add_disk(disk);
+	list_add_tail(&lo->lo_list, &loop_devices);
+	return lo;
+
+out_free_queue:
+	blk_cleanup_queue(lo->lo_queue);
+out_free_dev:
+	kfree(lo);
+out:
+	return ERR_PTR(-ENOMEM);
+}

-	loop_dev = kmalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
-	if (!loop_dev)
-		goto out_mem1;
-	memset(loop_dev, 0, max_loop * sizeof(struct loop_device));
+static void loop_del_one(struct loop_device *lo)
+{
+	del_gendisk(lo->lo_disk);
+	blk_cleanup_queue(lo->lo_queue);
+	put_disk(lo->lo_disk);
+	list_del(&lo->lo_list);
+	kfree(lo);
+}

-	disks = kmalloc(max_loop * sizeof(struct gendisk *), GFP_KERNEL);
-	if (!disks)
-		goto out_mem2;
+static struct kobject *loop_probe(dev_t dev, int *part, void *data)
+{
+	unsigned int number = dev & MINORMASK;
+	struct loop_device *lo;

-	for (i = 0; i < max_loop; i++) {
-		disks[i] = alloc_disk(1);
-		if (!disks[i])
-			goto out_mem3;
-	}
+	mutex_lock(&loop_device_mutex);
+	lo = loop_find_dev(number);
+	if (lo == NULL)
+		lo = loop_init_one(number);
+	mutex_unlock(&loop_device_mutex);

-	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;
-	}
+	*part = 0;
+	if (IS_ERR(lo))
+		return (void *)lo;
+	else
+		return &lo->lo_disk->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);

-	/* We cannot fail after we call this, so another loop!*/
-	for (i = 0; i < max_loop; i++)
-		add_disk(disks[i]);
-	printk(KERN_INFO "loop: loaded (max %d devices)\n", max_loop);
+	lo = loop_init_one(0);
+	if (IS_ERR(lo))
+		goto out_free;
+
+	printk(KERN_INFO "loop: loaded");
 	return 0;

-out_mem4:
-	while (i--)
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-	i = max_loop;
-out_mem3:
-	while (i--)
-		put_disk(disks[i]);
-	kfree(disks);
-out_mem2:
-	kfree(loop_dev);
-out_mem1:
+out_free:
 	unregister_blkdev(LOOP_MAJOR, "loop");
 	printk(KERN_ERR "loop: ran out of memory\n");
 	return -ENOMEM;
 }

-static void loop_exit(void)
+static void __exit loop_exit(void)
 {
-	int i;
+	struct loop_device *lo, *next;

-	for (i = 0; i < max_loop; i++) {
-		del_gendisk(disks[i]);
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-		put_disk(disks[i]);
-	}
+	list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
+		loop_del_one(lo);
+
+	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), 1UL << MINORBITS);
 	if (unregister_blkdev(LOOP_MAJOR, "loop"))
 		printk(KERN_WARNING "loop: cannot unregister blkdev\n");
-
-	kfree(disks);
-	kfree(loop_dev);
 }

 module_init(loop_init);
 module_exit(loop_exit);
-
-#ifndef MODULE
-static int __init max_loop_setup(char *str)
-{
-	max_loop = simple_strtol(str, NULL, 0);
-	return 1;
-}
-
-__setup("max_loop=", max_loop_setup);
-#endif
diff --git a/include/linux/loop.h b/include/linux/loop.h
index 191a595..0b99b31 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -64,6 +64,8 @@ struct loop_device {
 	wait_queue_head_t	lo_event;

 	request_queue_t		*lo_queue;
+	struct gendisk		*lo_disk;
+	struct list_head	lo_list;
 };

 #endif /* __KERNEL__ */

Re: [patch] remove artificial software max_loop limit

[Jan Engelhardt]

On Mar 30 2007 02:25, Ken Chen wrote:
>
> Oh, crap.  Google mail is innocent.  It was me who did some ugly
> copy-paste between apps.

Well, you did it again :p

> I don't mind either way, this thing won't be bigger than 1^20 anyway.
> Oh, which reminds me that we probably should explicitly test and cap
> that limit (1^20 that is).

I do not think is needed, since it will already be caught by the
layout of a dev_t.

> +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;
> +	}

Can do without {}


Jan
-- 

Re: [patch] remove artificial software max_loop limit

[Andrew Morton]

On Fri, 30 Mar 2007 02:25:37 -0700
"Ken Chen" <kenchen@google.com> wrote:

> -module_param(max_loop, int, 0);
> -MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-256)");

So..  this change will cause a fatal error for anyone who is presently
using max_loop, won't it?  If they're doing that within their
initramfs/initrd/etc then things could get rather ugly for them.

I don't know how much of a problem this will be in practice - do people use
max_loop much?

btw, did you test this change as both a module and as linked-into-vmlinux?

Re: [patch] remove artificial software max_loop limit

[Ken Chen]

On 3/30/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> So..  this change will cause a fatal error for anyone who is presently
> using max_loop, won't it?  If they're doing that within their
> initramfs/initrd/etc then things could get rather ugly for them.

probably, if they access loop device non-sequentially.


> I don't know how much of a problem this will be in practice - do people use
> max_loop much?

I don't know either.


> btw, did you test this change as both a module and as linked-into-vmlinux?

as linked-into-vmlinux.  why do you ask?  It breaks if it is module?
I made last minute change to a mutex name and shamely posted without
doing a compile test.  Besides that, is there something else breaks?

- Ken

Re: [patch] remove artificial software max_loop limit

[Andrew Morton]

On Fri, 30 Mar 2007 15:06:03 -0700
"Ken Chen" <kenchen@google.com> wrote:

> On 3/30/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> > So..  this change will cause a fatal error for anyone who is presently
> > using max_loop, won't it?  If they're doing that within their
> > initramfs/initrd/etc then things could get rather ugly for them.
> 
> probably, if they access loop device non-sequentially.
> 

My point is that the modprobe will fail if it is passed an unrecognised
module parameter (won't it?)

So if we're worried about not breaking existing setups, we should retain
this module parameter as a do-nothing thing, maybe with a
this-is-going-away warning printk, too.

> 
> > I don't know how much of a problem this will be in practice - do people use
> > max_loop much?
> 
> I don't know either.

hm.

> 
> > btw, did you test this change as both a module and as linked-into-vmlinux?
> 
> as linked-into-vmlinux.  why do you ask?  It breaks if it is module?
> I made last minute change to a mutex name and shamely posted without
> doing a compile test.  Besides that, is there something else breaks?

Just idle curiosity regarding how much testing it had seen.

Generally one would expect things to be OK, but there can be startup
ordering problems.

The most common problem is that the module simply doesn't load because it's
using some not-exported-to-modules symbol

Re: [patch] remove artificial software max_loop limit

[Greg KH]

On Fri, Mar 30, 2007 at 02:15:24PM -0700, Andrew Morton wrote:
> On Fri, 30 Mar 2007 02:25:37 -0700
> "Ken Chen" <kenchen@google.com> wrote:
> 
> > -module_param(max_loop, int, 0);
> > -MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-256)");
> 
> So..  this change will cause a fatal error for anyone who is presently
> using max_loop, won't it?  If they're doing that within their
> initramfs/initrd/etc then things could get rather ugly for them.
> 
> I don't know how much of a problem this will be in practice - do people use
> max_loop much?

Yes, the distros do, and they recommend it to their users a lot.

thanks,

greg k-h

Re: [patch] remove artificial software max_loop limit

[Andrew Morton]

On Sat, 31 Mar 2007 10:07:43 -0700 Greg KH <greg@kroah.com> wrote:

> On Fri, Mar 30, 2007 at 02:15:24PM -0700, Andrew Morton wrote:
> > On Fri, 30 Mar 2007 02:25:37 -0700
> > "Ken Chen" <kenchen@google.com> wrote:
> > 
> > > -module_param(max_loop, int, 0);
> > > -MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-256)");
> > 
> > So..  this change will cause a fatal error for anyone who is presently
> > using max_loop, won't it?  If they're doing that within their
> > initramfs/initrd/etc then things could get rather ugly for them.
> > 
> > I don't know how much of a problem this will be in practice - do people use
> > max_loop much?
> 
> Yes, the distros do, and they recommend it to their users a lot.

Thanks.  In that case I think we should retain the max_loop module parameter
for now.

Ken, when you respin that patch could you restore max_loop, and make its
use trigger a warning along the lines of "loop: the max_loop option is obsolete
and will be removed in March 2008"?

Re: [patch] remove artificial software max_loop limit

[Ken Chen]

[-- Attachment #1: Type: text/plain, Size: 9303 bytes --]

On 3/31/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> > Yes, the distros do, and they recommend it to their users a lot.
>
> Thanks.  In that case I think we should retain the max_loop module parameter
> for now.
>
> Ken, when you respin that patch could you restore max_loop, and make its
> use trigger a warning along the lines of "loop: the max_loop option is obsolete
> and will be removed in March 2008"?

OK, here is a re-spin patch that I tested as module or
link-in-vmlinux.  Both produce satisfactory result for me.  I also
enclosed the patch as an attachment just in case my email client
decide to eat away white spaces for the in-line text.

------------------------------------------------------
Subject: remove artificial software max_loop limit
From: "Ken Chen" <kenchen@google.com>

Remove artificial maximum 256 loop device that can be created due to a
legacy device number limit.  Searching through lkml archive, there are
several instances where users complained about the artificial limit
that the loop driver impose.  There is no reason to have such limit.

This patch rid the limit entirely and make loop device and associated
block queue instantiation on demand.  With on-demand instantiation, it
also gives the benefit of not wasting memory if these devices are not
in use (compare to current implementation that always create 8 loop
devices), a net improvement in both areas.  This version is both
tested with creation of large number of loop devices and is compatible
with existing losetup/mount user land tools.

There are a number of people who worked on this and provided valuable
suggestions, in no particular order, by:

Jens Axboe
Jan Engelhardt
Christoph Hellwig
Thomas M

Signed-off-by: Ken Chen <kenchen@google.com>
Cc: Jan Engelhardt <jengelh@linux01.gwdg.de>
Cc: Christoph Hellwig <hch@lst.de>

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6b5b642..605c1d3 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -77,9 +77,8 @@

 #include <asm/uaccess.h>

-static int max_loop = 8;
-static struct loop_device *loop_dev;
-static struct gendisk **disks;
+static LIST_HEAD(loop_devices);
+static DEFINE_MUTEX(loop_devices_mutex);

 /*
  * Transfer functions
@@ -183,7 +182,7 @@ figure_loop_size(struct loop_device *lo)
 	if (unlikely((loff_t)x != size))
 		return -EFBIG;

-	set_capacity(disks[lo->lo_number], x);
+	set_capacity(lo->lo_disk, x);
 	return 0;					
 }

@@ -812,7 +811,7 @@ static int loop_set_fd
 	lo->lo_queue->queuedata = lo;
 	lo->lo_queue->unplug_fn = loop_unplug;

-	set_capacity(disks[lo->lo_number], size);
+	set_capacity(lo->lo_disk, size);
 	bd_set_size(bdev, size << 9);

 	set_blocksize(bdev, lo_blocksize);
@@ -832,7 +831,7 @@ out_clr:
 	lo->lo_device = NULL;
 	lo->lo_backing_file = NULL;
 	lo->lo_flags = 0;
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	invalidate_bdev(bdev, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
@@ -918,7 +917,7 @@ static int loop_clr_fd
 	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
 	invalidate_bdev(bdev, 0);
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
 	lo->lo_state = Lo_unbound;
@@ -1322,6 +1321,18 @@ static long lo_compat_ioctl
 }
 #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;
@@ -1330,6 +1341,11 @@ static int lo_open
 	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;
 }

@@ -1357,8 +1373,9 @@ static struct block_device_operations lo_fops = {
 /*
  * And now the modules code and kernel interface.
  */
+static int max_loop;
 module_param(max_loop, int, 0);
-MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-256)");
+MODULE_PARM_DESC(max_loop, "obsolete, loop device is created on-demand");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);

@@ -1383,7 +1400,7 @@ int loop_unregister_transfer(int number)

 	xfer_funcs[n] = NULL;

-	for (lo = &loop_dev[0]; lo < &loop_dev[max_loop]; lo++) {
+	list_for_each_entry(lo, &loop_devices, lo_list) {
 		mutex_lock(&lo->lo_ctl_mutex);

 		if (lo->lo_encryption == xfer)
@@ -1398,91 +1415,110 @@ int loop_unregister_transfer(int number)
 EXPORT_SYMBOL(loop_register_transfer);
 EXPORT_SYMBOL(loop_unregister_transfer);

-static int __init loop_init(void)
+static struct loop_device *loop_init_one(int i)
+{
+	struct loop_device *lo;
+	struct gendisk *disk;
+
+	lo = kzalloc(sizeof(*lo), GFP_KERNEL);
+	if (!lo)
+		goto out;
+
+	lo->lo_queue = blk_alloc_queue(GFP_KERNEL);
+	if (!lo->lo_queue)
+		goto out_free_dev;
+
+	disk = lo->lo_disk = alloc_disk(1);
+	if (!disk)
+		goto out_free_queue;
+
+	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;
+	disk->private_data	= lo;
+	disk->queue		= lo->lo_queue;
+	sprintf(disk->disk_name, "loop%d", i);
+	add_disk(disk);
+	list_add_tail(&lo->lo_list, &loop_devices);
+	return lo;
+
+out_free_queue:
+	blk_cleanup_queue(lo->lo_queue);
+out_free_dev:
+	kfree(lo);
+out:
+	return ERR_PTR(-ENOMEM);
+}
+
+static void loop_del_one(struct loop_device *lo)
 {
-	int	i;
+	del_gendisk(lo->lo_disk);
+	blk_cleanup_queue(lo->lo_queue);
+	put_disk(lo->lo_disk);
+	list_del(&lo->lo_list);
+	kfree(lo);
+}

-	if (max_loop < 1 || max_loop > 256) {
-		printk(KERN_WARNING "loop: invalid max_loop (must be between"
-				    " 1 and 256), using default (8)\n");
-		max_loop = 8;
-	}
+static struct kobject *loop_probe(dev_t dev, int *part, void *data)
+{
+	unsigned int number = dev & MINORMASK;
+	struct loop_device *lo;
+
+	mutex_lock(&loop_devices_mutex);
+	lo = loop_find_dev(number);
+	if (lo == NULL)
+		lo = loop_init_one(number);
+	mutex_unlock(&loop_devices_mutex);
+
+	*part = 0;
+	if (IS_ERR(lo))
+		return (void *)lo;
+	else
+		return &lo->lo_disk->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);

-	loop_dev = kmalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
-	if (!loop_dev)
-		goto out_mem1;
-	memset(loop_dev, 0, max_loop * sizeof(struct loop_device));
+	lo = loop_init_one(0);
+	if (IS_ERR(lo))
+		goto out;

-	disks = kmalloc(max_loop * sizeof(struct gendisk *), GFP_KERNEL);
-	if (!disks)
-		goto out_mem2;
+	if (max_loop) {
+		printk(KERN_INFO "loop: the max_loop option is obsolete "
+				 "and will be removed in March 2008\n");

-	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]);
-	printk(KERN_INFO "loop: loaded (max %d devices)\n", max_loop);
+	printk(KERN_INFO "loop: module loaded\n");
 	return 0;

-out_mem4:
-	while (i--)
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-	i = max_loop;
-out_mem3:
-	while (i--)
-		put_disk(disks[i]);
-	kfree(disks);
-out_mem2:
-	kfree(loop_dev);
-out_mem1:
+out:
 	unregister_blkdev(LOOP_MAJOR, "loop");
 	printk(KERN_ERR "loop: ran out of memory\n");
 	return -ENOMEM;
 }

-static void loop_exit(void)
+static void __exit loop_exit(void)
 {
-	int i;
+	struct loop_device *lo, *next;

-	for (i = 0; i < max_loop; i++) {
-		del_gendisk(disks[i]);
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-		put_disk(disks[i]);
-	}
+	list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
+		loop_del_one(lo);
+
+	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), 1UL << MINORBITS);
 	if (unregister_blkdev(LOOP_MAJOR, "loop"))
 		printk(KERN_WARNING "loop: cannot unregister blkdev\n");
-
-	kfree(disks);
-	kfree(loop_dev);
 }

 module_init(loop_init);
diff --git a/include/linux/loop.h b/include/linux/loop.h
index 191a595..0b99b31 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -64,6 +64,8 @@ struct loop_device {
 	wait_queue_head_t	lo_event;

 	request_queue_t		*lo_queue;
+	struct gendisk		*lo_disk;
+	struct list_head	lo_list;
 };

 #endif /* __KERNEL__ */

[-- Attachment #2: loop-0331.patch --]
[-- Type: text/x-patch, Size: 7489 bytes --]

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6b5b642..605c1d3 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -77,9 +77,8 @@
 
 #include <asm/uaccess.h>
 
-static int max_loop = 8;
-static struct loop_device *loop_dev;
-static struct gendisk **disks;
+static LIST_HEAD(loop_devices);
+static DEFINE_MUTEX(loop_devices_mutex);
 
 /*
  * Transfer functions
@@ -183,7 +182,7 @@ figure_loop_size(struct loop_device *lo)
 	if (unlikely((loff_t)x != size))
 		return -EFBIG;
 
-	set_capacity(disks[lo->lo_number], x);
+	set_capacity(lo->lo_disk, x);
 	return 0;					
 }
 
@@ -812,7 +811,7 @@ static int loop_set_fd
 	lo->lo_queue->queuedata = lo;
 	lo->lo_queue->unplug_fn = loop_unplug;
 
-	set_capacity(disks[lo->lo_number], size);
+	set_capacity(lo->lo_disk, size);
 	bd_set_size(bdev, size << 9);
 
 	set_blocksize(bdev, lo_blocksize);
@@ -832,7 +831,7 @@ out_clr:
 	lo->lo_device = NULL;
 	lo->lo_backing_file = NULL;
 	lo->lo_flags = 0;
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	invalidate_bdev(bdev, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
@@ -918,7 +917,7 @@ static int loop_clr_fd
 	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
 	invalidate_bdev(bdev, 0);
-	set_capacity(disks[lo->lo_number], 0);
+	set_capacity(lo->lo_disk, 0);
 	bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
 	lo->lo_state = Lo_unbound;
@@ -1322,6 +1321,18 @@ static long lo_compat_ioctl
 }
 #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;
@@ -1330,6 +1341,11 @@ static int lo_open
 	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;
 }
 
@@ -1357,8 +1373,9 @@ static struct block_device_operations lo_fops = {
 /*
  * And now the modules code and kernel interface.
  */
+static int max_loop;
 module_param(max_loop, int, 0);
-MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-256)");
+MODULE_PARM_DESC(max_loop, "obsolete, loop device is created on-demand");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);
 
@@ -1383,7 +1400,7 @@ int loop_unregister_transfer(int number)
 
 	xfer_funcs[n] = NULL;
 
-	for (lo = &loop_dev[0]; lo < &loop_dev[max_loop]; lo++) {
+	list_for_each_entry(lo, &loop_devices, lo_list) {
 		mutex_lock(&lo->lo_ctl_mutex);
 
 		if (lo->lo_encryption == xfer)
@@ -1398,91 +1415,110 @@ int loop_unregister_transfer(int number)
 EXPORT_SYMBOL(loop_register_transfer);
 EXPORT_SYMBOL(loop_unregister_transfer);
 
-static int __init loop_init(void)
+static struct loop_device *loop_init_one(int i)
+{
+	struct loop_device *lo;
+	struct gendisk *disk;
+
+	lo = kzalloc(sizeof(*lo), GFP_KERNEL);
+	if (!lo)
+		goto out;
+
+	lo->lo_queue = blk_alloc_queue(GFP_KERNEL);
+	if (!lo->lo_queue)
+		goto out_free_dev;
+
+	disk = lo->lo_disk = alloc_disk(1);
+	if (!disk)
+		goto out_free_queue;
+
+	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;
+	disk->private_data	= lo;
+	disk->queue		= lo->lo_queue;
+	sprintf(disk->disk_name, "loop%d", i);
+	add_disk(disk);
+	list_add_tail(&lo->lo_list, &loop_devices);
+	return lo;
+
+out_free_queue:
+	blk_cleanup_queue(lo->lo_queue);
+out_free_dev:
+	kfree(lo);
+out:
+	return ERR_PTR(-ENOMEM);
+}
+
+static void loop_del_one(struct loop_device *lo)
 {
-	int	i;
+	del_gendisk(lo->lo_disk);
+	blk_cleanup_queue(lo->lo_queue);
+	put_disk(lo->lo_disk);
+	list_del(&lo->lo_list);
+	kfree(lo);
+}
 
-	if (max_loop < 1 || max_loop > 256) {
-		printk(KERN_WARNING "loop: invalid max_loop (must be between"
-				    " 1 and 256), using default (8)\n");
-		max_loop = 8;
-	}
+static struct kobject *loop_probe(dev_t dev, int *part, void *data)
+{
+	unsigned int number = dev & MINORMASK;
+	struct loop_device *lo;
+
+	mutex_lock(&loop_devices_mutex);
+	lo = loop_find_dev(number);
+	if (lo == NULL)
+		lo = loop_init_one(number);
+	mutex_unlock(&loop_devices_mutex);
+
+	*part = 0;
+	if (IS_ERR(lo))
+		return (void *)lo;
+	else
+		return &lo->lo_disk->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);
 
-	loop_dev = kmalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
-	if (!loop_dev)
-		goto out_mem1;
-	memset(loop_dev, 0, max_loop * sizeof(struct loop_device));
+	lo = loop_init_one(0);
+	if (IS_ERR(lo))
+		goto out;
 
-	disks = kmalloc(max_loop * sizeof(struct gendisk *), GFP_KERNEL);
-	if (!disks)
-		goto out_mem2;
+	if (max_loop) {
+		printk(KERN_INFO "loop: the max_loop option is obsolete "
+				 "and will be removed in March 2008\n");
 
-	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]);
-	printk(KERN_INFO "loop: loaded (max %d devices)\n", max_loop);
+	printk(KERN_INFO "loop: module loaded\n");
 	return 0;
 
-out_mem4:
-	while (i--)
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-	i = max_loop;
-out_mem3:
-	while (i--)
-		put_disk(disks[i]);
-	kfree(disks);
-out_mem2:
-	kfree(loop_dev);
-out_mem1:
+out:
 	unregister_blkdev(LOOP_MAJOR, "loop");
 	printk(KERN_ERR "loop: ran out of memory\n");
 	return -ENOMEM;
 }
 
-static void loop_exit(void)
+static void __exit loop_exit(void)
 {
-	int i;
+	struct loop_device *lo, *next;
 
-	for (i = 0; i < max_loop; i++) {
-		del_gendisk(disks[i]);
-		blk_cleanup_queue(loop_dev[i].lo_queue);
-		put_disk(disks[i]);
-	}
+	list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
+		loop_del_one(lo);
+
+	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), 1UL << MINORBITS);
 	if (unregister_blkdev(LOOP_MAJOR, "loop"))
 		printk(KERN_WARNING "loop: cannot unregister blkdev\n");
-
-	kfree(disks);
-	kfree(loop_dev);
 }
 
 module_init(loop_init);
diff --git a/include/linux/loop.h b/include/linux/loop.h
index 191a595..0b99b31 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -64,6 +64,8 @@ struct loop_device {
 	wait_queue_head_t	lo_event;
 
 	request_queue_t		*lo_queue;
+	struct gendisk		*lo_disk;
+	struct list_head	lo_list;
 };
 
 #endif /* __KERNEL__ */

Re: [patch] remove artificial software max_loop limit

[Tomas M]

 > OK, here is a re-spin patch that I tested as module or
 > link-in-vmlinux.  Both produce satisfactory result for me.

Is there a plan to include this brilliant code in mainline Kernel?
It works excellent, tested with 15000 loop devices, it's simply cool.

Thank you for your consideration.

Tomas M
slax.org



Ken Chen wrote:
> On 3/31/07, Andrew Morton <akpm@linux-foundation.org> wrote:
>> > Yes, the distros do, and they recommend it to their users a lot.
>>
>> Thanks.  In that case I think we should retain the max_loop module 
>> parameter
>> for now.
>>
>> Ken, when you respin that patch could you restore max_loop, and make its
>> use trigger a warning along the lines of "loop: the max_loop option is 
>> obsolete
>> and will be removed in March 2008"?
> 
> OK, here is a re-spin patch that I tested as module or
> link-in-vmlinux.  Both produce satisfactory result for me.  I also
> enclosed the patch as an attachment just in case my email client
> decide to eat away white spaces for the in-line text.
> 
> ------------------------------------------------------
> Subject: remove artificial software max_loop limit
> From: "Ken Chen" <kenchen@google.com>
> 
> Remove artificial maximum 256 loop device that can be created due to a
> legacy device number limit.  Searching through lkml archive, there are
> several instances where users complained about the artificial limit
> that the loop driver impose.  There is no reason to have such limit.
> 
> This patch rid the limit entirely and make loop device and associated
> block queue instantiation on demand.  With on-demand instantiation, it
> also gives the benefit of not wasting memory if these devices are not
> in use (compare to current implementation that always create 8 loop
> devices), a net improvement in both areas.  This version is both
> tested with creation of large number of loop devices and is compatible
> with existing losetup/mount user land tools.
> 
> There are a number of people who worked on this and provided valuable
> suggestions, in no particular order, by:
> 
> Jens Axboe
> Jan Engelhardt
> Christoph Hellwig
> Thomas M
> 
> Signed-off-by: Ken Chen <kenchen@google.com>
> Cc: Jan Engelhardt <jengelh@linux01.gwdg.de>
> Cc: Christoph Hellwig <hch@lst.de>
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 6b5b642..605c1d3 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -77,9 +77,8 @@
> 
> #include <asm/uaccess.h>
> 
> -static int max_loop = 8;
> -static struct loop_device *loop_dev;
> -static struct gendisk **disks;
> +static LIST_HEAD(loop_devices);
> +static DEFINE_MUTEX(loop_devices_mutex);
> 
> /*
>  * Transfer functions
> @@ -183,7 +182,7 @@ figure_loop_size(struct loop_device *lo)
>     if (unlikely((loff_t)x != size))
>         return -EFBIG;
> 
> -    set_capacity(disks[lo->lo_number], x);
> +    set_capacity(lo->lo_disk, x);
>     return 0;                   
> }
> 
> @@ -812,7 +811,7 @@ static int loop_set_fd
>     lo->lo_queue->queuedata = lo;
>     lo->lo_queue->unplug_fn = loop_unplug;
> 
> -    set_capacity(disks[lo->lo_number], size);
> +    set_capacity(lo->lo_disk, size);
>     bd_set_size(bdev, size << 9);
> 
>     set_blocksize(bdev, lo_blocksize);
> @@ -832,7 +831,7 @@ out_clr:
>     lo->lo_device = NULL;
>     lo->lo_backing_file = NULL;
>     lo->lo_flags = 0;
> -    set_capacity(disks[lo->lo_number], 0);
> +    set_capacity(lo->lo_disk, 0);
>     invalidate_bdev(bdev, 0);
>     bd_set_size(bdev, 0);
>     mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
> @@ -918,7 +917,7 @@ static int loop_clr_fd
>     memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
>     memset(lo->lo_file_name, 0, LO_NAME_SIZE);
>     invalidate_bdev(bdev, 0);
> -    set_capacity(disks[lo->lo_number], 0);
> +    set_capacity(lo->lo_disk, 0);
>     bd_set_size(bdev, 0);
>     mapping_set_gfp_mask(filp->f_mapping, gfp);
>     lo->lo_state = Lo_unbound;
> @@ -1322,6 +1321,18 @@ static long lo_compat_ioctl
> }
> #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;
> @@ -1330,6 +1341,11 @@ static int lo_open
>     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;
> }
> 
> @@ -1357,8 +1373,9 @@ static struct block_device_operations lo_fops = {
> /*
>  * And now the modules code and kernel interface.
>  */
> +static int max_loop;
> module_param(max_loop, int, 0);
> -MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-256)");
> +MODULE_PARM_DESC(max_loop, "obsolete, loop device is created on-demand");
> MODULE_LICENSE("GPL");
> MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);
> 
> @@ -1383,7 +1400,7 @@ int loop_unregister_transfer(int number)
> 
>     xfer_funcs[n] = NULL;
> 
> -    for (lo = &loop_dev[0]; lo < &loop_dev[max_loop]; lo++) {
> +    list_for_each_entry(lo, &loop_devices, lo_list) {
>         mutex_lock(&lo->lo_ctl_mutex);
> 
>         if (lo->lo_encryption == xfer)
> @@ -1398,91 +1415,110 @@ int loop_unregister_transfer(int number)
> EXPORT_SYMBOL(loop_register_transfer);
> EXPORT_SYMBOL(loop_unregister_transfer);
> 
> -static int __init loop_init(void)
> +static struct loop_device *loop_init_one(int i)
> +{
> +    struct loop_device *lo;
> +    struct gendisk *disk;
> +
> +    lo = kzalloc(sizeof(*lo), GFP_KERNEL);
> +    if (!lo)
> +        goto out;
> +
> +    lo->lo_queue = blk_alloc_queue(GFP_KERNEL);
> +    if (!lo->lo_queue)
> +        goto out_free_dev;
> +
> +    disk = lo->lo_disk = alloc_disk(1);
> +    if (!disk)
> +        goto out_free_queue;
> +
> +    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;
> +    disk->private_data    = lo;
> +    disk->queue        = lo->lo_queue;
> +    sprintf(disk->disk_name, "loop%d", i);
> +    add_disk(disk);
> +    list_add_tail(&lo->lo_list, &loop_devices);
> +    return lo;
> +
> +out_free_queue:
> +    blk_cleanup_queue(lo->lo_queue);
> +out_free_dev:
> +    kfree(lo);
> +out:
> +    return ERR_PTR(-ENOMEM);
> +}
> +
> +static void loop_del_one(struct loop_device *lo)
> {
> -    int    i;
> +    del_gendisk(lo->lo_disk);
> +    blk_cleanup_queue(lo->lo_queue);
> +    put_disk(lo->lo_disk);
> +    list_del(&lo->lo_list);
> +    kfree(lo);
> +}
> 
> -    if (max_loop < 1 || max_loop > 256) {
> -        printk(KERN_WARNING "loop: invalid max_loop (must be between"
> -                    " 1 and 256), using default (8)\n");
> -        max_loop = 8;
> -    }
> +static struct kobject *loop_probe(dev_t dev, int *part, void *data)
> +{
> +    unsigned int number = dev & MINORMASK;
> +    struct loop_device *lo;
> +
> +    mutex_lock(&loop_devices_mutex);
> +    lo = loop_find_dev(number);
> +    if (lo == NULL)
> +        lo = loop_init_one(number);
> +    mutex_unlock(&loop_devices_mutex);
> +
> +    *part = 0;
> +    if (IS_ERR(lo))
> +        return (void *)lo;
> +    else
> +        return &lo->lo_disk->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);
> 
> -    loop_dev = kmalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
> -    if (!loop_dev)
> -        goto out_mem1;
> -    memset(loop_dev, 0, max_loop * sizeof(struct loop_device));
> +    lo = loop_init_one(0);
> +    if (IS_ERR(lo))
> +        goto out;
> 
> -    disks = kmalloc(max_loop * sizeof(struct gendisk *), GFP_KERNEL);
> -    if (!disks)
> -        goto out_mem2;
> +    if (max_loop) {
> +        printk(KERN_INFO "loop: the max_loop option is obsolete "
> +                 "and will be removed in March 2008\n");
> 
> -    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]);
> -    printk(KERN_INFO "loop: loaded (max %d devices)\n", max_loop);
> +    printk(KERN_INFO "loop: module loaded\n");
>     return 0;
> 
> -out_mem4:
> -    while (i--)
> -        blk_cleanup_queue(loop_dev[i].lo_queue);
> -    i = max_loop;
> -out_mem3:
> -    while (i--)
> -        put_disk(disks[i]);
> -    kfree(disks);
> -out_mem2:
> -    kfree(loop_dev);
> -out_mem1:
> +out:
>     unregister_blkdev(LOOP_MAJOR, "loop");
>     printk(KERN_ERR "loop: ran out of memory\n");
>     return -ENOMEM;
> }
> 
> -static void loop_exit(void)
> +static void __exit loop_exit(void)
> {
> -    int i;
> +    struct loop_device *lo, *next;
> 
> -    for (i = 0; i < max_loop; i++) {
> -        del_gendisk(disks[i]);
> -        blk_cleanup_queue(loop_dev[i].lo_queue);
> -        put_disk(disks[i]);
> -    }
> +    list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
> +        loop_del_one(lo);
> +
> +    blk_unregister_region(MKDEV(LOOP_MAJOR, 0), 1UL << MINORBITS);
>     if (unregister_blkdev(LOOP_MAJOR, "loop"))
>         printk(KERN_WARNING "loop: cannot unregister blkdev\n");
> -
> -    kfree(disks);
> -    kfree(loop_dev);
> }
> 
> module_init(loop_init);
> diff --git a/include/linux/loop.h b/include/linux/loop.h
> index 191a595..0b99b31 100644
> --- a/include/linux/loop.h
> +++ b/include/linux/loop.h
> @@ -64,6 +64,8 @@ struct loop_device {
>     wait_queue_head_t    lo_event;
> 
>     request_queue_t        *lo_queue;
> +    struct gendisk        *lo_disk;
> +    struct list_head    lo_list;
> };
> 
> #endif /* __KERNEL__ */
> 
> 
> ------------------------------------------------------------------------
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 6b5b642..605c1d3 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -77,9 +77,8 @@
>  
>  #include <asm/uaccess.h>
>  
> -static int max_loop = 8;
> -static struct loop_device *loop_dev;
> -static struct gendisk **disks;
> +static LIST_HEAD(loop_devices);
> +static DEFINE_MUTEX(loop_devices_mutex);
>  
>  /*
>   * Transfer functions
> @@ -183,7 +182,7 @@ figure_loop_size(struct loop_device *lo)
>  	if (unlikely((loff_t)x != size))
>  		return -EFBIG;
>  
> -	set_capacity(disks[lo->lo_number], x);
> +	set_capacity(lo->lo_disk, x);
>  	return 0;					
>  }
>  
> @@ -812,7 +811,7 @@ static int loop_set_fd
>  	lo->lo_queue->queuedata = lo;
>  	lo->lo_queue->unplug_fn = loop_unplug;
>  
> -	set_capacity(disks[lo->lo_number], size);
> +	set_capacity(lo->lo_disk, size);
>  	bd_set_size(bdev, size << 9);
>  
>  	set_blocksize(bdev, lo_blocksize);
> @@ -832,7 +831,7 @@ out_clr:
>  	lo->lo_device = NULL;
>  	lo->lo_backing_file = NULL;
>  	lo->lo_flags = 0;
> -	set_capacity(disks[lo->lo_number], 0);
> +	set_capacity(lo->lo_disk, 0);
>  	invalidate_bdev(bdev, 0);
>  	bd_set_size(bdev, 0);
>  	mapping_set_gfp_mask(mapping, lo->old_gfp_mask);
> @@ -918,7 +917,7 @@ static int loop_clr_fd
>  	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
>  	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
>  	invalidate_bdev(bdev, 0);
> -	set_capacity(disks[lo->lo_number], 0);
> +	set_capacity(lo->lo_disk, 0);
>  	bd_set_size(bdev, 0);
>  	mapping_set_gfp_mask(filp->f_mapping, gfp);
>  	lo->lo_state = Lo_unbound;
> @@ -1322,6 +1321,18 @@ static long lo_compat_ioctl
>  }
>  #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;
> @@ -1330,6 +1341,11 @@ static int lo_open
>  	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;
>  }
>  
> @@ -1357,8 +1373,9 @@ static struct block_device_operations lo_fops = {
>  /*
>   * And now the modules code and kernel interface.
>   */
> +static int max_loop;
>  module_param(max_loop, int, 0);
> -MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-256)");
> +MODULE_PARM_DESC(max_loop, "obsolete, loop device is created on-demand");
>  MODULE_LICENSE("GPL");
>  MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);
>  
> @@ -1383,7 +1400,7 @@ int loop_unregister_transfer(int number)
>  
>  	xfer_funcs[n] = NULL;
>  
> -	for (lo = &loop_dev[0]; lo < &loop_dev[max_loop]; lo++) {
> +	list_for_each_entry(lo, &loop_devices, lo_list) {
>  		mutex_lock(&lo->lo_ctl_mutex);
>  
>  		if (lo->lo_encryption == xfer)
> @@ -1398,91 +1415,110 @@ int loop_unregister_transfer(int number)
>  EXPORT_SYMBOL(loop_register_transfer);
>  EXPORT_SYMBOL(loop_unregister_transfer);
>  
> -static int __init loop_init(void)
> +static struct loop_device *loop_init_one(int i)
> +{
> +	struct loop_device *lo;
> +	struct gendisk *disk;
> +
> +	lo = kzalloc(sizeof(*lo), GFP_KERNEL);
> +	if (!lo)
> +		goto out;
> +
> +	lo->lo_queue = blk_alloc_queue(GFP_KERNEL);
> +	if (!lo->lo_queue)
> +		goto out_free_dev;
> +
> +	disk = lo->lo_disk = alloc_disk(1);
> +	if (!disk)
> +		goto out_free_queue;
> +
> +	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;
> +	disk->private_data	= lo;
> +	disk->queue		= lo->lo_queue;
> +	sprintf(disk->disk_name, "loop%d", i);
> +	add_disk(disk);
> +	list_add_tail(&lo->lo_list, &loop_devices);
> +	return lo;
> +
> +out_free_queue:
> +	blk_cleanup_queue(lo->lo_queue);
> +out_free_dev:
> +	kfree(lo);
> +out:
> +	return ERR_PTR(-ENOMEM);
> +}
> +
> +static void loop_del_one(struct loop_device *lo)
>  {
> -	int	i;
> +	del_gendisk(lo->lo_disk);
> +	blk_cleanup_queue(lo->lo_queue);
> +	put_disk(lo->lo_disk);
> +	list_del(&lo->lo_list);
> +	kfree(lo);
> +}
>  
> -	if (max_loop < 1 || max_loop > 256) {
> -		printk(KERN_WARNING "loop: invalid max_loop (must be between"
> -				    " 1 and 256), using default (8)\n");
> -		max_loop = 8;
> -	}
> +static struct kobject *loop_probe(dev_t dev, int *part, void *data)
> +{
> +	unsigned int number = dev & MINORMASK;
> +	struct loop_device *lo;
> +
> +	mutex_lock(&loop_devices_mutex);
> +	lo = loop_find_dev(number);
> +	if (lo == NULL)
> +		lo = loop_init_one(number);
> +	mutex_unlock(&loop_devices_mutex);
> +
> +	*part = 0;
> +	if (IS_ERR(lo))
> +		return (void *)lo;
> +	else
> +		return &lo->lo_disk->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);
>  
> -	loop_dev = kmalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
> -	if (!loop_dev)
> -		goto out_mem1;
> -	memset(loop_dev, 0, max_loop * sizeof(struct loop_device));
> +	lo = loop_init_one(0);
> +	if (IS_ERR(lo))
> +		goto out;
>  
> -	disks = kmalloc(max_loop * sizeof(struct gendisk *), GFP_KERNEL);
> -	if (!disks)
> -		goto out_mem2;
> +	if (max_loop) {
> +		printk(KERN_INFO "loop: the max_loop option is obsolete "
> +				 "and will be removed in March 2008\n");
>  
> -	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]);
> -	printk(KERN_INFO "loop: loaded (max %d devices)\n", max_loop);
> +	printk(KERN_INFO "loop: module loaded\n");
>  	return 0;
>  
> -out_mem4:
> -	while (i--)
> -		blk_cleanup_queue(loop_dev[i].lo_queue);
> -	i = max_loop;
> -out_mem3:
> -	while (i--)
> -		put_disk(disks[i]);
> -	kfree(disks);
> -out_mem2:
> -	kfree(loop_dev);
> -out_mem1:
> +out:
>  	unregister_blkdev(LOOP_MAJOR, "loop");
>  	printk(KERN_ERR "loop: ran out of memory\n");
>  	return -ENOMEM;
>  }
>  
> -static void loop_exit(void)
> +static void __exit loop_exit(void)
>  {
> -	int i;
> +	struct loop_device *lo, *next;
>  
> -	for (i = 0; i < max_loop; i++) {
> -		del_gendisk(disks[i]);
> -		blk_cleanup_queue(loop_dev[i].lo_queue);
> -		put_disk(disks[i]);
> -	}
> +	list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
> +		loop_del_one(lo);
> +
> +	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), 1UL << MINORBITS);
>  	if (unregister_blkdev(LOOP_MAJOR, "loop"))
>  		printk(KERN_WARNING "loop: cannot unregister blkdev\n");
> -
> -	kfree(disks);
> -	kfree(loop_dev);
>  }
>  
>  module_init(loop_init);
> diff --git a/include/linux/loop.h b/include/linux/loop.h
> index 191a595..0b99b31 100644
> --- a/include/linux/loop.h
> +++ b/include/linux/loop.h
> @@ -64,6 +64,8 @@ struct loop_device {
>  	wait_queue_head_t	lo_event;
>  
>  	request_queue_t		*lo_queue;
> +	struct gendisk		*lo_disk;
> +	struct list_head	lo_list;
>  };
>  
>  #endif /* __KERNEL__ */

Re: [patch] remove artificial software max_loop limit

[Andrew Morton]

On Wed, 04 Apr 2007 12:31:25 +0200 Tomas M <tomas@slax.org> wrote:

>  > OK, here is a re-spin patch that I tested as module or
>  > link-in-vmlinux.  Both produce satisfactory result for me.
> 
> Is there a plan to include this brilliant code in mainline Kernel?
> It works excellent, tested with 15000 loop devices, it's simply cool.

I ecpect it will join all the other brilliant code in 2.6.22 ;)

Re: [patch] remove artificial software max_loop limit

[Tomas M]

Andrew Morton wrote:
> On Fri, 30 Mar 2007 02:25:37 -0700
> "Ken Chen" <kenchen@google.com> wrote:
> 
>> -module_param(max_loop, int, 0);
>> -MODULE_PARM_DESC(max_loop, "Maximum number of loop devices (1-256)");
> 
> So..  this change will cause a fatal error for anyone who is presently
> using max_loop, won't it?  If they're doing that within their
> initramfs/initrd/etc then things could get rather ugly for them.

I consider myself the most precious user of max_loop.

The max_loop parameter would cause a fatal error only in the case if you 
modprobe loop manually, for example:

  $ modprobe loop max_loop=200

But people don't usually use this, read below.

> I don't know how much of a problem this will be in practice - do
> people use max_loop much?

yes, but no as a module parameter.

People usually use max_loop as a 'kernel boot parameter' passed in 
APPEND section in a boot loader (such as LILO for example), not as a 
parameter for module in initrd. Why? Because it's easier; people are 
lazy, people compile loop.c into kernel so they don't need to update the 
loop.ko module in initrd every time a new Kernel is released.

I believe that IF you _really_ need to preserve the boot parameter, then 
the parameter should _not_ be ignored, rather it should have the same 
function like before - to limit the loop driver so if you use 
max_loop=10 for example, it should not allow loop.c to create more than 
10 loops.

And if no parameter is used at all, there will be unlimited amount of 
loops. Simply clever :)

This will make it _completely_ backward-compatible, with very small code 
change I guess.

Just my two cents.

Thank you for reading so far.

Tomas M
slax.org

Re: [patch] remove artificial software max_loop limit

[Tomas M]

I'm sorry I made a mistake,

there should be
> module parameter

instead of
 > boot parameter.

I am sorry.
The entire paragraph in my previous post should be the following:

I believe that IF you _really_ need to preserve the max_loop module 
parameter, then the parameter should _not_ be ignored, rather it should 
have the same function like before - to limit the loop driver so if you 
use max_loop=10 for example, it should not allow loop.c to create more 
than 10 loops.


Tomas M
slax.org

Re: [patch] remove artificial software max_loop limit

[Ken Chen]

On 4/1/07, Tomas M <tomas@slax.org> wrote:
> I believe that IF you _really_ need to preserve the max_loop module
> parameter, then the parameter should _not_ be ignored, rather it should
> have the same function like before - to limit the loop driver so if you
> use max_loop=10 for example, it should not allow loop.c to create more
> than 10 loops.

Blame on the dual meaning of max_loop that it uses currently: to
initialize a set of loop devices and as a side effect, it also sets
the upper limit.  People are complaining about the former constrain,
isn't it?  Does anyone uses the 2nd meaning of upper limit?

- Ken

Re: [patch] remove artificial software max_loop limit

[Jan Engelhardt]

On Apr 1 2007 11:10, Ken Chen wrote:
> On 4/1/07, Tomas M <tomas@slax.org> wrote:
>
>> I believe that IF you _really_ need to preserve the max_loop module 
>> parameter, then the parameter should _not_ be ignored, rather it 
>> should have the same function like before - to limit the loop driver 
>> so if you use max_loop=10 for example, it should not allow loop.c to 
>> create more than 10 loops.
>
> Blame on the dual meaning of max_loop that it uses currently: to 
> initialize a set of loop devices and as a side effect, it also sets 
> the upper limit.  People are complaining about the former constrain, 
> isn't it?  Does anyone uses the 2nd meaning of upper limit?

Who cares if the user specifies max_loop=8 but still is able to open up 
/dev/loop8, loop9, etc.? max_loop=X basically meant (at least to me) 
"have at least X" loops ready.


Jan
-- 

Re: [patch] remove artificial software max_loop limit

[Bill Davidsen]

Jan Engelhardt wrote:
> On Apr 1 2007 11:10, Ken Chen wrote:
>> On 4/1/07, Tomas M <tomas@slax.org> wrote:
>>
>>> I believe that IF you _really_ need to preserve the max_loop module 
>>> parameter, then the parameter should _not_ be ignored, rather it 
>>> should have the same function like before - to limit the loop driver 
>>> so if you use max_loop=10 for example, it should not allow loop.c to 
>>> create more than 10 loops.
>> Blame on the dual meaning of max_loop that it uses currently: to 
>> initialize a set of loop devices and as a side effect, it also sets 
>> the upper limit.  People are complaining about the former constrain, 
>> isn't it?  Does anyone uses the 2nd meaning of upper limit?
> 
> Who cares if the user specifies max_loop=8 but still is able to open up 
> /dev/loop8, loop9, etc.? max_loop=X basically meant (at least to me) 
> "have at least X" loops ready.
> 
You have just come up with a really good reason not to do unlimited 
loops. With the current limit people can count on a script mounting 
files, or similar, to neither loop for a VERY long time or to eat their 
memory. Whatever you think of programs without limit checking, this 
falls in the range of expecting an unsigned char to have a certain upper 
bound, and argues that the default limit should be the current limit and 
that setting a lower bound should work as a real and enforced limit.

If a new capability is being added, and I think it's a great one, then 
people using the capability should be the ones explicitly doing 
something different. Plauger's law of least astonishment.

-- 
Bill Davidsen <davidsen@tmr.com>
   "We have more to fear from the bungling of the incompetent than from
the machinations of the wicked."  - from Slashdot

Re: [patch] remove artificial software max_loop limit

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On Fri, 06 Apr 2007 16:33:32 EDT, Bill Davidsen said:
> Jan Engelhardt wrote:

> > Who cares if the user specifies max_loop=8 but still is able to open up 
> > /dev/loop8, loop9, etc.? max_loop=X basically meant (at least to me) 
> > "have at least X" loops ready.
> > 
> You have just come up with a really good reason not to do unlimited 
> loops.

That, and I'd expect the intuitive name for "have at least N ready" to
be 'min_loop=N'.  'max_loop=N' means (to me, at least) "If I ask for N+1,
something has obviously gone very wrong, so please shoot my process before
it gets worse".

Maybe what's needed is *both* a max_ and min_ parameter?

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [patch] remove artificial software max_loop limit

[Bill Davidsen]

Valdis.Kletnieks@vt.edu wrote:
> On Fri, 06 Apr 2007 16:33:32 EDT, Bill Davidsen said:
>   
>> Jan Engelhardt wrote:
>>     
>
>   
>>> Who cares if the user specifies max_loop=8 but still is able to open up 
>>> /dev/loop8, loop9, etc.? max_loop=X basically meant (at least to me) 
>>> "have at least X" loops ready.
>>>
>>>       
>> You have just come up with a really good reason not to do unlimited 
>> loops.
>>     
>
> That, and I'd expect the intuitive name for "have at least N ready" to
> be 'min_loop=N'.  'max_loop=N' means (to me, at least) "If I ask for N+1,
> something has obviously gone very wrong, so please shoot my process before
> it gets worse".
>
> Maybe what's needed is *both* a max_ and min_ parameter?
>   
I think that max_loop is a sufficient statement of the highest number of 
devices needed, and can reasonably interpreted as both "I may need this 
many" and "I won't legitimately want more."

As I recall memory is allocated as the device is set up, so unless you 
want to use the max memory at boot, "just in case," the minimum won't be 
guaranteed anyway. Something else could eat memory.

In practice I think asking for way too many is more common than not 
being able to get to the max. It may happen but it's a corner case, and 
status is returned.

-- 
bill davidsen <davidsen@tmr.com>
  CTO TMR Associates, Inc
  Doing interesting things with small computers since 1979

Re: [patch] remove artificial software max_loop limit

[Andrew Morton]

ahem.

On Fri, 30 Mar 2007 02:25:37 -0700
"Ken Chen" <kenchen@google.com> wrote:

> +static DEFINE_MUTEX(loop_devices_mutex);
> ...
> +	mutex_lock(&loop_device_mutex);

which makes me suspect that you didn't send the patch which you meant to
send, so I'll drop it.

Re: [patch] remove artificial software max_loop limit

[Jan Engelhardt]

On Mar 30 2007 14:46, Andrew Morton wrote:
>
>ahem.
>
>On Fri, 30 Mar 2007 02:25:37 -0700
>"Ken Chen" <kenchen@google.com> wrote:
>
>> +static DEFINE_MUTEX(loop_devices_mutex);
>> ...
>> +	mutex_lock(&loop_device_mutex);
>
>which makes me suspect that you didn't send the patch which you meant to
>send, so I'll drop it.

/me smells plagiarism :)


Jan
--