[RFC/PATCH 2/2] shmem: use lib/parser for mount options

[RFC/PATCH 2/2] shmem: use lib/parser for mount options

[Randy Dunlap]

Build-tested only.  I will run-test it, but I want to ask first:
Is there any trick to mounting/testing shmem/tmpfs?

Thanks.
---

From: Randy Dunlap <randy.dunlap@oracle.com>

Convert shmem (tmpfs) to use the in-kernel mount options parsing library.

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 mm/shmem.c |  150 +++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 88 insertions(+), 62 deletions(-)

--- linux-2622-rc2.orig/mm/shmem.c
+++ linux-2622-rc2/mm/shmem.c
@@ -32,6 +32,7 @@
 #include <linux/mman.h>
 #include <linux/file.h>
 #include <linux/swap.h>
+#include <linux/parser.h>
 #include <linux/pagemap.h>
 #include <linux/string.h>
 #include <linux/slab.h>
@@ -84,6 +85,23 @@ enum sgp_type {
 	SGP_WRITE,	/* may exceed i_size, may allocate page */
 };
 
+enum {
+	Opt_size, Opt_nr_blocks, Opt_nr_inodes,
+	Opt_mode, Opt_uid, Opt_gid,
+	Opt_mpol, Opt_err,
+};
+
+static match_table_t tokens = {
+	{Opt_size,	"size=%u"},
+	{Opt_nr_blocks,	"nr_blocks=%u"},
+	{Opt_nr_inodes,	"nr_inodes=%u"},
+	{Opt_mode,	"mode=%u"},	/* not for remount */
+	{Opt_uid,	"uid=%u"},	/* not for remount */
+	{Opt_gid,	"gid=%u"},	/* not for remount */
+	{Opt_mpol,	"mpol=%s"},	/* various NUMA memory policy options */
+	{Opt_err,	NULL},
+};
+
 static int shmem_getpage(struct inode *inode, unsigned long idx,
 			 struct page **pagep, enum sgp_type sgp, int *type);
 
@@ -2113,92 +2131,100 @@ static struct export_operations shmem_ex
 
 static int shmem_parse_options(char *options, int *mode, uid_t *uid,
 	gid_t *gid, unsigned long *blocks, unsigned long *inodes,
-	int *policy, nodemask_t *policy_nodes)
+	int *policy, nodemask_t *policy_nodes, int is_remount)
 {
-	char *this_char, *value, *rest;
+	char *rest;
+	substring_t args[MAX_OPT_ARGS];
+	char *p;
+	int option;
 
-	while (options != NULL) {
-		this_char = options;
-		for (;;) {
-			/*
-			 * NUL-terminate this option: unfortunately,
-			 * mount options form a comma-separated list,
-			 * but mpol's nodelist may also contain commas.
-			 */
-			options = strchr(options, ',');
-			if (options == NULL)
-				break;
-			options++;
-			if (!isdigit(*options)) {
-				options[-1] = '\0';
-				break;
-			}
-		}
-		if (!*this_char)
-			continue;
-		if ((value = strchr(this_char,'=')) != NULL) {
-			*value++ = 0;
-		} else {
-			printk(KERN_ERR
-			    "tmpfs: No value for mount option '%s'\n",
-			    this_char);
-			return 1;
-		}
+	if (!options)
+		return 0;
 
-		if (!strcmp(this_char,"size")) {
+	while ((p = strsep(&options, ",")) != NULL) {
+		int token;
+
+		if (!*p)
+			continue;
+		token = match_token(p, tokens, args);
+		switch (token) {
+		case Opt_size: {
 			unsigned long long size;
-			size = memparse(value,&rest);
+			size = memparse(args[0].from, &rest);
 			if (*rest == '%') {
 				size <<= PAGE_SHIFT;
 				size *= totalram_pages;
 				do_div(size, 100);
 				rest++;
 			}
-			if (*rest)
-				goto bad_val;
 			*blocks = size >> PAGE_CACHE_SHIFT;
-		} else if (!strcmp(this_char,"nr_blocks")) {
-			*blocks = memparse(value,&rest);
-			if (*rest)
-				goto bad_val;
-		} else if (!strcmp(this_char,"nr_inodes")) {
-			*inodes = memparse(value,&rest);
-			if (*rest)
-				goto bad_val;
-		} else if (!strcmp(this_char,"mode")) {
+			break;
+		}
+		case Opt_nr_blocks:
+			*blocks = memparse(args[0].from, &rest);
+			break;
+		case Opt_nr_inodes:
+			*inodes = memparse(args[0].from, &rest);
+			break;
+		case Opt_mode:
+			if (is_remount)		/* not valid on remount */
+				break;
 			if (!mode)
-				continue;
-			*mode = simple_strtoul(value,&rest,8);
-			if (*rest)
+				break;
+			if (match_int(&args[0], &option))
 				goto bad_val;
-		} else if (!strcmp(this_char,"uid")) {
+			*mode = option;
+			break;
+		case Opt_uid:
+			if (is_remount)		/* not valid on remount */
+				break;
 			if (!uid)
-				continue;
-			*uid = simple_strtoul(value,&rest,0);
-			if (*rest)
+				break;
+			if (match_int(&args[0], &option))
 				goto bad_val;
-		} else if (!strcmp(this_char,"gid")) {
+			*uid = option;
+			break;
+		case Opt_gid:
+			if (is_remount)		/* not valid on remount */
+				break;
 			if (!gid)
-				continue;
-			*gid = simple_strtoul(value,&rest,0);
-			if (*rest)
+				break;
+			if (match_int(&args[0], &option))
 				goto bad_val;
-		} else if (!strcmp(this_char,"mpol")) {
-			if (shmem_parse_mpol(value,policy,policy_nodes))
+			*gid = option;
+			break;
+		case Opt_mpol:
+			if (shmem_parse_mpol(args[0].from, policy, policy_nodes))
 				goto bad_val;
-		} else {
-			printk(KERN_ERR "tmpfs: Bad mount option %s\n",
-			       this_char);
+			break;
+		default:
+			printk(KERN_ERR "tmpfs: Bad mount option %s\n", p);
 			return 1;
+			break;
+		}
+
+		for (;;) {
+			/*
+			 * NUL-terminate this option: unfortunately,
+			 * mount options form a comma-separated list,
+			 * but mpol's nodelist may also contain commas.
+			 */
+			options = strchr(options, ',');
+			if (options == NULL)
+				break;
+			options++;
+			if (!isdigit(*options)) {
+				options[-1] = '\0';
+				break;
+			}
 		}
 	}
 	return 0;
 
 bad_val:
 	printk(KERN_ERR "tmpfs: Bad value '%s' for mount option '%s'\n",
-	       value, this_char);
+	       args[0].from, p);
 	return 1;
-
 }
 
 static int shmem_remount_fs(struct super_block *sb, int *flags, char *data)
@@ -2213,7 +2239,7 @@ static int shmem_remount_fs(struct super
 	int error = -EINVAL;
 
 	if (shmem_parse_options(data, NULL, NULL, NULL, &max_blocks,
-				&max_inodes, &policy, &policy_nodes))
+				&max_inodes, &policy, &policy_nodes, 1))
 		return error;
 
 	spin_lock(&sbinfo->stat_lock);
