Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

[Al Viro]

On Mon, May 21, 2007 at 03:00:55PM -0700, akpm@linux-foundation.org wrote:
> +	if (register_blkdev(LOOP_MAJOR, "loop"))
> +		return -EIO;
> +	blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
> +				  THIS_MODULE, loop_probe, NULL, NULL);
> +
> +	for (i = 0; i < nr; i++) {
> +		if (!loop_init_one(i))
> +			goto err;
> +	}
> +
> +	printk(KERN_INFO "loop: module loaded\n");
> +	return 0;
> +err:
> +	loop_exit();

This isn't good.  You *can't* fail once a single disk has been registered.
Anyone could've opened it by now.

IOW, you need to
	* register region *after* you are past the point of no return
	* either not fail on failing loop_init_one() here, or separate
allocations and actual add_disk().

Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

[Ken Chen]

On 5/21/07, Al Viro <viro@ftp.linux.org.uk> wrote:
> On Mon, May 21, 2007 at 03:00:55PM -0700, akpm@linux-foundation.org wrote:
> > +     if (register_blkdev(LOOP_MAJOR, "loop"))
> > +             return -EIO;
> > +     blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
> > +                               THIS_MODULE, loop_probe, NULL, NULL);
> > +
> > +     for (i = 0; i < nr; i++) {
> > +             if (!loop_init_one(i))
> > +                     goto err;
> > +     }
> > +
> > +     printk(KERN_INFO "loop: module loaded\n");
> > +     return 0;
> > +err:
> > +     loop_exit();
>
> This isn't good.  You *can't* fail once a single disk has been registered.
> Anyone could've opened it by now.
>
> IOW, you need to
>         * register region *after* you are past the point of no return

That option is a lot harder than I thought.  This requires an array to
keep intermediate result of preallocated "lo" device, blk_queue, and
disk structure before calling add_disk() or register region.  And this
array could be potentially 1 million entries.  Maybe I will use
vmalloc for it, but seems rather sick.

Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

[Al Viro]

On Mon, May 21, 2007 at 06:30:15PM -0700, Ken Chen wrote:
> On 5/21/07, Al Viro <viro@ftp.linux.org.uk> wrote:
> >On Mon, May 21, 2007 at 03:00:55PM -0700, akpm@linux-foundation.org wrote:
> >> +     if (register_blkdev(LOOP_MAJOR, "loop"))
> >> +             return -EIO;
> >> +     blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
> >> +                               THIS_MODULE, loop_probe, NULL, NULL);
> >> +
> >> +     for (i = 0; i < nr; i++) {
> >> +             if (!loop_init_one(i))
> >> +                     goto err;
> >> +     }
> >> +
> >> +     printk(KERN_INFO "loop: module loaded\n");
> >> +     return 0;
> >> +err:
> >> +     loop_exit();
> >
> >This isn't good.  You *can't* fail once a single disk has been registered.
> >Anyone could've opened it by now.
> >
> >IOW, you need to
> >        * register region *after* you are past the point of no return
> 
> That option is a lot harder than I thought.  This requires an array to
> keep intermediate result of preallocated "lo" device, blk_queue, and
> disk structure before calling add_disk() or register region.  And this
> array could be potentially 1 million entries.  Maybe I will use
> vmalloc for it, but seems rather sick.

No, it doesn't.  Really.  It's easy to split; untested incremental to your
patch follows:

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 0aae8d8..2300490 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1394,16 +1394,11 @@ int loop_unregister_transfer(int number)
 EXPORT_SYMBOL(loop_register_transfer);
 EXPORT_SYMBOL(loop_unregister_transfer);
 
-static struct loop_device *loop_init_one(int i)
+static struct loop_device *loop_alloc(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;
@@ -1427,8 +1422,6 @@ static struct loop_device *loop_init_one(int i)
 	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:
@@ -1439,6 +1432,23 @@ out:
 	return NULL;
 }
 
