Re: [patch] [bugfix] loop.c

On Fri, Mar 23, 2007 at 03:04:54PM +0100, Tomas M wrote:
I posted this yesterday but it seems people didn't understand the real
goal of my patch. So I will explain once more again:

This is a bugfix for loop.c block driver, as it currently allocates more
memory then it needs, without any further use.

If 'max_loop=255' parameter is given, the loop.c driver allocates this
amount of memory:

kmalloc(max_loop * sizeof(struct loop_device))

But in this case, (max_loop * sizeof) is greater than 65536, and thus
kmalloc must allocate the next bigger size (which is 128KB of RAM).

Unfortunately the loop.c driver doesn't allow users to use bigger
max_loop then 255 (for reasons which are completely obsolete) so the
rest of memory is *unused*.

This patch doesn't fix unused memory in the case of max_loop=255, but
rather it allows user to specify bigger max_loop (up to 455 on 386), so
the user can fully use all the memory, which would be allocated anyway.

Thank you for your consideration

The right thing to start with is to split the allocation up, and allocate
each loop device by itself, like in the untested patch below:

After that you're not wasting memory for any off number of loop devices
and can create as many as you have device numbers available:


Index: linux-2.6/drivers/block/loop.c
===================================================================
--- linux-2.6.orig/drivers/block/loop.c 2007-03-23 15:21:57.000000000 +0100
+++ linux-2.6/drivers/block/loop.c 2007-03-23 15:39:44.000000000 +0100
@@ -78,8 +78,7 @@
#include <asm/uaccess.h>

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

/*
* 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;
@@ -1383,7 +1382,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,9 +1397,56 @@ int loop_unregister_transfer(int number)
EXPORT_SYMBOL(loop_register_transfer);
EXPORT_SYMBOL(loop_unregister_transfer);

+static int __init loop_init_one(int i)
+{
+ struct loop_device *lo;
+
+ 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;
+ lo->lo_disk = alloc_disk(1);
+ if (!lo->lo_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);
+ list_add(&lo->lo_list, &loop_devices);
+ lo->lo_disk->major = LOOP_MAJOR;
+ lo->lo_disk->first_minor = i;
+ lo->lo_disk->fops = &lo_fops;
+ sprintf(lo->lo_disk->disk_name, "loop%d", i);
+ lo->lo_disk->private_data = lo;
+ lo->lo_disk->queue = lo->lo_queue;
+ add_disk(lo->lo_disk);
+ return 0;
+
+ out_free_queue:
+ blk_cleanup_queue(lo->lo_queue);
+ out_free_dev:
+ kfree(lo);
+ out:
+ return -ENOMEM;
+}
+
+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);
+}
+
static int __init loop_init(void)
{
- int i;
+ struct loop_device *lo, *next;
+ int err, i;

if (max_loop < 1 || max_loop > 256) {
printk(KERN_WARNING "loop: invalid max_loop (must be between"
@@ -1411,78 +1457,32 @@ static int __init loop_init(void)
if (register_blkdev(LOOP_MAJOR, "loop"))
return -EIO;

- 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));
-
- disks = kmalloc(max_loop * sizeof(struct gendisk *), GFP_KERNEL);
- if (!disks)
- goto out_mem2;
-
for (i = 0; i < max_loop; i++) {
- disks[i] = alloc_disk(1);
- if (!disks[i])
- goto out_mem3;
+ err = loop_init_one(i);
+ if (err)
+ goto out_free_devs;
}

- 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);
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_devs:
+ list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
+ loop_del_one(lo);
unregister_blkdev(LOOP_MAJOR, "loop");
printk(KERN_ERR "loop: ran out of memory\n");
- return -ENOMEM;
+ return err;
}

static void loop_exit(void)
{
- int i;
+ struct loop_device *lo, *next;
+
+ list_for_each_entry_safe(lo, next, &loop_devices, lo_list)
+ loop_del_one(lo);

- for (i = 0; i < max_loop; i++) {
- del_gendisk(disks[i]);
- blk_cleanup_queue(loop_dev[i].lo_queue);
- put_disk(disks[i]);
- }
if (unregister_blkdev(LOOP_MAJOR, "loop"))
printk(KERN_WARNING "loop: cannot unregister blkdev\n");
-
- kfree(disks);
- kfree(loop_dev);
}

module_init(loop_init);
Index: linux-2.6/include/linux/loop.h
===================================================================
--- linux-2.6.orig/include/linux/loop.h 2007-03-23 15:23:32.000000000 +0100
+++ linux-2.6/include/linux/loop.h 2007-03-23 15:30:40.000000000 +0100
@@ -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__ */
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages