login
Search
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 = ...
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) ...Could not we use kobject_uevent(), like I did (though perhaps without the k.oops), to make udev create a node? Jan -- -
Oh, crap. Google mail is innocent. It was me who did some ugly
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
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, ...I do not think is needed, since it will already be caught by the
Can do without {}
Jan
--
-
On Fri, 30 Mar 2007 02:25:37 -0700 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? -
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 -
On Fri, 30 Mar 2007 15:06:03 -0700 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 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 -
Yes, the distros do, and they recommend it to their users a lot. thanks, greg k-h -
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], ...
> 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 -
I ecpect it will join all the other brilliant code in 2.6.22 ;) -
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 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 -
I'm sorry I made a mistake, 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 -
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 -
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 -- -
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 -
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 -
ahem. On Fri, 30 Mar 2007 02:25:37 -0700 which makes me suspect that you didn't send the patch which you meant to send, so I'll drop it. -