+static struct loop_device *loop_init_one(int i)
+{
+	struct loop_device *lo;
+
+	list_for_each_entry(lo, &loop_devices, lo_list) {
+		if (lo->lo_number == i)
+			return lo;
+	}
+
+	lo = loop_alloc(i);
+	if (lo) {
+		add_disk(lo->lo_disk);
+		list_add_tail(&lo->lo_list, &loop_devices);
+	}
+	return lo;
+}
+
 static void loop_del_one(struct loop_device *lo)
 {
 	del_gendisk(lo->lo_disk);
@@ -1481,6 +1491,7 @@ static int __init loop_init(void)
 {
 	int i, nr;
 	unsigned long range;
+	struct loop_device *lo;
 
 	/*
 	 * loop module now has a feature to instantiate underlying device
@@ -1506,19 +1517,34 @@ static int __init loop_init(void)
 
 	if (register_blkdev(LOOP_MAJOR, "loop"))
 		return -EIO;
-	blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
-				  THIS_MODULE, loop_probe, NULL, NULL);
 
 	for (i = 0; i < nr; i++) {
-		if (!loop_init_one(i))
-			goto err;
+		lo = loop_alloc(i);
+		if (!lo)
+			goto Enomem;
+		list_add_tail(&lo->lo_list, &loop_devices);
 	}
 
+	/* point of no return */
+
+	list_for_each_entry(lo, &loop_devices, lo_list)
+		add_disk(lo->lo_disk);
+
+	blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
+				  THIS_MODULE, loop_probe, NULL, NULL);
+
 	printk(KERN_INFO "loop: module loaded\n");
 	return 0;
-err:
-	loop_exit();
+
+Enomem:
 	printk(KERN_INFO "loop: out of memory\n");
+
+	while(!list_empty(&loop_devices)) {
+		lo = list_entry(loop_devices.next, struct loop_device, lo_list);
+		loop_del_one(lo);
+	}
+
+	unregister_blkdev(LOOP_MAJOR, "loop");
 	return -ENOMEM;
 }
 

Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

[Ken Chen]

On 5/21/07, Al Viro <viro@ftp.linux.org.uk> wrote:
> No, it doesn't.  Really.  It's easy to split; untested incremental to your
> patch follows:
>
>         for (i = 0; i < nr; i++) {
> -               if (!loop_init_one(i))
> -                       goto err;
> +               lo = loop_alloc(i);
> +               if (!lo)
> +                       goto Enomem;
> +               list_add_tail(&lo->lo_list, &loop_devices);
>         }

ah, yes, use the loop_device list_head to link all the pending devices.


> +       /* point of no return */
> +
> +       list_for_each_entry(lo, &loop_devices, lo_list)
> +               add_disk(lo->lo_disk);
> +
> +       blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
> +                                 THIS_MODULE, loop_probe, NULL, NULL);
> +
>         printk(KERN_INFO "loop: module loaded\n");
>         return 0;
> -err:
> -       loop_exit();
> +
> +Enomem:
>         printk(KERN_INFO "loop: out of memory\n");
> +
> +       while(!list_empty(&loop_devices)) {
> +               lo = list_entry(loop_devices.next, struct loop_device, lo_list);
> +               loop_del_one(lo);
> +       }
> +
> +       unregister_blkdev(LOOP_MAJOR, "loop");
>         return -ENOMEM;
>  }

I suppose the loop_del_one call in Enomem label needs to be split up
too since in the error path, it hasn't done add_disk() yet?

Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

[Ken Chen]

On 5/21/07, Ken Chen <kenchen@google.com> wrote:
> On 5/21/07, Al Viro <viro@ftp.linux.org.uk> wrote:
> > No, it doesn't.  Really.  It's easy to split; untested incremental to your
> > patch follows:
> >
> >         for (i = 0; i < nr; i++) {
> > -               if (!loop_init_one(i))
> > -                       goto err;
> > +               lo = loop_alloc(i);
> > +               if (!lo)
> > +                       goto Enomem;
> > +               list_add_tail(&lo->lo_list, &loop_devices);
> >         }
>
> ah, yes, use the loop_device list_head to link all the pending devices.
>
>
> > +       /* point of no return */
> > +
> > +       list_for_each_entry(lo, &loop_devices, lo_list)
> > +               add_disk(lo->lo_disk);
> > +
> > +       blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
> > +                                 THIS_MODULE, loop_probe, NULL, NULL);
> > +
> >         printk(KERN_INFO "loop: module loaded\n");
> >         return 0;
> > -err:
> > -       loop_exit();
> > +
> > +Enomem:
> >         printk(KERN_INFO "loop: out of memory\n");
> > +
> > +       while(!list_empty(&loop_devices)) {
> > +               lo = list_entry(loop_devices.next, struct loop_device, lo_list);
> > +               loop_del_one(lo);
> > +       }
> > +
> > +       unregister_blkdev(LOOP_MAJOR, "loop");
> >         return -ENOMEM;
> >  }
>
> I suppose the loop_del_one call in Enomem label needs to be split up
> too since in the error path, it hasn't done add_disk() yet?


