[PATCH 1/7] fat: Fix parse_options()

[PATCH 1/7] fat: Fix parse_options()

[OGAWA Hirofumi]

Current parse_options() exits too early. We need to run the code of
bottom in this function even if users doesn't specify options.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/fat/inode.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff -puN fs/fat/inode.c~fat_default_opt-fix fs/fat/inode.c
--- linux-2.6/fs/fat/inode.c~fat_default_opt-fix	2008-06-08 23:05:53.000000000 +0900
+++ linux-2.6-hirofumi/fs/fat/inode.c	2008-06-08 23:05:53.000000000 +0900
@@ -950,7 +950,7 @@ static int parse_options(char *options, 
 	*debug = 0;
 
 	if (!options)
-		return 0;
+		goto out;
 
 	while ((p = strsep(&options, ",")) != NULL) {
 		int token;
@@ -1104,10 +1104,13 @@ static int parse_options(char *options, 
 			return -EINVAL;
 		}
 	}
+
+out:
 	/* UTF-8 doesn't provide FAT semantics */
 	if (!strcmp(opts->iocharset, "utf8")) {
 		printk(KERN_ERR "FAT: utf8 is not a recommended IO charset"
-		       " for FAT filesystems, filesystem will be case sensitive!\n");
+		       " for FAT filesystems, filesystem will be "
+		       "case sensitive!\n");
 	}
 
 	/* If user doesn't specify allow_utime, it's initialized from dmask. */
_

[PATCH 5/7] fat: use same logic in fat_search_long() and __fat_readdir()

[OGAWA Hirofumi]

This uses uses stack for shortname, and uses __getname() for longname
in fat_search_long() and __fat_readdir(). By this, it removes
unneeded __getname() for shortname.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/fat/dir.c |   42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff -puN fs/fat/dir.c~fat_dir-mem-cleanup fs/fat/dir.c
--- linux-2.6/fs/fat/dir.c~fat_dir-mem-cleanup	2008-07-01 10:01:52.000000000 +0900
+++ linux-2.6-hirofumi/fs/fat/dir.c	2008-07-01 10:01:52.000000000 +0900
@@ -325,6 +325,19 @@ parse_long:
 }
 
 /*
+ * Maximum buffer size of short name.
+ * [(MSDOS_NAME + '.') * max one char + nul]
+ * For msdos style, ['.' (hidden) + MSDOS_NAME + '.' + nul]
+ */
+#define FAT_MAX_SHORT_SIZE	((MSDOS_NAME + 1) * NLS_MAX_CHARSET_SIZE + 1)
+/*
+ * Maximum buffer size of unicode chars from slots.
+ * [(max longname slots * 13 (size in a slot) + nul) * sizeof(wchar_t)]
+ */
+#define FAT_MAX_UNI_CHARS	((MSDOS_SLOTS - 1) * 13 + 1)
+#define FAT_MAX_UNI_SIZE	(FAT_MAX_UNI_CHARS * sizeof(wchar_t))
+
+/*
  * Return values: negative -> error, 0 -> not found, positive -> found,
  * value is the total amount of slots, including the shortname entry.
  */
