2007-05-22 00:19:00

by Al Viro

[permalink] [raw]
Subject: Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

On Mon, May 21, 2007 at 03:00:55PM -0700, [email protected] 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().


2007-05-22 01:30:33

by Ken Chen

[permalink] [raw]
Subject: Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

On 5/21/07, Al Viro <[email protected]> wrote:
> On Mon, May 21, 2007 at 03:00:55PM -0700, [email protected] 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.

2007-05-22 01:48:34

by Al Viro

[permalink] [raw]
Subject: Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

On Mon, May 21, 2007 at 06:30:15PM -0700, Ken Chen wrote:
> On 5/21/07, Al Viro <[email protected]> wrote:
> >On Mon, May 21, 2007 at 03:00:55PM -0700, [email protected] 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;
}

2007-05-22 02:15:14

by Ken Chen

[permalink] [raw]
Subject: Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

On 5/21/07, Al Viro <[email protected]> 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?

2007-05-22 02:40:38

by Ken Chen

[permalink] [raw]
Subject: Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

On 5/21/07, Ken Chen <[email protected]> wrote:
> On 5/21/07, Al Viro <[email protected]> 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");
}

2007-05-22 04:19:10

by Al Viro

[permalink] [raw]
Subject: Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

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.

2007-05-31 16:06:48

by walt

[permalink] [raw]
Subject: Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

Ken Chen wrote:
> On 5/21/07, Ken Chen <[email protected]> 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");
> }



2007-05-31 16:51:52

by Ken Chen

[permalink] [raw]
Subject: Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

On 5/31/07, walt <[email protected]> wrote:
> Ken Chen wrote:
> > On 5/21/07, Ken Chen <[email protected]> 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?

2007-05-31 18:11:17

by Andrew Morton

[permalink] [raw]
Subject: Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

On Thu, 31 May 2007 09:51:36 -0700
"Ken Chen" <[email protected]> wrote:

> On 5/31/07, walt <[email protected]> wrote:
> > Ken Chen wrote:
> > > On 5/21/07, Ken Chen <[email protected]> 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?

2007-05-31 18:23:47

by Ken Chen

[permalink] [raw]
Subject: Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

On 5/31/07, Andrew Morton <[email protected]> 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.

2007-05-31 18:35:23

by Andrew Morton

[permalink] [raw]
Subject: Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

On Thu, 31 May 2007 11:23:32 -0700
"Ken Chen" <[email protected]> wrote:

> On 5/31/07, Andrew Morton <[email protected]> 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.

2007-06-01 16:29:52

by Ken Chen

[permalink] [raw]
Subject: Re: + loop-preallocate-eight-loop-devices.patch added to -mm tree

On 5/31/07, Andrew Morton <[email protected]> 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 <[email protected]>
Acked-by: Al Viro <[email protected]>


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