tested, like this?

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 5526ead..0ed5470 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1354,7 +1354,7 @@ #endif
  */
 static int max_loop;
 module_param(max_loop, int, 0);
-MODULE_PARM_DESC(max_loop, "obsolete, loop device is created on-demand");
+MODULE_PARM_DESC(max_loop, "Maximum number of loop devices");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);

@@ -1394,16 +1394,11 @@ int loop_unregister_transfer
 EXPORT_SYMBOL(loop_register_transfer);
 EXPORT_SYMBOL(loop_unregister_transfer);

-static struct loop_device *loop_init_one(int i)
+static struct loop_device *loop_alloc(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;
@@ -1427,8 +1422,6 @@ static struct loop_device *loop_init_one
 	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:
@@ -1439,15 +1432,37 @@ out:
 	return NULL;
 }

-static void loop_del_one(struct loop_device *lo)
+static void loop_free(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);
 }

+static struct loop_device *loop_init_one(int i)
+{
+	struct loop_device *lo;
+
+	list_for_each_entry(lo, &loop_devices, lo_list) {
+		if (lo->lo_number == i)
+			return lo;
+	}
+
+	lo = loop_alloc(i);
+	if (lo) {
+		add_disk(lo->lo_disk);
+		list_add_tail(&lo->lo_list, &loop_devices);
+	}
+	return lo;
+}
+
+static void loop_del_one(struct loop_device *lo)
+{
+	del_gendisk(lo->lo_disk);
+	loop_free(lo);
+}
+
 static struct kobject *loop_probe(dev_t dev, int *part, void *data)
 {
 	struct loop_device *lo;
@@ -1464,28 +1479,77 @@ static struct kobject *loop_probe

 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, NULL, NULL);
+	int i, nr;
+	unsigned long range;
+	struct loop_device *lo, *next;
+
+	/*
+	 * loop module now has a feature to instantiate underlying device
+	 * structure on-demand, provided that there is an access dev node.
+	 * However, this will not work well with user space tool that doesn't
+	 * know about such "feature".  In order to not break any existing
+	 * tool, we do the following:
+	 *
+	 * (1) if max_loop is specified, create that many upfront, and this
+	 *     also becomes a hard limit.
+	 * (2) if max_loop is not specified, create 8 loop device on module
+	 *     load, user can further extend loop device by create dev node
+	 *     themselves and have kernel automatically instantiate actual
+	 *     device on-demand.
+	 */
+	if (max_loop > 1UL << MINORBITS)
+		return -EINVAL;

 	if (max_loop) {
-		printk(KERN_INFO "loop: the max_loop option is obsolete "
-				 "and will be removed in March 2008\n");
+		nr = max_loop;
+		range = max_loop;
+	} else {
+		nr = 8;
+		range = 1UL << MINORBITS;
+	}
+
+	if (register_blkdev(LOOP_MAJOR, "loop"))
+		return -EIO;

+	for (i = 0; i < nr; i++) {
+		lo = loop_alloc(i);
+		if (!lo)
+			goto Enomem;
+		list_add_tail(&lo->lo_list, &loop_devices);
 	}
+
+	/* point of no return */
+
+	list_for_each_entry(lo, &loop_devices, lo_list)
+		add_disk(lo->lo_disk);
+
+	blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
+				  THIS_MODULE, loop_probe, NULL, NULL);
+
 	printk(KERN_INFO "loop: module loaded\n");
 	return 0;