@@ -2280,7 +2306,7 @@ static int shmem_fill_super(struct super
 		if (inodes > blocks)
 			inodes = blocks;
 		if (shmem_parse_options(data, &mode, &uid, &gid, &blocks,
-					&inodes, &policy, &policy_nodes))
+					&inodes, &policy, &policy_nodes, 0))
 			return -EINVAL;
 	}
 	sb->s_export_op = &shmem_export_ops;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC/PATCH 2/2] shmem: use lib/parser for mount options

[Hugh Dickins]

On Thu, 24 May 2007, Randy Dunlap wrote:

> 
> Build-tested only.  I will run-test it, but I want to ask first:
> Is there any trick to mounting/testing shmem/tmpfs?

Sorry, no, I've no tricks or shortcuts for that.

Thanks for trying this: it's never seemed worth the effort to me,
but I know Andrew's in favour - here's our exchange from last year.

+  >  > [ Vaguely suprised that tmpfs isn't using match_token()... ]
+  > 
+  >  I did briefly consider that back in the days when I noticed a host of
+  >  fs filesystems got converted.  But didn't see any point in messing
+  >  with what was already working.  Haven't looked recently: would it
+  >  actually be a useful change to make?
+  
+  I guess it'd be nice to do for uniformity's sake, but it's hardly pressing.
+  I have a vague memory that the ext3 conversion actually increased .text
+  size, which was a bit irritating.

Well, your changes do save something like 150 bytes, so that's good.

Though you end up using both match_token and memparse (I'm not surprised,
I thought it likely that tmpfs mount option quirks might require that),
so it's not entirely satisfying.  But if your testing works out, yes,
let's do it.

Your addition of an "is_remount" arg looks redundant to me: isn't it?
By all means add your "not valid on remount" comment to the !mode,
!uid, !gid lines (though "ignored on remount" would be clearer),
but I don't see the point of the "is_remount" addition.

hugetlbfs would be another candidate for conversion if you wish.

Thanks,
Hugh

> 
> Thanks.
> ---
> 
> From: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Convert shmem (tmpfs) to use the in-kernel mount options parsing library.
> 
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
> ---
>  mm/shmem.c |  150 +++++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 88 insertions(+), 62 deletions(-)
> 
> --- linux-2622-rc2.orig/mm/shmem.c
> +++ linux-2622-rc2/mm/shmem.c
> @@ -32,6 +32,7 @@
>  #include <linux/mman.h>
>  #include <linux/file.h>
>  #include <linux/swap.h>
> +#include <linux/parser.h>
>  #include <linux/pagemap.h>
>  #include <linux/string.h>
>  #include <linux/slab.h>
> @@ -84,6 +85,23 @@ enum sgp_type {
>  	SGP_WRITE,	/* may exceed i_size, may allocate page */
>  };
>  
> +enum {
> +	Opt_size, Opt_nr_blocks, Opt_nr_inodes,
> +	Opt_mode, Opt_uid, Opt_gid,
> +	Opt_mpol, Opt_err,
> +};
> +
> +static match_table_t tokens = {
> +	{Opt_size,	"size=%u"},
> +	{Opt_nr_blocks,	"nr_blocks=%u"},
> +	{Opt_nr_inodes,	"nr_inodes=%u"},
> +	{Opt_mode,	"mode=%u"},	/* not for remount */
> +	{Opt_uid,	"uid=%u"},	/* not for remount */
> +	{Opt_gid,	"gid=%u"},	/* not for remount */
> +	{Opt_mpol,	"mpol=%s"},	/* various NUMA memory policy options */
> +	{Opt_err,	NULL},
> +};
> +
>  static int shmem_getpage(struct inode *inode, unsigned long idx,
>  			 struct page **pagep, enum sgp_type sgp, int *type);
>  
> @@ -2113,92 +2131,100 @@ static struct export_operations shmem_ex
>  
>  static int shmem_parse_options(char *options, int *mode, uid_t *uid,
>  	gid_t *gid, unsigned long *blocks, unsigned long *inodes,
> -	int *policy, nodemask_t *policy_nodes)
> +	int *policy, nodemask_t *policy_nodes, int is_remount)
>  {
> -	char *this_char, *value, *rest;
> +	char *rest;
> +	substring_t args[MAX_OPT_ARGS];
> +	char *p;
> +	int option;
>  
> -	while (options != NULL) {
> -		this_char = options;
> -		for (;;) {
> -			/*
> -			 * NUL-terminate this option: unfortunately,
> -			 * mount options form a comma-separated list,
> -			 * but mpol's nodelist may also contain commas.
> -			 */
> -			options = strchr(options, ',');
> -			if (options == NULL)
> -				break;
> -			options++;
> -			if (!isdigit(*options)) {
> -				options[-1] = '\0';
> -				break;
> -			}
> -		}
> -		if (!*this_char)
> -			continue;
> -		if ((value = strchr(this_char,'=')) != NULL) {
> -			*value++ = 0;
> -		} else {
> -			printk(KERN_ERR
> -			    "tmpfs: No value for mount option '%s'\n",
> -			    this_char);
> -			return 1;
> -		}
> +	if (!options)
> +		return 0;
>  
> -		if (!strcmp(this_char,"size")) {
> +	while ((p = strsep(&options, ",")) != NULL) {
> +		int token;
> +
> +		if (!*p)
> +			continue;
> +		token = match_token(p, tokens, args);
> +		switch (token) {
> +		case Opt_size: {
>  			unsigned long long size;
> -			size = memparse(value,&rest);
> +			size = memparse(args[0].from, &rest);
>  			if (*rest == '%') {
>  				size <<= PAGE_SHIFT;
>  				size *= totalram_pages;
>  				do_div(size, 100);
>  				rest++;
>  			}
> -			if (*rest)
> -				goto bad_val;
>  			*blocks = size >> PAGE_CACHE_SHIFT;
> -		} else if (!strcmp(this_char,"nr_blocks")) {
> -			*blocks = memparse(value,&rest);
> -			if (*rest)
> -				goto bad_val;
> -		} else if (!strcmp(this_char,"nr_inodes")) {
> -			*inodes = memparse(value,&rest);
> -			if (*rest)
> -				goto bad_val;
> -		} else if (!strcmp(this_char,"mode")) {
> +			break;
> +		}
> +		case Opt_nr_blocks:
> +			*blocks = memparse(args[0].from, &rest);
> +			break;
> +		case Opt_nr_inodes:
> +			*inodes = memparse(args[0].from, &rest);
> +			break;
> +		case Opt_mode:
> +			if (is_remount)		/* not valid on remount */
> +				break;
>  			if (!mode)
> -				continue;
> -			*mode = simple_strtoul(value,&rest,8);
> -			if (*rest)
> +				break;
> +			if (match_int(&args[0], &option))
>  				goto bad_val;
> -		} else if (!strcmp(this_char,"uid")) {
> +			*mode = option;
> +			break;
> +		case Opt_uid:
> +			if (is_remount)		/* not valid on remount */
> +				break;
>  			if (!uid)
> -				continue;
> -			*uid = simple_strtoul(value,&rest,0);
> -			if (*rest)
> +				break;
> +			if (match_int(&args[0], &option))
>  				goto bad_val;
> -		} else if (!strcmp(this_char,"gid")) {
> +			*uid = option;
> +			break;
> +		case Opt_gid:
> +			if (is_remount)		/* not valid on remount */
> +				break;
>  			if (!gid)
> -				continue;
> -			*gid = simple_strtoul(value,&rest,0);
> -			if (*rest)
> +				break;
> +			if (match_int(&args[0], &option))
>  				goto bad_val;
> -		} else if (!strcmp(this_char,"mpol")) {
> -			if (shmem_parse_mpol(value,policy,policy_nodes))
> +			*gid = option;
> +			break;
> +		case Opt_mpol:
> +			if (shmem_parse_mpol(args[0].from, policy, policy_nodes))
>  				goto bad_val;
> -		} else {
> -			printk(KERN_ERR "tmpfs: Bad mount option %s\n",
> -			       this_char);
> +			break;
> +		default:
> +			printk(KERN_ERR "tmpfs: Bad mount option %s\n", p);
>  			return 1;
> +			break;
> +		}
> +
> +		for (;;) {
> +			/*
> +			 * NUL-terminate this option: unfortunately,
> +			 * mount options form a comma-separated list,
> +			 * but mpol's nodelist may also contain commas.
> +			 */
> +			options = strchr(options, ',');
> +			if (options == NULL)
> +				break;
> +			options++;
> +			if (!isdigit(*options)) {
> +				options[-1] = '\0';
> +				break;
> +			}
>  		}
>  	}
>  	return 0;
>  
>  bad_val:
>  	printk(KERN_ERR "tmpfs: Bad value '%s' for mount option '%s'\n",
> -	       value, this_char);
> +	       args[0].from, p);
>  	return 1;
> -
>  }
>  
>  static int shmem_remount_fs(struct super_block *sb, int *flags, char *data)
> @@ -2213,7 +2239,7 @@ static int shmem_remount_fs(struct super
>  	int error = -EINVAL;
>  
>  	if (shmem_parse_options(data, NULL, NULL, NULL, &max_blocks,
> -				&max_inodes, &policy, &policy_nodes))
> +				&max_inodes, &policy, &policy_nodes, 1))
>  		return error;
>  
>  	spin_lock(&sbinfo->stat_lock);
> @@ -2280,7 +2306,7 @@ static int shmem_fill_super(struct super
>  		if (inodes > blocks)
>  			inodes = blocks;
>  		if (shmem_parse_options(data, &mode, &uid, &gid, &blocks,
> -					&inodes, &policy, &policy_nodes))
> +					&inodes, &policy, &policy_nodes, 0))
>  			return -EINVAL;
>  	}
>  	sb->s_export_op = &shmem_export_ops;
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC/PATCH v2] shmem: use lib/parser for mount options

