Patchwork block/partitions: optimize memory allocation in check_partition()

login
register
mail settings
Submitter Tim Gardner
Date Aug. 5, 2013, 7:40 a.m.
Message ID <1375688404-19956-1-git-send-email-tim.gardner@canonical.com>
Download mbox | patch
Permalink /patch/264592/
State New
Headers show

Comments

Tim Gardner - Aug. 5, 2013, 7:40 a.m.
From: Ming Lei <ming.lei@canonical.com>

BugLink: http://bugs.launchpad.net/bugs/1206837

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 always

Signed-off-by: Ming Lei <ming.lei@canonical.com>
Reported-by: Felipe Balbi <balbi@ti.com>
Cc: Jens Axboe <axboe@kernel.dk>
Reviewed-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
(cherry picked from commit ac2e5327a5e4f6477afc6a3b3b0dc6e0476d71d4)

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 block/partition-generic.c |    4 ++--
 block/partitions/check.c  |   37 ++++++++++++++++++++++++++++++++-----
 block/partitions/check.h  |    4 +++-
 3 files changed, 37 insertions(+), 8 deletions(-)
Seth Forshee - Aug. 5, 2013, 8:11 a.m.

Brad Figg - Aug. 5, 2013, 8:22 a.m.
On 08/05/2013 08:40 AM, Tim Gardner wrote:
> From: Ming Lei <ming.lei@canonical.com>
>
> BugLink: http://bugs.launchpad.net/bugs/1206837
>
> 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 always
>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> Reported-by: Felipe Balbi <balbi@ti.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Reviewed-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> (cherry picked from commit ac2e5327a5e4f6477afc6a3b3b0dc6e0476d71d4)
>
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
>   block/partition-generic.c |    4 ++--
>   block/partitions/check.c  |   37 ++++++++++++++++++++++++++++++++-----
>   block/partitions/check.h  |    4 +++-
>   3 files changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index 1cb4dec..789cdea 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..19ba207 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,45 @@ static int (*check_part[])(struct parsed_partitions *) = {
>   	NULL
>   };
>
> +static struct parsed_partitions *allocate_partitions(struct gendisk *hd)
> +{
> +	struct parsed_partitions *state;
> +	int nr;
> +
> +	state = kzalloc(sizeof(*state), GFP_KERNEL);
> +	if (!state)
> +		return NULL;
> +
> +	nr = disk_max_parts(hd);
> +	state->parts = vzalloc(nr * sizeof(state->parts[0]));
> +	if (!state->parts) {
> +		kfree(state);
> +		return NULL;
> +	}
> +
> +	state->limit = nr;
> +
> +	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);
> +	state = allocate_partitions(hd);
>   	if (!state)
>   		return NULL;
>   	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 +156,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 +188,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 *);
>
>
Tim Gardner - Aug. 5, 2013, 8:33 a.m.

Patch

diff --git a/block/partition-generic.c b/block/partition-generic.c
index 1cb4dec..789cdea 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..19ba207 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,45 @@  static int (*check_part[])(struct parsed_partitions *) = {
 	NULL
 };
 
+static struct parsed_partitions *allocate_partitions(struct gendisk *hd)
+{
+	struct parsed_partitions *state;
+	int nr;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	nr = disk_max_parts(hd);
+	state->parts = vzalloc(nr * sizeof(state->parts[0]));
+	if (!state->parts) {
+		kfree(state);
+		return NULL;
+	}
+
+	state->limit = nr;
+
+	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);
+	state = allocate_partitions(hd);
 	if (!state)
 		return NULL;
 	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 +156,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 +188,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 *);