+
+Enomem:
+	printk(KERN_INFO "loop: out of memory\n");
+
+	list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
+		loop_free(lo);
+
+	unregister_blkdev(LOOP_MAJOR, "loop");
+	return -ENOMEM;
 }

 static void __exit loop_exit(void)
 {
+	unsigned long range;
 	struct loop_device *lo, *next;

+	range = max_loop ? max_loop :  1UL << MINORBITS;
+
 	list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
 		loop_del_one(lo);

-	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), 1UL << MINORBITS);
+	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), range);
 	if (unregister_blkdev(LOOP_MAJOR, "loop"))
 		printk(KERN_WARNING "loop: cannot unregister blkdev\n");
 }

Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

[Al Viro]

On Mon, May 21, 2007 at 07:40:14PM -0700, Ken Chen wrote:
> tested, like this?
 
ACK.  Could merge loop_init_one() into the only remaining caller,
but it won't make the code simpler, so let's leave it at that.

Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

[walt]

Ken Chen wrote:
> On 5/21/07, Ken Chen <kenchen@google.com> wrote:
...
> tested, like this?

Ken, your patch below works for me.  Are you going to send this
on to Linus?

> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 5526ead..0ed5470 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1354,7 +1354,7 @@ #endif
>  */
> static int max_loop;
> module_param(max_loop, int, 0);
> -MODULE_PARM_DESC(max_loop, "obsolete, loop device is created on-demand");
> +MODULE_PARM_DESC(max_loop, "Maximum number of loop devices");
> MODULE_LICENSE("GPL");
> MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);
> 
> @@ -1394,16 +1394,11 @@ int loop_unregister_transfer
> EXPORT_SYMBOL(loop_register_transfer);
> EXPORT_SYMBOL(loop_unregister_transfer);
> 
> -static struct loop_device *loop_init_one(int i)
> +static struct loop_device *loop_alloc(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;
> @@ -1427,8 +1422,6 @@ static struct loop_device *loop_init_one
>     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:
> @@ -1439,15 +1432,37 @@ out:
>     return NULL;
> }
> 
> -static void loop_del_one(struct loop_device *lo)
> +static void loop_free(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);
> }
> 
> +static struct loop_device *loop_init_one(int i)
> +{
> +    struct loop_device *lo;
> +
> +    list_for_each_entry(lo, &loop_devices, lo_list) {
> +        if (lo->lo_number == i)
> +            return lo;
> +    }
> +
> +    lo = loop_alloc(i);
> +    if (lo) {
> +        add_disk(lo->lo_disk);
> +        list_add_tail(&lo->lo_list, &loop_devices);
> +    }
> +    return lo;
> +}
> +
> +static void loop_del_one(struct loop_device *lo)
> +{
> +    del_gendisk(lo->lo_disk);
> +    loop_free(lo);
> +}
> +
> static struct kobject *loop_probe(dev_t dev, int *part, void *data)
> {
>     struct loop_device *lo;
> @@ -1464,28 +1479,77 @@ static struct kobject *loop_probe
> 
> 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, NULL, NULL);
> +    int i, nr;
> +    unsigned long range;
> +    struct loop_device *lo, *next;
> +
> +    /*
> +     * loop module now has a feature to instantiate underlying device
> +     * structure on-demand, provided that there is an access dev node.
> +     * However, this will not work well with user space tool that doesn't
> +     * know about such "feature".  In order to not break any existing
> +     * tool, we do the following:
> +     *
> +     * (1) if max_loop is specified, create that many upfront, and this
> +     *     also becomes a hard limit.
> +     * (2) if max_loop is not specified, create 8 loop device on module
> +     *     load, user can further extend loop device by create dev node
> +     *     themselves and have kernel automatically instantiate actual
> +     *     device on-demand.
> +     */
> +    if (max_loop > 1UL << MINORBITS)
> +        return -EINVAL;
> 
>     if (max_loop) {
> -        printk(KERN_INFO "loop: the max_loop option is obsolete "
> -                 "and will be removed in March 2008\n");
> +        nr = max_loop;
> +        range = max_loop;
> +    } else {
> +        nr = 8;
> +        range = 1UL << MINORBITS;
> +    }
> +
> +    if (register_blkdev(LOOP_MAJOR, "loop"))
> +        return -EIO;
> 
> +    for (i = 0; i < nr; i++) {
> +        lo = loop_alloc(i);
> +        if (!lo)
> +            goto Enomem;
> +        list_add_tail(&lo->lo_list, &loop_devices);
>     }
> +
> +    /* point of no return */
> +
> +    list_for_each_entry(lo, &loop_devices, lo_list)
> +        add_disk(lo->lo_disk);
> +
> +    blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
> +                  THIS_MODULE, loop_probe, NULL, NULL);
> +
>     printk(KERN_INFO "loop: module loaded\n");
>     return 0;
> +
> +Enomem:
> +    printk(KERN_INFO "loop: out of memory\n");
> +
> +    list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
> +        loop_free(lo);
> +
> +    unregister_blkdev(LOOP_MAJOR, "loop");
> +    return -ENOMEM;
> }
> 
> static void __exit loop_exit(void)
> {
> +    unsigned long range;
>     struct loop_device *lo, *next;
> 
> +    range = max_loop ? max_loop :  1UL << MINORBITS;
> +
>     list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
>         loop_del_one(lo);
> 
> -    blk_unregister_region(MKDEV(LOOP_MAJOR, 0), 1UL << MINORBITS);
> +    blk_unregister_region(MKDEV(LOOP_MAJOR, 0), range);
>     if (unregister_blkdev(LOOP_MAJOR, "loop"))
>         printk(KERN_WARNING "loop: cannot unregister blkdev\n");
> }

Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