[Randy Dunlap, Randy Dunlap]

Convert shmem (tmpfs) to use the in-kernel mount options parsing library.

Old size: 0x368 = 872 bytes
New size: 0x3b6 = 950 bytes

If you feel that there is no significant advantage to this, that's OK,
I can just drop it.

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 mm/shmem.c |  179 ++++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 120 insertions(+), 59 deletions(-)

--- linux-2622-rc4.orig/mm/shmem.c
+++ linux-2622-rc4/mm/shmem.c
@@ -32,6 +32,7 @@
 #include <linux/mman.h>
 #include <linux/file.h>
 #include <linux/swap.h>
+#include <linux/parser.h>
 #include <linux/pagemap.h>
 #include <linux/string.h>
 #include <linux/slab.h>
@@ -84,6 +85,23 @@ enum sgp_type {
 	SGP_WRITE,	/* may exceed i_size, may allocate page */
 };
 
+enum {
+	Opt_size, Opt_nr_blocks, Opt_nr_inodes,
+	Opt_mode, Opt_uid, Opt_gid,
+	Opt_mpol, Opt_err,
+};
+
+static match_table_t tokens = {
+	{Opt_size,	"size=%s"},
+	{Opt_nr_blocks,	"nr_blocks=%s"},
+	{Opt_nr_inodes,	"nr_inodes=%s"},
+	{Opt_mode,	"mode=%o"},	/* not for remount */
+	{Opt_uid,	"uid=%u"},	/* not for remount */
+	{Opt_gid,	"gid=%u"},	/* not for remount */
+	{Opt_mpol,	"mpol=%s"},	/* various NUMA memory policy options */
+	{Opt_err,	NULL},
+};
+
 static int shmem_getpage(struct inode *inode, unsigned long idx,
 			 struct page **pagep, enum sgp_type sgp, int *type);
 
