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

[PATCH] 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>
---
 block/partition-generic.c |    2 +-
 block/partitions/check.c  |   35 ++++++++++++++++++++++++++++++-----
 block/partitions/check.h  |    4 +++-
 3 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/block/partition-generic.c b/block/partition-generic.c
index f1d1451..043d0bd 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -525,7 +525,7 @@ rescan:
 			md_autodetect_dev(part_to_dev(part)->devt);
 #endif
 	}
-	kfree(state);
+	release_partitions(state);
 	return 0;
 }
 
diff --git a/block/partitions/check.c b/block/partitions/check.c
index bc90867..86c9450 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 release_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);
+		release_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);
+	release_partitions(state);
 	return ERR_PTR(res);
 }
diff --git a/block/partitions/check.h b/block/partitions/check.h
index 52b1003..4fefdbb 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 release_partitions(struct parsed_partitions *state);
+
 struct parsed_partitions *
 check_partition(struct gendisk *, struct block_device *);
 
-- 
1.7.9.5

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

[Joe Perches]

On Fri, 2013-02-01 at 11:07 +0800, Ming Lei wrote:
> Currently, sizeof(struct parsed_partitions) may be 64KB in 32bit arch,
> so it is easy to trigger page allocation failure by check_partition,
[]
> 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
[]
> diff --git 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;

This is relatively unusual style.
Could you break out this struct instead?

struct partition_parts {
	...
};

and use

	struct partition_parts *parts;

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

[Ming Lei]

On Fri, Feb 1, 2013 at 11:20 AM, Joe Perches <joe@perches.com> wrote:
>> @@ -15,13 +15,15 @@ struct parsed_partitions {
>>               int flags;
>>               bool has_info;
>>               struct partition_meta_info info;
>> -     } parts[DISK_MAX_PARTS];
>> +     } *parts;
>
> This is relatively unusual style.
> Could you break out this struct instead?

IMO, looks it isn't necessary since no one uses the type of the embedded
structure directly, and the style isn't introduced by this patch, which only
follows the previous one.

Thanks,
--
Ming Lei

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

[Yasuaki Ishimatsu]

2013/02/01 12:07, Ming Lei 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
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>   block/partition-generic.c |    2 +-
>   block/partitions/check.c  |   35 ++++++++++++++++++++++++++++++-----
>   block/partitions/check.h  |    4 +++-
>   3 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index f1d1451..043d0bd 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -525,7 +525,7 @@ rescan:
>   			md_autodetect_dev(part_to_dev(part)->devt);
>   #endif
>   	}
> -	kfree(state);
> +	release_partitions(state);
>   	return 0;
>   }

Do you check all kfree(state)?

# block/partition-generic.c
414 int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
415 {
416         struct parsed_partitions *state = NULL;
417         struct hd_struct *part;
418         int p, highest, res;
419 rescan:
420         if (state && !IS_ERR(state)) {
421                 kfree(state);
422                 state = NULL;
423         }   

At least, you should use release_partitions() instead of kfree() here.

Thanks,
Yasuaki Ishimatsu

>   
> diff --git a/block/partitions/check.c b/block/partitions/check.c
> index bc90867..86c9450 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 release_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);
> +		release_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);
> +	release_partitions(state);
>   	return ERR_PTR(res);
>   }
> diff --git a/block/partitions/check.h b/block/partitions/check.h
> index 52b1003..4fefdbb 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 release_partitions(struct parsed_partitions *state);
> +
>   struct parsed_partitions *
>   check_partition(struct gendisk *, struct block_device *);
>   
> 

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

[Ming Lei]

On Fri, Feb 1, 2013 at 4:33 PM, Yasuaki Ishimatsu
<isimatu.yasuaki@jp.fujitsu.com> wrote:

> At least, you should use release_partitions() instead of kfree() here.

Good catch, thank you for pointing it out, and I will post v1 later
with the update.

Thanks,
--
Ming Lei