[Ken Chen]

On 5/31/07, walt <wa1ter@myrealbox.com> wrote:
> Ken Chen wrote:
> > On 5/21/07, Ken Chen <kenchen@google.com> wrote:
> ...
> > tested, like this?
>
> Ken, your patch below works for me.  Are you going to send this
> on to Linus?

I think akpm will route this to Linus.  andrew?

Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

[Andrew Morton]

On Thu, 31 May 2007 09:51:36 -0700
"Ken Chen" <kenchen@google.com> wrote:

> On 5/31/07, walt <wa1ter@myrealbox.com> wrote:
> > Ken Chen wrote:
> > > On 5/21/07, Ken Chen <kenchen@google.com> wrote:
> > ...
> > > tested, like this?
> >
> > Ken, your patch below works for me.  Are you going to send this
> > on to Linus?
> 
> I think akpm will route this to Linus.  andrew?

I have a note here that it needs additional work.  This discussion:

http://lkml.org/lkml/2007/5/21/602

seemed to peter out and go nowhere?

Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

[Ken Chen]

On 5/31/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> I have a note here that it needs additional work.  This discussion:
>
> http://lkml.org/lkml/2007/5/21/602
>
> seemed to peter out and go nowhere?

The first rev went in -mm needs work and the above url is the result
of feedback from Al Viro.  He also acked the patch in the follow on
thread:
http://lkml.org/lkml/2007/5/22/5

Not sure how widely it was tested on that last patch, I see a few
reports that the patch works out OK.

Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

[Andrew Morton]

On Thu, 31 May 2007 11:23:32 -0700
"Ken Chen" <kenchen@google.com> wrote:

> On 5/31/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> > I have a note here that it needs additional work.  This discussion:
> >
> > http://lkml.org/lkml/2007/5/21/602
> >
> > seemed to peter out and go nowhere?
> 
> The first rev went in -mm needs work and the above url is the result
> of feedback from Al Viro.  He also acked the patch in the follow on
> thread:
> http://lkml.org/lkml/2007/5/22/5
> 
> Not sure how widely it was tested on that last patch, I see a few
> reports that the patch works out OK.

Could you please send a fresh, shiny, new, changelogged patch against mainline?

Thanks.

Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

[Ken Chen]

On 5/31/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> Could you please send a fresh, shiny, new, changelogged patch against mainline?

OK, here it is.  It would be nice to merge this before final 2.6.22
release.  Thank you.


The kernel on-demand loop device instantiation breaks several user space
tools as the tools are not ready to cope with the "on-demand feature".  Fix
it by instantiate default 8 loop devices and also reinstate max_loop module
parameter.