@@ -2113,92 +2131,135 @@ static struct export_operations shmem_ex
 
 static int shmem_parse_options(char *options, int *mode, uid_t *uid,
 	gid_t *gid, unsigned long *blocks, unsigned long *inodes,
-	int *policy, nodemask_t *policy_nodes)
+	int *policy, nodemask_t *policy_nodes, int is_remount)
 {
-	char *this_char, *value, *rest;
+	char *rest;
+	substring_t args[MAX_OPT_ARGS];
+	char *p, *prev_opt = NULL;
+	int option;
 
-	while (options != NULL) {
-		this_char = options;
-		for (;;) {
-			/*
-			 * NUL-terminate this option: unfortunately,
-			 * mount options form a comma-separated list,
-			 * but mpol's nodelist may also contain commas.
-			 */
-			options = strchr(options, ',');
-			if (options == NULL)
-				break;
-			options++;
-			if (!isdigit(*options)) {
-				options[-1] = '\0';
-				break;
-			}
-		}
-		if (!*this_char)
+	if (!options)
+		return 0;
+
+	while ((p = strsep(&options, ",")) != NULL) {
+		int token;
+
+		if (!*p) {
+			prev_opt = options;
 			continue;
-		if ((value = strchr(this_char,'=')) != NULL) {
-			*value++ = 0;
-		} else {
-			printk(KERN_ERR
-			    "tmpfs: No value for mount option '%s'\n",
-			    this_char);
-			return 1;
 		}
-
-		if (!strcmp(this_char,"size")) {
+		token = match_token(p, tokens, args);
+		switch (token) {
+		case Opt_size: {
 			unsigned long long size;
-			size = memparse(value,&rest);
+			/* memparse() will accept a K/M/G without a digit */
+			if (!isdigit(*args[0].from))
+				goto bad_val;
+			size = memparse(args[0].from, &rest);
 			if (*rest == '%') {
 				size <<= PAGE_SHIFT;
 				size *= totalram_pages;
 				do_div(size, 100);
 				rest++;
 			}
-			if (*rest)
-				goto bad_val;
 			*blocks = size >> PAGE_CACHE_SHIFT;
-		} else if (!strcmp(this_char,"nr_blocks")) {
-			*blocks = memparse(value,&rest);
-			if (*rest)
+			break;
+		}
+		case Opt_nr_blocks:
+			/* memparse() will accept a K/M/G without a digit */
+			if (!isdigit(*args[0].from))
 				goto bad_val;
-		} else if (!strcmp(this_char,"nr_inodes")) {
-			*inodes = memparse(value,&rest);
-			if (*rest)
+			*blocks = memparse(args[0].from, &rest);
+			break;
+		case Opt_nr_inodes:
+			/* memparse() will accept a K/M/G without a digit */
+			if (!isdigit(*args[0].from))
 				goto bad_val;
-		} else if (!strcmp(this_char,"mode")) {
+			*inodes = memparse(args[0].from, &rest);
+			break;
+		case Opt_mode:
+			if (is_remount)		/* not valid on remount */
+				break;
 			if (!mode)
-				continue;
-			*mode = simple_strtoul(value,&rest,8);
-			if (*rest)
+				break;
+			if (match_octal(&args[0], &option))
 				goto bad_val;
-		} else if (!strcmp(this_char,"uid")) {
+			*mode = option;
+			break;
+		case Opt_uid:
+			if (is_remount)		/* not valid on remount */
+				break;
 			if (!uid)
-				continue;
-			*uid = simple_strtoul(value,&rest,0);
-			if (*rest)
+				break;
+			if (match_int(&args[0], &option))
 				goto bad_val;
-		} else if (!strcmp(this_char,"gid")) {
+			*uid = option;
+			break;
+		case Opt_gid:
+			if (is_remount)		/* not valid on remount */
+				break;
 			if (!gid)
-				continue;
-			*gid = simple_strtoul(value,&rest,0);
-			if (*rest)
+				break;
+			if (match_int(&args[0], &option))
 				goto bad_val;
-		} else if (!strcmp(this_char,"mpol")) {
-			if (shmem_parse_mpol(value,policy,policy_nodes))
+			*gid = option;
+			break;
+		case Opt_mpol: {
+			/*
+			 * strsep() broke the mount options string at a comma,
+			 * but tmpfs accepts "mpol=type:nodelist", where
+			 * nodelist may contain commas, so restore the
+			 * comma and then insert a nul char at the end of
+			 * the nodelist. Also update 'options' so that the
+			 * next call to strsep() points to the next mount
+			 * option(s).
+			 */
+			char *delim, *opt = prev_opt + 5; /* skip "mpol=" */
+			char *fixup = NULL; /* temp change nul char to comma */
+
+			if (!options)	/* no fixups needed */
+				goto do_mpol;
+
+			/* there are more options, so put the comma back */
+			fixup = prev_opt + strlen(prev_opt);
+			*fixup = ',';	/* this lets (mpol=)policy[:nodelist] be parsed */
+			/* now find the end of the mpol= option */
+			delim = strchr(prev_opt, ':');
+			if (!delim) { /* no colon, restore nul char, done */
+				*fixup = '\0';
+				goto do_mpol;
+			}
+			/* scan over the node(list) & insert nul char at its end */
+			delim++;	/* past colon */
+			while (*delim) {
+				if (*delim == ',' || isdigit(*delim) || *delim == '-')
+					delim++;
+				else
+					break;
+			}
+			options = delim;	/* for next time in main loop */
+			if (*delim)	/* not end of string */
+				delim[-1] = '\0';
+do_mpol:
+			if (shmem_parse_mpol(opt, policy, policy_nodes))
 				goto bad_val;
-		} else {
-			printk(KERN_ERR "tmpfs: Bad mount option %s\n",
-			       this_char);
+
+			break;
+		}
+		default:
+			printk(KERN_ERR "tmpfs: Bad mount option: %s\n", p);
 			return 1;
+			break;
 		}
+
+		prev_opt = options;
 	}
 	return 0;
 
 bad_val:
 	printk(KERN_ERR "tmpfs: Bad value '%s' for mount option '%s'\n",
-	       value, this_char);
+	       args[0].from, p);
 	return 1;
-
 }
 
 static int shmem_remount_fs(struct super_block *sb, int *flags, char *data)
@@ -2213,7 +2274,7 @@ static int shmem_remount_fs(struct super
 	int error = -EINVAL;
 
 	if (shmem_parse_options(data, NULL, NULL, NULL, &max_blocks,
-				&max_inodes, &policy, &policy_nodes))
+				&max_inodes, &policy, &policy_nodes, 1))
 		return error;
 
 	spin_lock(&sbinfo->stat_lock);
@@ -2280,7 +2341,7 @@ static int shmem_fill_super(struct super
 		if (inodes > blocks)
 			inodes = blocks;
 		if (shmem_parse_options(data, &mode, &uid, &gid, &blocks,
-					&inodes, &policy, &policy_nodes))
+					&inodes, &policy, &policy_nodes, 0))
 			return -EINVAL;
 	}
 	sb->s_export_op = &shmem_export_ops;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC/PATCH v2] shmem: use lib/parser for mount options

[Hugh Dickins]

On Tue, 5 Jun 2007, Randy Dunlap wrote:
> From: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Convert shmem (tmpfs) to use the in-kernel mount options parsing library.
> 
> Old size: 0x368 = 872 bytes
> New size: 0x3b6 = 950 bytes

Varies with arch/config: in some cases the old is smaller,
in other cases your new.  And you're not accounting for (nor
drawing attention to) any bugfixes you added (nor, for that matter,
on any bugs you added - but we usually keep quiet about those ;)

> If you feel that there is no significant advantage to this, that's OK,
> I can just drop it.

Hmm.  Hmm.  My own personal feeling is that it's not really an
improvement; especially the Opt_mpol block (and the unnecessary
is_remount arg I already commented on).

But I'm familiar with what's already there: I'd happily be overruled
on this if others feel yours really is an improvement.  Anyone? 
And you've cleaned up that "no space after comma" coding style.

For me, the main question is, did you fix any bugs?  You certainly
discovered the nonline mpol crash, which was worthwhile in itself.
And you've discovered that memparse accepts k, M, G without a digit:
if it treated that as 1 I wouldn't mind so much, but it treats it as 0.

We can live with that; but if we're to fix it, I'd prefer the fix to
go into memparse itself - though it's called from many places, so
maybe there's an audit job to see if the present behaviour could
make sense in any of them.

