[PATCH v1] block: partition: optimize memory allocation in check_partition

[PATCH v1] block: partition: optimize memory allocation in check_partition

[Ming Lei]

Currently, sizeof(struct parsed_partitions) may be 64KB in 32bit arch,
so it is easy to trigger page allocation failure by check_partition,
especially in hotplug block device situation(such as, USB mass storage,
MMC card, ...), and Felipe Balbi has observed the failure.

This patch does below optimizations on the allocation of struct
parsed_partitions to try to address the issue:

- make parsed_partitions.parts as pointer so that the pointed memory
can fit in 32KB buffer, then approximate 32KB memory can be saved

- vmalloc the buffer pointed by parsed_partitions.parts because
32KB is still a bit big for kmalloc

- given that many devices have the partition count limit, so only
allocate disk_max_parts() partitions instead of 256 partitions

Cc: stable@vger.kernel.org
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
v1:
	- add one missing free_partitions
	- rename release_partitions as free_partitions

 block/partition-generic.c |    4 ++--
 block/partitions/check.c  |   35 ++++++++++++++++++++++++++++++-----
 block/partitions/check.h  |    4 +++-
 3 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/block/partition-generic.c b/block/partition-generic.c
index f1d1451..17d44ea 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -418,7 +418,7 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 	int p, highest, res;
 rescan:
 	if (state && !IS_ERR(state)) {
-		kfree(state);
+		free_partitions(state);
 		state = NULL;
 	}
 
@@ -525,7 +525,7 @@ rescan:
 			md_autodetect_dev(part_to_dev(part)->devt);
 #endif
 	}
-	kfree(state);
+	free_partitions(state);
 	return 0;
 }
 
diff --git a/block/partitions/check.c b/block/partitions/check.c
index bc90867..944fe22 100644
--- a/block/partitions/check.c
+++ b/block/partitions/check.c
@@ -14,6 +14,7 @@
  */
 
 #include <linux/slab.h>
+#include <linux/vmalloc.h>
 #include <linux/ctype.h>
 #include <linux/genhd.h>
 
@@ -106,18 +107,43 @@ static int (*check_part[])(struct parsed_partitions *) = {
 	NULL
 };
 