Signed-off-by: Ken Chen <kenchen@google.com>
Acked-by: Al Viro <viro@zeniv.linux.org.uk>


diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 5526ead..0ed5470 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1354,7 +1354,7 @@ #endif
  */
 static int max_loop;
 module_param(max_loop, int, 0);
-MODULE_PARM_DESC(max_loop, "obsolete, loop device is created on-demand");
+MODULE_PARM_DESC(max_loop, "Maximum number of loop devices");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);

@@ -1394,16 +1394,11 @@ int loop_unregister_transfer
 EXPORT_SYMBOL(loop_register_transfer);
 EXPORT_SYMBOL(loop_unregister_transfer);

-static struct loop_device *loop_init_one(int i)
+static struct loop_device *loop_alloc(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;
@@ -1427,8 +1422,6 @@ static struct loop_device *loop_init_one
 	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:
@@ -1439,15 +1432,37 @@ out:
 	return NULL;
 }

-static void loop_del_one(struct loop_device *lo)
+static void loop_free(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);
 }

+static struct loop_device *loop_init_one(int i)
+{
+	struct loop_device *lo;
+
+	list_for_each_entry(lo, &loop_devices, lo_list) {
+		if (lo->lo_number == i)
+			return lo;
+	}
+
+	lo = loop_alloc(i);
+	if (lo) {
+		add_disk(lo->lo_disk);
+		list_add_tail(&lo->lo_list, &loop_devices);
+	}
+	return lo;
+}
+
+static void loop_del_one(struct loop_device *lo)
+{
+	del_gendisk(lo->lo_disk);
+	loop_free(lo);
+}
+
 static struct kobject *loop_probe(dev_t dev, int *part, void *data)
 {
 	struct loop_device *lo;
@@ -1464,28 +1479,77 @@ static struct kobject *loop_probe

 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, NULL, NULL);
+	int i, nr;
+	unsigned long range;
+	struct loop_device *lo, *next;
+
+	/*
+	 * loop module now has a feature to instantiate underlying device
+	 * structure on-demand, provided that there is an access dev node.
+	 * However, this will not work well with user space tool that doesn't
+	 * know about such "feature".  In order to not break any existing
+	 * tool, we do the following:
+	 *
+	 * (1) if max_loop is specified, create that many upfront, and this
+	 *     also becomes a hard limit.
+	 * (2) if max_loop is not specified, create 8 loop device on module
+	 *     load, user can further extend loop device by create dev node
+	 *     themselves and have kernel automatically instantiate actual
+	 *     device on-demand.
+	 */
+	if (max_loop > 1UL << MINORBITS)
+		return -EINVAL;

 	if (max_loop) {
-		printk(KERN_INFO "loop: the max_loop option is obsolete "
-				 "and will be removed in March 2008\n");
+		nr = max_loop;
+		range = max_loop;
+	} else {
+		nr = 8;
+		range = 1UL << MINORBITS;
+	}
+
+	if (register_blkdev(LOOP_MAJOR, "loop"))
+		return -EIO;

+	for (i = 0; i < nr; i++) {
+		lo = loop_alloc(i);
+		if (!lo)
+			goto Enomem;
+		list_add_tail(&lo->lo_list, &loop_devices);
 	}
+
+	/* point of no return */
+
+	list_for_each_entry(lo, &loop_devices, lo_list)
+		add_disk(lo->lo_disk);
+
+	blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
+				  THIS_MODULE, loop_probe, NULL, NULL);
+
 	printk(KERN_INFO "loop: module loaded\n");
 	return 0;
+
+Enomem:
+	printk(KERN_INFO "loop: out of memory\n");
+
+	list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
+		loop_free(lo);
+
+	unregister_blkdev(LOOP_MAJOR, "loop");
+	return -ENOMEM;
 }

 static void __exit loop_exit(void)
 {
+	unsigned long range;
 	struct loop_device *lo, *next;

+	range = max_loop ? max_loop :  1UL << MINORBITS;
+
 	list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
 		loop_del_one(lo);

-	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), 1UL << MINORBITS);
+	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), range);
 	if (unregister_blkdev(LOOP_MAJOR, "loop"))
 		printk(KERN_WARNING "loop: cannot unregister blkdev\n");
 }