Hugh

> 
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
> ---
>  mm/shmem.c |  179 ++++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 120 insertions(+), 59 deletions(-)
> 
> --- linux-2622-rc4.orig/mm/shmem.c
> +++ linux-2622-rc4/mm/shmem.c
> @@ -32,6 +32,7 @@
>  #include <linux/mman.h>
>  #include <linux/file.h>
>  #include <linux/swap.h>
> +#include <linux/parser.h>
>  #include <linux/pagemap.h>
>  #include <linux/string.h>
>  #include <linux/slab.h>
> @@ -84,6 +85,23 @@ enum sgp_type {
>  	SGP_WRITE,	/* may exceed i_size, may allocate page */
>  };
>  
> +enum {
> +	Opt_size, Opt_nr_blocks, Opt_nr_inodes,
> +	Opt_mode, Opt_uid, Opt_gid,
> +	Opt_mpol, Opt_err,
> +};
> +
> +static match_table_t tokens = {
> +	{Opt_size,	"size=%s"},
> +	{Opt_nr_blocks,	"nr_blocks=%s"},
> +	{Opt_nr_inodes,	"nr_inodes=%s"},
> +	{Opt_mode,	"mode=%o"},	/* not for remount */
> +	{Opt_uid,	"uid=%u"},	/* not for remount */
> +	{Opt_gid,	"gid=%u"},	/* not for remount */
> +	{Opt_mpol,	"mpol=%s"},	/* various NUMA memory policy options */
> +	{Opt_err,	NULL},
> +};
> +
>  static int shmem_getpage(struct inode *inode, unsigned long idx,
>  			 struct page **pagep, enum sgp_type sgp, int *type);
>  
> @@ -2113,92 +2131,135 @@ static struct export_operations shmem_ex
>  
>  static int shmem_parse_options(char *options, int *mode, uid_t *uid,
>  	gid_t *gid, unsigned long *blocks, unsigned long *inodes,
> -	int *policy, nodemask_t *policy_nodes)
> +	int *policy, nodemask_t *policy_nodes, int is_remount)
>  {
> -	char *this_char, *value, *rest;
> +	char *rest;
> +	substring_t args[MAX_OPT_ARGS];
> +	char *p, *prev_opt = NULL;
> +	int option;
>  
> -	while (options != NULL) {
> -		this_char = options;
> -		for (;;) {
> -			/*
> -			 * NUL-terminate this option: unfortunately,
> -			 * mount options form a comma-separated list,
> -			 * but mpol's nodelist may also contain commas.
> -			 */
> -			options = strchr(options, ',');
> -			if (options == NULL)
> -				break;
> -			options++;
> -			if (!isdigit(*options)) {
> -				options[-1] = '\0';
> -				break;
> -			}
> -		}
> -		if (!*this_char)
> +	if (!options)
> +		return 0;
> +
> +	while ((p = strsep(&options, ",")) != NULL) {
> +		int token;
> +
> +		if (!*p) {
> +			prev_opt = options;
>  			continue;
> -		if ((value = strchr(this_char,'=')) != NULL) {
> -			*value++ = 0;
> -		} else {
> -			printk(KERN_ERR
> -			    "tmpfs: No value for mount option '%s'\n",
> -			    this_char);
> -			return 1;
>  		}
> -
> -		if (!strcmp(this_char,"size")) {
> +		token = match_token(p, tokens, args);
> +		switch (token) {
> +		case Opt_size: {
>  			unsigned long long size;
> -			size = memparse(value,&rest);
> +			/* memparse() will accept a K/M/G without a digit */
> +			if (!isdigit(*args[0].from))
> +				goto bad_val;
> +			size = memparse(args[0].from, &rest);
>  			if (*rest == '%') {
>  				size <<= PAGE_SHIFT;
>  				size *= totalram_pages;
>  				do_div(size, 100);
>  				rest++;
>  			}
> -			if (*rest)
> -				goto bad_val;
>  			*blocks = size >> PAGE_CACHE_SHIFT;
> -		} else if (!strcmp(this_char,"nr_blocks")) {
> -			*blocks = memparse(value,&rest);
> -			if (*rest)
> +			break;
> +		}
> +		case Opt_nr_blocks:
> +			/* memparse() will accept a K/M/G without a digit */
> +			if (!isdigit(*args[0].from))
>  				goto bad_val;
> -		} else if (!strcmp(this_char,"nr_inodes")) {
> -			*inodes = memparse(value,&rest);
> -			if (*rest)
> +			*blocks = memparse(args[0].from, &rest);
> +			break;
> +		case Opt_nr_inodes:
> +			/* memparse() will accept a K/M/G without a digit */
> +			if (!isdigit(*args[0].from))
>  				goto bad_val;
> -		} else if (!strcmp(this_char,"mode")) {
> +			*inodes = memparse(args[0].from, &rest);
> +			break;
> +		case Opt_mode:
> +			if (is_remount)		/* not valid on remount */
> +				break;
>  			if (!mode)
> -				continue;
> -			*mode = simple_strtoul(value,&rest,8);
> -			if (*rest)
> +				break;
> +			if (match_octal(&args[0], &option))
>  				goto bad_val;
> -		} else if (!strcmp(this_char,"uid")) {
> +			*mode = option;
> +			break;
> +		case Opt_uid:
> +			if (is_remount)		/* not valid on remount */
> +				break;
>  			if (!uid)
> -				continue;
> -			*uid = simple_strtoul(value,&rest,0);
> -			if (*rest)
> +				break;
> +			if (match_int(&args[0], &option))
>  				goto bad_val;
> -		} else if (!strcmp(this_char,"gid")) {
> +			*uid = option;
> +			break;
> +		case Opt_gid:
> +			if (is_remount)		/* not valid on remount */
> +				break;
>  			if (!gid)
> -				continue;
> -			*gid = simple_strtoul(value,&rest,0);
> -			if (*rest)
> +				break;
> +			if (match_int(&args[0], &option))
>  				goto bad_val;
> -		} else if (!strcmp(this_char,"mpol")) {
> -			if (shmem_parse_mpol(value,policy,policy_nodes))
> +			*gid = option;
> +			break;
> +		case Opt_mpol: {
> +			/*
> +			 * strsep() broke the mount options string at a comma,
> +			 * but tmpfs accepts "mpol=type:nodelist", where
> +			 * nodelist may contain commas, so restore the
> +			 * comma and then insert a nul char at the end of
> +			 * the nodelist. Also update 'options' so that the
> +			 * next call to strsep() points to the next mount
> +			 * option(s).
> +			 */
> +			char *delim, *opt = prev_opt + 5; /* skip "mpol=" */
> +			char *fixup = NULL; /* temp change nul char to comma */
> +
> +			if (!options)	/* no fixups needed */
> +				goto do_mpol;
> +
> +			/* there are more options, so put the comma back */
> +			fixup = prev_opt + strlen(prev_opt);
> +			*fixup = ',';	/* this lets (mpol=)policy[:nodelist] be parsed */
> +			/* now find the end of the mpol= option */
> +			delim = strchr(prev_opt, ':');
> +			if (!delim) { /* no colon, restore nul char, done */
> +				*fixup = '\0';
> +				goto do_mpol;
> +			}
> +			/* scan over the node(list) & insert nul char at its end */
> +			delim++;	/* past colon */
> +			while (*delim) {
> +				if (*delim == ',' || isdigit(*delim) || *delim == '-')
> +					delim++;
> +				else
> +					break;
> +			}
> +			options = delim;	/* for next time in main loop */
> +			if (*delim)	/* not end of string */
> +				delim[-1] = '\0';
> +do_mpol:
> +			if (shmem_parse_mpol(opt, policy, policy_nodes))
>  				goto bad_val;
> -		} else {
> -			printk(KERN_ERR "tmpfs: Bad mount option %s\n",
> -			       this_char);
> +
> +			break;
> +		}
> +		default:
> +			printk(KERN_ERR "tmpfs: Bad mount option: %s\n", p);
>  			return 1;
> +			break;
>  		}
> +
> +		prev_opt = options;
>  	}
>  	return 0;
>  
>  bad_val:
>  	printk(KERN_ERR "tmpfs: Bad value '%s' for mount option '%s'\n",
> -	       value, this_char);
> +	       args[0].from, p);
>  	return 1;
> -
>  }
>  
>  static int shmem_remount_fs(struct super_block *sb, int *flags, char *data)
> @@ -2213,7 +2274,7 @@ static int shmem_remount_fs(struct super
>  	int error = -EINVAL;
>  
>  	if (shmem_parse_options(data, NULL, NULL, NULL, &max_blocks,
> -				&max_inodes, &policy, &policy_nodes))
> +				&max_inodes, &policy, &policy_nodes, 1))
>  		return error;
>  
>  	spin_lock(&sbinfo->stat_lock);
> @@ -2280,7 +2341,7 @@ static int shmem_fill_super(struct super
>  		if (inodes > blocks)
>  			inodes = blocks;
>  		if (shmem_parse_options(data, &mode, &uid, &gid, &blocks,
> -					&inodes, &policy, &policy_nodes))
> +					&inodes, &policy, &policy_nodes, 0))
>  			return -EINVAL;
>  	}
>  	sb->s_export_op = &shmem_export_ops;
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC/PATCH v2] shmem: use lib/parser for mount options