+static struct parsed_partitions *allocate_partitions(int nr)
+{
+	struct parsed_partitions *state;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	state->parts = vzalloc(nr * sizeof(state->parts[0]));
+	if (!state->parts) {
+		kfree(state);
+		return NULL;
+	}
+
+	return state;
+}
+
+void free_partitions(struct parsed_partitions *state)
+{
+	vfree(state->parts);
+	kfree(state);
+}
+
 struct parsed_partitions *
 check_partition(struct gendisk *hd, struct block_device *bdev)
 {
 	struct parsed_partitions *state;
 	int i, res, err;
 
-	state = kzalloc(sizeof(struct parsed_partitions), GFP_KERNEL);
+	i = disk_max_parts(hd);
+	state = allocate_partitions(i);
 	if (!state)
 		return NULL;
+	state->limit = i;
 	state->pp_buf = (char *)__get_free_page(GFP_KERNEL);
 	if (!state->pp_buf) {
-		kfree(state);
+		free_partitions(state);
 		return NULL;
 	}
 	state->pp_buf[0] = '\0';
@@ -128,10 +154,9 @@ check_partition(struct gendisk *hd, struct block_device *bdev)
 	if (isdigit(state->name[strlen(state->name)-1]))
 		sprintf(state->name, "p");
 
-	state->limit = disk_max_parts(hd);
 	i = res = err = 0;
 	while (!res && check_part[i]) {
-		memset(&state->parts, 0, sizeof(state->parts));
+		memset(state->parts, 0, state->limit * sizeof(state->parts[0]));
 		res = check_part[i++](state);
 		if (res < 0) {
 			/* We have hit an I/O error which we don't report now.
@@ -161,6 +186,6 @@ check_partition(struct gendisk *hd, struct block_device *bdev)
 	printk(KERN_INFO "%s", state->pp_buf);
 
 	free_page((unsigned long)state->pp_buf);
-	kfree(state);
+	free_partitions(state);
 	return ERR_PTR(res);
 }
diff --git a/block/partitions/check.h b/block/partitions/check.h
index 52b1003..eade17e 100644
--- a/block/partitions/check.h
+++ b/block/partitions/check.h
@@ -15,13 +15,15 @@ struct parsed_partitions {
 		int flags;
 		bool has_info;
 		struct partition_meta_info info;
-	} parts[DISK_MAX_PARTS];
+	} *parts;
 	int next;
 	int limit;
 	bool access_beyond_eod;
 	char *pp_buf;
 };
 
+void free_partitions(struct parsed_partitions *state);
+
 struct parsed_partitions *
 check_partition(struct gendisk *, struct block_device *);
 
-- 
1.7.9.5

Re: [PATCH v1] block: partition: optimize memory allocation in check_partition

[Andrew Morton]

On Fri,  1 Feb 2013 20:23:12 +0800
Ming Lei <ming.lei@canonical.com> wrote:

> Currently, sizeof(struct parsed_partitions) may be 64KB in 32bit arch,
> so it is easy to trigger page allocation failure by check_partition,
> especially in hotplug block device situation(such as, USB mass storage,
> MMC card, ...), and Felipe Balbi has observed the failure.
> 
> This patch does below optimizations on the allocation of struct
> parsed_partitions to try to address the issue:
> 
> - make parsed_partitions.parts as pointer so that the pointed memory
> can fit in 32KB buffer, then approximate 32KB memory can be saved
> 
> - vmalloc the buffer pointed by parsed_partitions.parts because
> 32KB is still a bit big for kmalloc
> 
> - given that many devices have the partition count limit, so only
> allocate disk_max_parts() partitions instead of 256 partitions

This is only true when !(disk->flags & GENHD_FL_EXT_DEVT) in
disk_max_parts().  Which I suspect is basically "never".  Oh well.

> Cc: stable@vger.kernel.org

I don't think I agree with the -stable backport.  The bug isn't
terribly serious and the patch is far more extensive than it really
needed to be.

If we do think the fix should be backported then it would be better to
do it as a series of two patches.  A nice simple one (say, a basic
s/kmalloc/vmalloc/) for 3.8 and -stable, then a more extensive
optimise-things patch for 3.9-rc1.

> Signed-off-by: Ming Lei <ming.lei@canonical.com>

A Reported-by:Felipe would be nice here.  We appreciate bug reports and
this little gesture is the least we can do.

>
> ...
>
>  struct parsed_partitions *
>  check_partition(struct gendisk *hd, struct block_device *bdev)
>  {
>  	struct parsed_partitions *state;
>  	int i, res, err;
>  
> -	state = kzalloc(sizeof(struct parsed_partitions), GFP_KERNEL);
> +	i = disk_max_parts(hd);
> +	state = allocate_partitions(i);
>  	if (!state)
>  		return NULL;
> +	state->limit = i;

I suggest this assignment be performed in allocate_partitions() itself.
That's better than requiring that all allocate_partitions() callers
remember to fill it in.


>  	state->pp_buf = (char *)__get_free_page(GFP_KERNEL);
>  	if (!state->pp_buf) {
> -		kfree(state);
> +		free_partitions(state);
>  		return NULL;
>  	}
>  	state->pp_buf[0] = '\0';
>
> ...
>
> --- a/block/partitions/check.h
> +++ b/block/partitions/check.h
> @@ -15,13 +15,15 @@ struct parsed_partitions {
>  		int flags;
>  		bool has_info;
>  		struct partition_meta_info info;
> -	} parts[DISK_MAX_PARTS];
> +	} *parts;
>  	int next;
>  	int limit;
>  	bool access_beyond_eod;
>  	char *pp_buf;
>  };

With this change, DISK_MAX_PARTS becomes a rather dangerous thing - do
we have code floating around which does

	for (i = 0; i < DISK_MAX_PARTS; i++)
		access(parsed_partitions.parts[i]);

?

If so, we should do s/DISK_MAX_PARTS/parsed_partitions.limit/.

The only such code I can find is in
block/partitions/mac.c:mac_partition().  And with your patch, this code
is potentially buggy, I suspect.  We could do
s/DISK_MAX_PARTS/state->limit/, but would that work?  What happens if
disk_max_parts() returned a value which is smaller than blocks_in_map?

It needs some thought.  I'm reluctant to apply this version of the
patch due to this.

>
> ...
>

Re: [PATCH v1] block: partition: optimize memory allocation in check_partition

[Ming Lei]

On Sat, Feb 2, 2013 at 6:43 AM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri,  1 Feb 2013 20:23:12 +0800

I just return from holiday, sorry for the delay, and thanks for the review.

> Ming Lei <ming.lei@canonical.com> wrote:
>
>> Currently, sizeof(struct parsed_partitions) may be 64KB in 32bit arch,
>> so it is easy to trigger page allocation failure by check_partition,
>> especially in hotplug block device situation(such as, USB mass storage,
>> MMC card, ...), and Felipe Balbi has observed the failure.
>>
>> This patch does below optimizations on the allocation of struct
>> parsed_partitions to try to address the issue:
>>
>> - make parsed_partitions.parts as pointer so that the pointed memory
>> can fit in 32KB buffer, then approximate 32KB memory can be saved
>>
>> - vmalloc the buffer pointed by parsed_partitions.parts because
>> 32KB is still a bit big for kmalloc
>>
>> - given that many devices have the partition count limit, so only
>> allocate disk_max_parts() partitions instead of 256 partitions
>
> This is only true when !(disk->flags & GENHD_FL_EXT_DEVT) in
> disk_max_parts().  Which I suspect is basically "never".  Oh well.

At first glance, I knew mmc block device don't set the flag. In fact, only
very few block devices have set the flag:

$git grep -n GENHD_FL_EXT_DEVT drivers/
drivers/block/loop.c:1654:      disk->flags |= GENHD_FL_EXT_DEVT;
drivers/ide/ide-gd.c:418:       g->flags |= GENHD_FL_EXT_DEVT;
drivers/md/md.c:4881:   disk->flags |= GENHD_FL_EXT_DEVT;
drivers/scsi/sd.c:2827: gd->flags = GENHD_FL_EXT_DEVT;

>
>> Cc: stable@vger.kernel.org
>
> I don't think I agree with the -stable backport.  The bug isn't
> terribly serious and the patch is far more extensive than it really
> needed to be.
>
> If we do think the fix should be backported then it would be better to
> do it as a series of two patches.  A nice simple one (say, a basic
> s/kmalloc/vmalloc/) for 3.8 and -stable, then a more extensive
> optimise-things patch for 3.9-rc1.

OK, I will remove the stable tag considered that it is not easy to
reproduce.

>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>
> A Reported-by:Felipe would be nice here.  We appreciate bug reports and
> this little gesture is the least we can do.

OK, will add it.

>
>>
>> ...
>>
>>  struct parsed_partitions *
>>  check_partition(struct gendisk *hd, struct block_device *bdev)
>>  {
>>       struct parsed_partitions *state;
>>       int i, res, err;
>>
>> -     state = kzalloc(sizeof(struct parsed_partitions), GFP_KERNEL);
>> +     i = disk_max_parts(hd);
>> +     state = allocate_partitions(i);
>>       if (!state)
>>               return NULL;
>> +     state->limit = i;
>
> I suggest this assignment be performed in allocate_partitions() itself.
> That's better than requiring that all allocate_partitions() callers
> remember to fill it in.

OK.

>
>>       state->pp_buf = (char *)__get_free_page(GFP_KERNEL);
>>       if (!state->pp_buf) {
>> -             kfree(state);
>> +             free_partitions(state);
>>               return NULL;
>>       }
>>       state->pp_buf[0] = '\0';
>>
>> ...
>>
>> --- a/block/partitions/check.h
>> +++ b/block/partitions/check.h
>> @@ -15,13 +15,15 @@ struct parsed_partitions {
>>               int flags;
>>               bool has_info;
>>               struct partition_meta_info info;
>> -     } parts[DISK_MAX_PARTS];
>> +     } *parts;
>>       int next;
>>       int limit;
>>       bool access_beyond_eod;
>>       char *pp_buf;
>>  };
>
> With this change, DISK_MAX_PARTS becomes a rather dangerous thing - do
> we have code floating around which does
>
>         for (i = 0; i < DISK_MAX_PARTS; i++)
>                 access(parsed_partitions.parts[i]);
>
> ?
>
> If so, we should do s/DISK_MAX_PARTS/parsed_partitions.limit/.
>
> The only such code I can find is in
> block/partitions/mac.c:mac_partition().  And with your patch, this code
> is potentially buggy, I suspect.  We could do
> s/DISK_MAX_PARTS/state->limit/, but would that work?  What happens if
> disk_max_parts() returned a value which is smaller than blocks_in_map?

IMO, looks it is safe to do s/DISK_MAX_PARTS/state->limit/ for
block/partitions/mac.c:mac_partition(). If the driver sets the
flag of GENHD_FL_EXT_DEVT, the replacement has no effect.
If the flag isn't set, drivers just want to put an limit on access to the
partitions, and device node of the partitions above the limit aren't
visible for users too. So could we do the below replacement?

diff --git a/block/partitions/mac.c b/block/partitions/mac.c
index 11f688b..c7d2da5 100644
--- a/block/partitions/mac.c
+++ b/block/partitions/mac.c
@@ -59,10 +59,12 @@ int mac_partition(struct parsed_partitions *state)
 		return 0;		/* not a MacOS disk */
 	}
 	blocks_in_map = be32_to_cpu(part->map_count);
-	if (blocks_in_map < 0 || blocks_in_map >= DISK_MAX_PARTS) {
+	if (blocks_in_map < 0) {
 		put_dev_sector(sect);
 		return 0;
-	}
+	} else if (blocks_in_map >= state->limit)
+		blocks_in_map = state->limit - 1;
+
 	strlcat(state->pp_buf, " [mac]", PAGE_SIZE);
 	for (slot = 1; slot <= blocks_in_map; ++slot) {
 		int pos = slot * secsize;


Thanks,
--
Ming Lei