@@ -340,15 +353,11 @@ int fat_search_long(struct inode *inode,
 	wchar_t bufuname[14];
 	wchar_t *unicode = NULL;
 	unsigned char work[MSDOS_NAME];
-	unsigned char *bufname = NULL;
+	unsigned char bufname[FAT_MAX_SHORT_SIZE];
 	unsigned short opt_shortname = sbi->options.shortname;
 	loff_t cpos = 0;
 	int chl, i, j, last_u, err, len;
 
-	bufname = __getname();
-	if (!bufname)
-		return -ENOMEM;
-
 	err = -ENOENT;
 	while (1) {
 		if (fat_get_entry(inode, &cpos, &bh, &de) == -1)
@@ -414,14 +423,17 @@ parse_record:
 
 		/* Compare shortname */
 		bufuname[last_u] = 0x0000;
-		len = fat_uni_to_x8(sbi, bufuname, bufname, PATH_MAX);
+		len = fat_uni_to_x8(sbi, bufuname, bufname, sizeof(bufname));
 		if (fat_name_match(sbi, name, name_len, bufname, len))
 			goto found;
 
 		if (nr_slots) {
+			void *longname = unicode + FAT_MAX_UNI_CHARS;
+			int size = PATH_MAX - FAT_MAX_UNI_SIZE;
+
 			/* Compare longname */
-			len = fat_uni_to_x8(sbi, unicode, bufname, PATH_MAX);
-			if (fat_name_match(sbi, name, name_len, bufname, len))
+			len = fat_uni_to_x8(sbi, unicode, longname, size);
+			if (fat_name_match(sbi, name, name_len, longname, len))
 				goto found;
 		}
 	}
@@ -435,8 +447,6 @@ found:
 	sinfo->i_pos = fat_make_i_pos(sb, sinfo->bh, sinfo->de);
 	err = 0;
 end_of_dir:
-	if (bufname)
-		__putname(bufname);
 	if (unicode)
 		__putname(unicode);
 
@@ -466,7 +476,8 @@ static int __fat_readdir(struct inode *i
 	unsigned char nr_slots;
 	wchar_t bufuname[14];
 	wchar_t *unicode = NULL;
-	unsigned char c, work[MSDOS_NAME], bufname[56], *ptname = bufname;
+	unsigned char c, work[MSDOS_NAME];
+	unsigned char bufname[FAT_MAX_SHORT_SIZE], *ptname = bufname;
 	unsigned short opt_shortname = sbi->options.shortname;
 	int isvfat = sbi->options.isvfat;
 	int nocase = sbi->options.nocase;
@@ -620,11 +631,10 @@ parse_record:
 	fill_name = bufname;
 	fill_len = i;
 	if (!short_only && nr_slots) {
-		/* convert the unicode long name. 261 is maximum size
-		 * of unicode buffer. (13 * slots + nul) */
-		void *longname = unicode + 261;
-		int buf_size = PATH_MAX - (261 * sizeof(unicode[0]));
-		int long_len = fat_uni_to_x8(sbi, unicode, longname, buf_size);
+		void *longname = unicode + FAT_MAX_UNI_CHARS;
+		int long_len, size = PATH_MAX - FAT_MAX_UNI_SIZE;
+
+		long_len = fat_uni_to_x8(sbi, unicode, longname, size);
 
 		if (!both) {
 			fill_name = longname;
_

[PATCH 6/7] fat: small optimaize __fat_readdir()

[OGAWA Hirofumi]

This removes unnecessary parsing for directory entries.

If short_only, we don't need to parse longname. And if !both and it
found the longname, we don't need shortname.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/fat/dir.c |   71 +++++++++++++++++++++++++++++++---------------------------
 1 file changed, 39 insertions(+), 32 deletions(-)

diff -puN fs/fat/dir.c~fat_dir-optimize fs/fat/dir.c
--- linux-2.6/fs/fat/dir.c~fat_dir-optimize	2008-07-01 10:12:23.000000000 +0900
+++ linux-2.6-hirofumi/fs/fat/dir.c	2008-07-01 10:12:23.000000000 +0900
@@ -481,11 +481,11 @@ static int __fat_readdir(struct inode *i
 	unsigned short opt_shortname = sbi->options.shortname;
 	int isvfat = sbi->options.isvfat;
 	int nocase = sbi->options.nocase;
-	const char *fill_name;
+	const char *fill_name = NULL;
 	unsigned long inum;
 	unsigned long lpos, dummy, *furrfu = &lpos;
 	loff_t cpos;
-	int chi, chl, i, i2, j, last, last_u, dotoffset = 0, fill_len;
+	int chi, chl, i, i2, j, last, last_u, dotoffset = 0, fill_len = 0;
 	int ret = 0;
 
 	lock_kernel();
@@ -516,8 +516,11 @@ get_new:
 		goto end_of_dir;
 parse_record:
 	nr_slots = 0;
-	/* Check for long filename entry */
-	if (isvfat) {
+	/*
+	 * Check for long filename entry, but if short_only, we don't
+	 * need to parse long filename.
+	 */
+	if (isvfat && !short_only) {
 		if (de->name[0] == DELETED_FLAG)
 			goto record_end;
 		if (de->attr != ATTR_EXT && (de->attr & ATTR_VOLUME))
@@ -542,6 +545,18 @@ parse_record:
 			goto parse_record;
 		else if (status == PARSE_EOF)
 			goto end_of_dir;
+
+		if (nr_slots) {
+			void *longname = unicode + FAT_MAX_UNI_CHARS;
+			int size = PATH_MAX - FAT_MAX_UNI_SIZE;
+			int len = fat_uni_to_x8(sbi, unicode, longname, size);
+
+			fill_name = longname;
+			fill_len = len;
+			/* !both && !short_only, so we don't need shortname. */
+			if (!both)
+				goto start_filldir;
+		}
 	}
 
 	if (sbi->options.dotsOK) {
@@ -608,6 +623,26 @@ parse_record:
 	i = last + dotoffset;
 	j = last_u;
 
+	if (isvfat) {
+		bufuname[j] = 0x0000;
+		i = fat_uni_to_x8(sbi, bufuname, bufname, sizeof(bufname));
+	}
+	if (nr_slots) {
+		/* hack for fat_ioctl_filldir() */
+		struct fat_ioctl_filldir_callback *p = dirent;
+
+		p->longname = fill_name;
+		p->long_len = fill_len;
+		p->shortname = bufname;
+		p->short_len = i;
+		fill_name = NULL;
+		fill_len = 0;
+	} else {
+		fill_name = bufname;
+		fill_len = i;
+	}
+
+start_filldir:
 	lpos = cpos - (nr_slots + 1) * sizeof(struct msdos_dir_entry);
 	if (!memcmp(de->name, MSDOS_DOT, MSDOS_NAME))
 		inum = inode->i_ino;
@@ -623,34 +658,6 @@ parse_record:
 			inum = iunique(sb, MSDOS_ROOT_INO);
 	}
 
-	if (isvfat) {
-		bufuname[j] = 0x0000;
-		i = fat_uni_to_x8(sbi, bufuname, bufname, sizeof(bufname));
-	}
-
-	fill_name = bufname;
-	fill_len = i;
-	if (!short_only && nr_slots) {
-		void *longname = unicode + FAT_MAX_UNI_CHARS;
-		int long_len, size = PATH_MAX - FAT_MAX_UNI_SIZE;
-
-		long_len = fat_uni_to_x8(sbi, unicode, longname, size);
-
-		if (!both) {
-			fill_name = longname;
-			fill_len = long_len;
-		} else {
-			/* hack for fat_ioctl_filldir() */
-			struct fat_ioctl_filldir_callback *p = dirent;
-
-			p->longname = longname;
-			p->long_len = long_len;
-			p->shortname = bufname;
-			p->short_len = i;
-			fill_name = NULL;
-			fill_len = 0;
-		}
-	}
 	if (filldir(dirent, fill_name, fill_len, *furrfu, inum,
 		    (de->attr & ATTR_DIR) ? DT_DIR : DT_REG) < 0)
 		goto fill_failed;
_

[PATCH 4/7] fat: cleanup fs/fat/dir.c

[OGAWA Hirofumi]

This is no logic changes, just cleans fs/fat/dir.c up.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/fat/dir.c |  131 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 67 insertions(+), 64 deletions(-)

diff -puN fs/fat/dir.c~fat_dir-cleanup fs/fat/dir.c
--- linux-2.6/fs/fat/dir.c~fat_dir-cleanup	2008-07-01 09:55:52.000000000 +0900
+++ linux-2.6-hirofumi/fs/fat/dir.c	2008-07-01 09:55:52.000000000 +0900
@@ -123,10 +123,11 @@ static inline int fat_get_entry(struct i
  * but ignore that right now.
  * Ahem... Stack smashing in ring 0 isn't fun. Fixed.
  */
-static int uni16_to_x8(unsigned char *ascii, wchar_t *uni, int len,
+static int uni16_to_x8(unsigned char *ascii, const wchar_t *uni, int len,
 		       int uni_xlate, struct nls_table *nls)
 {
-	wchar_t *ip, ec;
+	const wchar_t *ip;
+	wchar_t ec;
 	unsigned char *op, nc;
 	int charlen;
 	int k;
@@ -166,6 +167,16 @@ static int uni16_to_x8(unsigned char *as
 	return (op - ascii);
 }
 
+static inline int fat_uni_to_x8(struct msdos_sb_info *sbi, const wchar_t *uni,
+				unsigned char *buf, int size)
+{
+	if (sbi->options.utf8)
+		return utf8_wcstombs(buf, uni, size);
+	else
+		return uni16_to_x8(buf, uni, size, sbi->options.unicode_xlate,
+				   sbi->nls_io);
+}
+
 static inline int
 fat_short2uni(struct nls_table *t, unsigned char *c, int clen, wchar_t *uni)
 {
@@ -226,6 +237,19 @@ fat_shortname2uni(struct nls_table *nls,
 	return len;
 }
 
+static inline int fat_name_match(struct msdos_sb_info *sbi,
+				 const unsigned char *a, int a_len,
+				 const unsigned char *b, int b_len)
+{
+	if (a_len != b_len)
+		return 0;
+
+	if (sbi->options.name_check != 's')
+		return !nls_strnicmp(sbi->nls_io, a, b, a_len);
+	else
+		return !memcmp(a, b, a_len);
+}
+
 enum { PARSE_INVALID = 1, PARSE_NOT_LONGNAME, PARSE_EOF, };
 
 /**
@@ -311,29 +335,24 @@ int fat_search_long(struct inode *inode,
 	struct msdos_sb_info *sbi = MSDOS_SB(sb);
 	struct buffer_head *bh = NULL;
 	struct msdos_dir_entry *de;
-	struct nls_table *nls_io = sbi->nls_io;
 	struct nls_table *nls_disk = sbi->nls_disk;
-	wchar_t bufuname[14];
 	unsigned char nr_slots;
-	int xlate_len;
+	wchar_t bufuname[14];
 	wchar_t *unicode = NULL;
 	unsigned char work[MSDOS_NAME];
 	unsigned char *bufname = NULL;
-	int uni_xlate = sbi->options.unicode_xlate;
-	int utf8 = sbi->options.utf8;
-	int anycase = (sbi->options.name_check != 's');
 	unsigned short opt_shortname = sbi->options.shortname;
 	loff_t cpos = 0;
-	int chl, i, j, last_u, err;
+	int chl, i, j, last_u, err, len;
 
 	bufname = __getname();
 	if (!bufname)
 		return -ENOMEM;
 
 	err = -ENOENT;
-	while(1) {
+	while (1) {
 		if (fat_get_entry(inode, &cpos, &bh, &de) == -1)
-			goto EODir;
+			goto end_of_dir;
 parse_record:
 		nr_slots = 0;
 		if (de->name[0] == DELETED_FLAG)
@@ -352,7 +371,7 @@ parse_record:
 			else if (status == PARSE_NOT_LONGNAME)
 				goto parse_record;
 			else if (status == PARSE_EOF)
-				goto EODir;
+				goto end_of_dir;
 		}
 
 		memcpy(work, de->name, sizeof(de->name));
@@ -393,30 +412,21 @@ parse_record:
 		if (!last_u)
 			continue;
 
+		/* Compare shortname */
 		bufuname[last_u] = 0x0000;
-		xlate_len = utf8
-			?utf8_wcstombs(bufname, bufuname, PATH_MAX)
-			:uni16_to_x8(bufname, bufuname, PATH_MAX, uni_xlate, nls_io);
-		if (xlate_len == name_len)
-			if ((!anycase && !memcmp(name, bufname, xlate_len)) ||
-			    (anycase && !nls_strnicmp(nls_io, name, bufname,
-								xlate_len)))
-				goto Found;
+		len = fat_uni_to_x8(sbi, bufuname, bufname, PATH_MAX);
+		if (fat_name_match(sbi, name, name_len, bufname, len))
+			goto found;
 
 		if (nr_slots) {
-			xlate_len = utf8
-				?utf8_wcstombs(bufname, unicode, PATH_MAX)
-				:uni16_to_x8(bufname, unicode, PATH_MAX, uni_xlate, nls_io);
-			if (xlate_len != name_len)
-				continue;
-			if ((!anycase && !memcmp(name, bufname, xlate_len)) ||
-			    (anycase && !nls_strnicmp(nls_io, name, bufname,
-								xlate_len)))
-				goto Found;
+			/* Compare longname */
+			len = fat_uni_to_x8(sbi, unicode, bufname, PATH_MAX);
+			if (fat_name_match(sbi, name, name_len, bufname, len))
+				goto found;
 		}
 	}
 
-Found:
+found:
 	nr_slots++;	/* include the de */
 	sinfo->slot_off = cpos - nr_slots * sizeof(*de);
 	sinfo->nr_slots = nr_slots;
@@ -424,7 +434,7 @@ Found:
 	sinfo->bh = bh;
 	sinfo->i_pos = fat_make_i_pos(sb, sinfo->bh, sinfo->de);
 	err = 0;
-EODir:
+end_of_dir:
 	if (bufname)
 		__putname(bufname);
 	if (unicode)
@@ -452,23 +462,19 @@ static int __fat_readdir(struct inode *i
 	struct msdos_sb_info *sbi = MSDOS_SB(sb);
 	struct buffer_head *bh;
 	struct msdos_dir_entry *de;
-	struct nls_table *nls_io = sbi->nls_io;
 	struct nls_table *nls_disk = sbi->nls_disk;
-	unsigned char long_slots;
-	const char *fill_name;
-	int fill_len;
+	unsigned char nr_slots;
 	wchar_t bufuname[14];
 	wchar_t *unicode = NULL;
 	unsigned char c, work[MSDOS_NAME], bufname[56], *ptname = bufname;
-	unsigned long lpos, dummy, *furrfu = &lpos;
-	int uni_xlate = sbi->options.unicode_xlate;
+	unsigned short opt_shortname = sbi->options.shortname;
 	int isvfat = sbi->options.isvfat;
-	int utf8 = sbi->options.utf8;
 	int nocase = sbi->options.nocase;
-	unsigned short opt_shortname = sbi->options.shortname;
+	const char *fill_name;
 	unsigned long inum;
-	int chi, chl, i, i2, j, last, last_u, dotoffset = 0;
+	unsigned long lpos, dummy, *furrfu = &lpos;
 	loff_t cpos;
+	int chi, chl, i, i2, j, last, last_u, dotoffset = 0, fill_len;
 	int ret = 0;
 
 	lock_kernel();
@@ -488,43 +494,43 @@ static int __fat_readdir(struct inode *i
 			cpos = 0;
 		}
 	}
-	if (cpos & (sizeof(struct msdos_dir_entry)-1)) {
+	if (cpos & (sizeof(struct msdos_dir_entry) - 1)) {
 		ret = -ENOENT;
 		goto out;
 	}
 
 	bh = NULL;
-GetNew:
+get_new:
 	if (fat_get_entry(inode, &cpos, &bh, &de) == -1)
-		goto EODir;
+		goto end_of_dir;
 parse_record:
-	long_slots = 0;
+	nr_slots = 0;
 	/* Check for long filename entry */
 	if (isvfat) {
 		if (de->name[0] == DELETED_FLAG)
-			goto RecEnd;
+			goto record_end;
 		if (de->attr != ATTR_EXT && (de->attr & ATTR_VOLUME))
-			goto RecEnd;
+			goto record_end;
 		if (de->attr != ATTR_EXT && IS_FREE(de->name))
-			goto RecEnd;
+			goto record_end;
 	} else {
 		if ((de->attr & ATTR_VOLUME) || IS_FREE(de->name))
-			goto RecEnd;
+			goto record_end;
 	}
 
 	if (isvfat && de->attr == ATTR_EXT) {
 		int status = fat_parse_long(inode, &cpos, &bh, &de,
-					    &unicode, &long_slots);
+					    &unicode, &nr_slots);
 		if (status < 0) {
 			filp->f_pos = cpos;
 			ret = status;
 			goto out;
 		} else if (status == PARSE_INVALID)
-			goto RecEnd;
+			goto record_end;
 		else if (status == PARSE_NOT_LONGNAME)
 			goto parse_record;
 		else if (status == PARSE_EOF)
-			goto EODir;
+			goto end_of_dir;
 	}
 
 	if (sbi->options.dotsOK) {
@@ -586,12 +592,12 @@ parse_record:
 		}
 	}
 	if (!last)
-		goto RecEnd;
+		goto record_end;
 
 	i = last + dotoffset;
 	j = last_u;
 
-	lpos = cpos - (long_slots+1)*sizeof(struct msdos_dir_entry);
+	lpos = cpos - (nr_slots + 1) * sizeof(struct msdos_dir_entry);
 	if (!memcmp(de->name, MSDOS_DOT, MSDOS_NAME))
 		inum = inode->i_ino;
 	else if (!memcmp(de->name, MSDOS_DOTDOT, MSDOS_NAME)) {
@@ -608,20 +614,17 @@ parse_record:
 
 	if (isvfat) {
 		bufuname[j] = 0x0000;
-		i = utf8 ? utf8_wcstombs(bufname, bufuname, sizeof(bufname))
-			 : uni16_to_x8(bufname, bufuname, sizeof(bufname), uni_xlate, nls_io);
+		i = fat_uni_to_x8(sbi, bufuname, bufname, sizeof(bufname));
 	}
 
 	fill_name = bufname;
 	fill_len = i;
-	if (!short_only && long_slots) {
+	if (!short_only && nr_slots) {
 		/* convert the unicode long name. 261 is maximum size
 		 * of unicode buffer. (13 * slots + nul) */
 		void *longname = unicode + 261;
 		int buf_size = PATH_MAX - (261 * sizeof(unicode[0]));
-		int long_len = utf8
-			? utf8_wcstombs(longname, unicode, buf_size)
-			: uni16_to_x8(longname, unicode, buf_size, uni_xlate, nls_io);
+		int long_len = fat_uni_to_x8(sbi, unicode, longname, buf_size);
 
 		if (!both) {
 			fill_name = longname;
@@ -640,15 +643,15 @@ parse_record:
 	}
 	if (filldir(dirent, fill_name, fill_len, *furrfu, inum,
 		    (de->attr & ATTR_DIR) ? DT_DIR : DT_REG) < 0)
-		goto FillFailed;
+		goto fill_failed;
 
-RecEnd:
+record_end:
 	furrfu = &lpos;
 	filp->f_pos = cpos;
-	goto GetNew;
-EODir:
+	goto get_new;
+end_of_dir:
 	filp->f_pos = cpos;
-FillFailed:
+fill_failed:
 	brelse(bh);
 	if (unicode)
 		__putname(unicode);
_

[PATCH 3/7] fat/dir.c: switch to struct __fat_dirent

[OGAWA Hirofumi]

struct __fat_dirent is what was formerly the kernel struct dirent
(that was different from the userspace struct dirent).

Converting all fat users to struct __fat_dirent will allow us to get 
rid of the conflicting struct dirent definition.

Signed-off-by: Adrian Bunk <bunk@kernel.org>
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/fat/dir.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff -puN fs/fat/dir.c~fat_use-fat_fs_dirent fs/fat/dir.c
--- linux-2.6/fs/fat/dir.c~fat_use-fat_fs_dirent	2008-07-01 09:50:49.000000000 +0900
+++ linux-2.6-hirofumi/fs/fat/dir.c	2008-07-01 11:34:35.000000000 +0900
@@ -17,7 +17,6 @@
 #include <linux/slab.h>
 #include <linux/time.h>
 #include <linux/msdos_fs.h>
-#include <linux/dirent.h>
 #include <linux/smp_lock.h>
 #include <linux/buffer_head.h>
 #include <linux/compat.h>
@@ -715,7 +714,7 @@ efault:									   \
 	return -EFAULT;							   \
 }
 
-FAT_IOCTL_FILLDIR_FUNC(fat_ioctl_filldir, dirent)
+FAT_IOCTL_FILLDIR_FUNC(fat_ioctl_filldir, __fat_dirent)
 
 static int fat_ioctl_readdir(struct inode *inode, struct file *filp,
 			     void __user *dirent, filldir_t filldir,
@@ -741,7 +740,7 @@ static int fat_ioctl_readdir(struct inod
 static int fat_dir_ioctl(struct inode *inode, struct file *filp,
 			 unsigned int cmd, unsigned long arg)
 {
-	struct dirent __user *d1 = (struct dirent __user *)arg;
+	struct __fat_dirent __user *d1 = (struct __fat_dirent __user *)arg;
 	int short_only, both;
 
 	switch (cmd) {
@@ -757,7 +756,7 @@ static int fat_dir_ioctl(struct inode *i
 		return fat_generic_ioctl(inode, filp, cmd, arg);
 	}
 
-	if (!access_ok(VERIFY_WRITE, d1, sizeof(struct dirent[2])))
+	if (!access_ok(VERIFY_WRITE, d1, sizeof(struct __fat_dirent[2])))
 		return -EFAULT;
 	/*
 	 * Yes, we don't need this put_user() absolutely. However old
_

[PATCH 2/7] fat: Fix VFAT_IOCTL_READDIR_xxx and cleanup for userland

[OGAWA Hirofumi]

"struct dirent" is a kernel type here, but is a **different type** in 
userspace!  This means both the structure and the IOCTL number is wrong!

So, this adds new "struct __fat_dirent" to generate correct IOCTL
number.  And kernel stuff moves to under __KERNEL__.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 include/linux/msdos_fs.h |   47 ++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff -puN include/linux/msdos_fs.h~fat_msdos_fs_h-cleanup include/linux/msdos_fs.h
--- linux-2.6/include/linux/msdos_fs.h~fat_msdos_fs_h-cleanup	2008-06-30 12:47:40.000000000 +0900
+++ linux-2.6-hirofumi/include/linux/msdos_fs.h	2008-07-01 11:37:03.000000000 +0900
@@ -2,11 +2,11 @@
 #define _LINUX_MSDOS_FS_H
 
 #include <linux/magic.h>
+#include <asm/byteorder.h>
 
 /*
  * The MS-DOS filesystem constants/structures
  */
-#include <asm/byteorder.h>
 
 #define SECTOR_SIZE	512		/* sector size (bytes) */
 #define SECTOR_BITS	9		/* log2(SECTOR_SIZE) */
@@ -89,24 +89,22 @@
 #define IS_FSINFO(x)	(le32_to_cpu((x)->signature1) == FAT_FSINFO_SIG1 \
 			 && le32_to_cpu((x)->signature2) == FAT_FSINFO_SIG2)
 
+struct __fat_dirent {
+	long		d_ino;
+	__kernel_off_t	d_off;
+	unsigned short	d_reclen;
+	char		d_name[256]; /* We must not include limits.h! */
+};
+
 /*
  * ioctl commands
  */
-#define VFAT_IOCTL_READDIR_BOTH		_IOR('r', 1, struct dirent [2])
-#define VFAT_IOCTL_READDIR_SHORT	_IOR('r', 2, struct dirent [2])
+#define VFAT_IOCTL_READDIR_BOTH		_IOR('r', 1, struct __fat_dirent[2])
+#define VFAT_IOCTL_READDIR_SHORT	_IOR('r', 2, struct __fat_dirent[2])
 /* <linux/videotext.h> has used 0x72 ('r') in collision, so skip a few */
 #define FAT_IOCTL_GET_ATTRIBUTES	_IOR('r', 0x10, __u32)
 #define FAT_IOCTL_SET_ATTRIBUTES	_IOW('r', 0x11, __u32)
 
-/*
- * vfat shortname flags
- */
-#define VFAT_SFN_DISPLAY_LOWER	0x0001 /* convert to lowercase for display */
-#define VFAT_SFN_DISPLAY_WIN95	0x0002 /* emulate win95 rule for display */
-#define VFAT_SFN_DISPLAY_WINNT	0x0004 /* emulate winnt rule for display */
-#define VFAT_SFN_CREATE_WIN95	0x0100 /* emulate win95 rule for create */
-#define VFAT_SFN_CREATE_WINNT	0x0200 /* emulate winnt rule for create */
-
 struct fat_boot_sector {
 	__u8	ignored[3];	/* Boot strap short or near jump */
 	__u8	system_id[8];	/* Name - can be used to special case
@@ -168,14 +166,6 @@ struct msdos_dir_slot {
 	__u8    name11_12[4];	/* last 2 characters in name */
 };
 
-struct fat_slot_info {
-	loff_t i_pos;		/* on-disk position of directory entry */
-	loff_t slot_off;	/* offset for slot or de start */
-	int nr_slots;		/* number of slots + 1(de) in filename */
-	struct msdos_dir_entry *de;
-	struct buffer_head *bh;
-};
-
 #ifdef __KERNEL__
 
 #include <linux/buffer_head.h>
@@ -184,6 +174,15 @@ struct fat_slot_info {
 #include <linux/fs.h>
 #include <linux/mutex.h>
 
+/*
+ * vfat shortname flags
+ */
+#define VFAT_SFN_DISPLAY_LOWER	0x0001 /* convert to lowercase for display */
+#define VFAT_SFN_DISPLAY_WIN95	0x0002 /* emulate win95 rule for display */
+#define VFAT_SFN_DISPLAY_WINNT	0x0004 /* emulate winnt rule for display */
+#define VFAT_SFN_CREATE_WIN95	0x0100 /* emulate win95 rule for create */
+#define VFAT_SFN_CREATE_WINNT	0x0200 /* emulate winnt rule for create */
+
 struct fat_mount_options {
 	uid_t fs_uid;
 	gid_t fs_gid;
@@ -267,6 +266,14 @@ struct msdos_inode_info {
 	struct inode vfs_inode;
 };
 
+struct fat_slot_info {
+	loff_t i_pos;		/* on-disk position of directory entry */
+	loff_t slot_off;	/* offset for slot or de start */
+	int nr_slots;		/* number of slots + 1(de) in filename */
+	struct msdos_dir_entry *de;
+	struct buffer_head *bh;
+};
+
 static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
 {
 	return sb->s_fs_info;
_

[PATCH 7/7] Fix the case of jiffies wrapping in mm/pdflush.c

[OGAWA Hirofumi]

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 mm/pdflush.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN mm/pdflush.c~pdfluh-jiffies-check-fix mm/pdflush.c
--- linux-2.6/mm/pdflush.c~pdfluh-jiffies-check-fix	2008-07-01 10:19:07.000000000 +0900
+++ linux-2.6-hirofumi/mm/pdflush.c	2008-07-01 10:19:07.000000000 +0900
@@ -130,7 +130,7 @@ static int __pdflush(struct pdflush_work
 		 * Thread creation: For how long have there been zero
 		 * available threads?
 		 */
-		if (jiffies - last_empty_jifs > 1 * HZ) {
+		if (time_after(jiffies, last_empty_jifs + 1 * HZ)) {
 			/* unlocked list_empty() test is OK here */
 			if (list_empty(&pdflush_list)) {
 				/* unlocked test is OK here */
@@ -151,7 +151,7 @@ static int __pdflush(struct pdflush_work
 		if (nr_pdflush_threads <= MIN_PDFLUSH_THREADS)
 			continue;
 		pdf = list_entry(pdflush_list.prev, struct pdflush_work, list);
-		if (jiffies - pdf->when_i_went_to_sleep > 1 * HZ) {
+		if (time_after(jiffies, pdf->when_i_went_to_sleep + 1 * HZ)) {
 			/* Limit exit rate */
 			pdf->when_i_went_to_sleep = jiffies;
 			break;					/* exeunt */
_

Re: [PATCH 3/7] fat/dir.c: switch to struct __fat_dirent

[Andrew Morton]

On Tue, 01 Jul 2008 11:57:03 +0900 OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:

> struct __fat_dirent is what was formerly the kernel struct dirent
> (that was different from the userspace struct dirent).
> 
> Converting all fat users to struct __fat_dirent will allow us to get 
> rid of the conflicting struct dirent definition.
> 
> Signed-off-by: Adrian Bunk <bunk@kernel.org>
> Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

I assume this was authored by Adrian.  Please tell me if that is wrong.

The way you sent this implies that you were the author.

The way to communicate authorship with emailed patches is to place the
author's "From:" line right at the top of the changelog.

Thanks.

Re: [PATCH 7/7] Fix the case of jiffies wrapping in mm/pdflush.c

[Andrew Morton]

On Tue, 01 Jul 2008 11:57:04 +0900 OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:

> 
> 
> Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> ---
> 
>  mm/pdflush.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff -puN mm/pdflush.c~pdfluh-jiffies-check-fix mm/pdflush.c
> --- linux-2.6/mm/pdflush.c~pdfluh-jiffies-check-fix	2008-07-01 10:19:07.000000000 +0900
> +++ linux-2.6-hirofumi/mm/pdflush.c	2008-07-01 10:19:07.000000000 +0900
> @@ -130,7 +130,7 @@ static int __pdflush(struct pdflush_work
>  		 * Thread creation: For how long have there been zero
>  		 * available threads?
>  		 */
> -		if (jiffies - last_empty_jifs > 1 * HZ) {
> +		if (time_after(jiffies, last_empty_jifs + 1 * HZ)) {
>  			/* unlocked list_empty() test is OK here */
>  			if (list_empty(&pdflush_list)) {
>  				/* unlocked test is OK here */
> @@ -151,7 +151,7 @@ static int __pdflush(struct pdflush_work
>  		if (nr_pdflush_threads <= MIN_PDFLUSH_THREADS)
>  			continue;
>  		pdf = list_entry(pdflush_list.prev, struct pdflush_work, list);
> -		if (jiffies - pdf->when_i_went_to_sleep > 1 * HZ) {
> +		if (time_after(jiffies, pdf->when_i_went_to_sleep + 1 * HZ)) {
>  			/* Limit exit rate */
>  			pdf->when_i_went_to_sleep = jiffies;
>  			break;					/* exeunt */

I don't think this actually "fixes" anything, does it?  The old code
should be correct at runtime.

I renamed the patch to "pdflush: use time_after() instead of open-coding it".

Re: [PATCH 3/7] fat/dir.c: switch to struct __fat_dirent

[OGAWA Hirofumi]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Tue, 01 Jul 2008 11:57:03 +0900 OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
>
>> struct __fat_dirent is what was formerly the kernel struct dirent
>> (that was different from the userspace struct dirent).
>> 
>> Converting all fat users to struct __fat_dirent will allow us to get 
>> rid of the conflicting struct dirent definition.
>> 
>> Signed-off-by: Adrian Bunk <bunk@kernel.org>
>> Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>
> I assume this was authored by Adrian.  Please tell me if that is wrong.

Yes, Adrian is author.

> The way you sent this implies that you were the author.

Oops.

> The way to communicate authorship with emailed patches is to place the
> author's "From:" line right at the top of the changelog.

I see. Sorry. I'll fix my script, and will add "From: " line next time.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 7/7] Fix the case of jiffies wrapping in mm/pdflush.c

[OGAWA Hirofumi]

Andrew Morton <akpm@linux-foundation.org> writes:

>> @@ -151,7 +151,7 @@ static int __pdflush(struct pdflush_work
>>  		if (nr_pdflush_threads <= MIN_PDFLUSH_THREADS)
>>  			continue;
>>  		pdf = list_entry(pdflush_list.prev, struct pdflush_work, list);
>> -		if (jiffies - pdf->when_i_went_to_sleep > 1 * HZ) {
>> +		if (time_after(jiffies, pdf->when_i_went_to_sleep + 1 * HZ)) {
>>  			/* Limit exit rate */
>>  			pdf->when_i_went_to_sleep = jiffies;
>>  			break;					/* exeunt */
>
> I don't think this actually "fixes" anything, does it?  The old code
> should be correct at runtime.

Um.. Yes. It seems I tested something wrong.

> I renamed the patch to "pdflush: use time_after() instead of open-coding it".

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 2/7] fat: Fix VFAT_IOCTL_READDIR_xxx and cleanup for userland

[Christoph Hellwig]

On Tue, Jul 01, 2008 at 11:57:03AM +0900, OGAWA Hirofumi wrote:
> 
> "struct dirent" is a kernel type here, but is a **different type** in 
> userspace!  This means both the structure and the IOCTL number is wrong!
> 
> So, this adds new "struct __fat_dirent" to generate correct IOCTL
> number.  And kernel stuff moves to under __KERNEL__.

Given that the current version can't actually work without defininig
it's own dirent and thus ioctl number symbolic name I wonder if these
ioctls are used at all?  They must have been completely untested for
a while, and I suspect we'd be better off just removing them.

Re: [PATCH 2/7] fat: Fix VFAT_IOCTL_READDIR_xxx and cleanup for userland

[OGAWA Hirofumi]

Christoph Hellwig <hch@infradead.org> writes:

> On Tue, Jul 01, 2008 at 11:57:03AM +0900, OGAWA Hirofumi wrote:
>> 
>> "struct dirent" is a kernel type here, but is a **different type** in 
>> userspace!  This means both the structure and the IOCTL number is wrong!
>> 
>> So, this adds new "struct __fat_dirent" to generate correct IOCTL
>> number.  And kernel stuff moves to under __KERNEL__.
>
> Given that the current version can't actually work without defininig
> it's own dirent and thus ioctl number symbolic name I wonder if these
> ioctls are used at all?  They must have been completely untested for
> a while, and I suspect we'd be better off just removing them.

I'm not sure whether all users doesn't use. (If user uses correct dirent
like I did to test in past, it generates correct number.)

Anyway, why is it better off?  I think, if users which copied, this
patch shouldn't have any impact, and other users can use fixed version?
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 2/7] fat: Fix VFAT_IOCTL_READDIR_xxx and cleanup for userland

[Adrian Bunk]

On Tue, Jul 01, 2008 at 03:40:33AM -0400, Christoph Hellwig wrote:
> On Tue, Jul 01, 2008 at 11:57:03AM +0900, OGAWA Hirofumi wrote:
> > 
> > "struct dirent" is a kernel type here, but is a **different type** in 
> > userspace!  This means both the structure and the IOCTL number is wrong!
> > 
> > So, this adds new "struct __fat_dirent" to generate correct IOCTL
> > number.  And kernel stuff moves to under __KERNEL__.
> 
> Given that the current version can't actually work without defininig
> it's own dirent and thus ioctl number symbolic name I wonder if these
> ioctls are used at all?  They must have been completely untested for
> a while, and I suspect we'd be better off just removing them.

I had the same thought, but when checking this I noticed that at least 
wine works around this problem:

<--  snip  -->

/* just in case... */
#undef VFAT_IOCTL_READDIR_BOTH
#undef USE_GETDENTS

#ifdef linux

/* We want the real kernel dirent structure, not the libc one */
typedef struct
{
    long d_ino;
    long d_off;
    unsigned short d_reclen;
    char d_name[256];
} KERNEL_DIRENT;

/* Define the VFAT ioctl to get both short and long file names */
#define VFAT_IOCTL_READDIR_BOTH  _IOR('r', 1, KERNEL_DIRENT [2] )

<--  snip  -->


cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH 2/7] fat: Fix VFAT_IOCTL_READDIR_xxx and cleanup for userland

[Christoph Hellwig]

On Tue, Jul 01, 2008 at 02:22:48PM +0300, Adrian Bunk wrote:
> > Given that the current version can't actually work without defininig
> > it's own dirent and thus ioctl number symbolic name I wonder if these
> > ioctls are used at all?  They must have been completely untested for
> > a while, and I suspect we'd be better off just removing them.
> 
> I had the same thought, but when checking this I noticed that at least 
> wine works around this problem:

Ok, I'll withdraw my objection in that case.

Re: [PATCH 2/7] fat: Fix VFAT_IOCTL_READDIR_xxx and cleanup for userland

[Christoph Hellwig]

>  
>  #include <linux/magic.h>
> +#include <asm/byteorder.h>

Why do we need this in the user-visible part of the header?

> +struct __fat_dirent {
> +	long		d_ino;
> +	__kernel_off_t	d_off;
> +	unsigned short	d_reclen;
> +	char		d_name[256]; /* We must not include limits.h! */
> +};

Any reason this is not called fat_dirent? 

Re: [PATCH 4/7] fat: cleanup fs/fat/dir.c

[Christoph Hellwig]

On Tue, Jul 01, 2008 at 11:57:03AM +0900, OGAWA Hirofumi wrote:
> 
> This is no logic changes, just cleans fs/fat/dir.c up.

I don't think it makes sense to mark the new functions inline, they are
quite big, and if it really makes sense to inline them the compiler will
do it for us.

Re: [PATCH 2/7] fat: Fix VFAT_IOCTL_READDIR_xxx and cleanup for userland

[OGAWA Hirofumi]

Christoph Hellwig <hch@infradead.org> writes:

>>  #include <linux/magic.h>
>> +#include <asm/byteorder.h>
>
> Why do we need this in the user-visible part of the header?

It was just moved from line below of this.

>> +struct __fat_dirent {
>> +	long		d_ino;
>> +	__kernel_off_t	d_off;
>> +	unsigned short	d_reclen;
>> +	char		d_name[256]; /* We must not include limits.h! */
>> +};
>
> Any reason this is not called fat_dirent? 

It was suggested, and I thought it's safe.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 4/7] fat: cleanup fs/fat/dir.c

[OGAWA Hirofumi]

Christoph Hellwig <hch@infradead.org> writes:

> On Tue, Jul 01, 2008 at 11:57:03AM +0900, OGAWA Hirofumi wrote:
>> 
>> This is no logic changes, just cleans fs/fat/dir.c up.
>
> I don't think it makes sense to mark the new functions inline, they are
> quite big, and if it really makes sense to inline them the compiler will
> do it for us.

I know people hate inline, me too recently. But, I also know old gcc is
not smart. Actually the optimize by hand was much faster (although it's
not kernel).

I've tested this without inline. gcc-3.4.6 doesn't inlined
fat_name_match(), but gcc-4.3.1 inlined it.

What do you think? If you still think it shouldn't, I'll remove it.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>