[Randy Dunlap]

On Thu, 7 Jun 2007 19:57:18 +0100 (BST) Hugh Dickins wrote:

> On Tue, 5 Jun 2007, Randy Dunlap wrote:
> > From: Randy Dunlap <randy.dunlap@oracle.com>
> > 
> > Convert shmem (tmpfs) to use the in-kernel mount options parsing library.
> > 
> > Old size: 0x368 = 872 bytes
> > New size: 0x3b6 = 950 bytes
> 
> Varies with arch/config: in some cases the old is smaller,
> in other cases your new.  And you're not accounting for (nor
> drawing attention to) any bugfixes you added (nor, for that matter,
> on any bugs you added - but we usually keep quiet about those ;)
> 
> > If you feel that there is no significant advantage to this, that's OK,
> > I can just drop it.
> 
> Hmm.  Hmm.  My own personal feeling is that it's not really an
> improvement; especially the Opt_mpol block (and the unnecessary
> is_remount arg I already commented on).

Yes, I reread that comment earlier today.

The mpol=args parameter string is messy, due to the commas that
may be in it, whereas the old & new parsers like to split options
at commas.


> But I'm familiar with what's already there: I'd happily be overruled
> on this if others feel yours really is an improvement.  Anyone? 
> And you've cleaned up that "no space after comma" coding style.
>  	 
> For me, the main question is, did you fix any bugs?  You certainly
> discovered the nonline mpol crash, which was worthwhile in itself.
> And you've discovered that memparse accepts k, M, G without a digit:
> if it treated that as 1 I wouldn't mind so much, but it treats it as 0.

Right.  memparse could be fixed, or lib/parser.c could gain some
additions/extensions, such as support for memparse and long long
option values.  I think that those would be useful additions.

> We can live with that; but if we're to fix it, I'd prefer the fix to
> go into memparse itself - though it's called from many places, so
> maybe there's an audit job to see if the present behaviour could
> make sense in any of them.
> 
> Hugh

I can't convince myself that it's worth the change...  :)

Thanks for looking.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>