Skip to contentSkip to conversation

fat: remove parent_logstart check in fat_get_parent

PATCH In review Namjae Jeon 2 participants 5 messages Oct 28, 2012 – Nov 6, 2012

Subsystems (2)
VFS VFAT/FAT/msdos filesystem
lore
Share & tools
lore

Namjae Jeon

author
#0 lore raw

PATCH 1/5 fat: remove parent_logstart check in fat_get_parent

From: Namjae Jeon <[email protected]>

The root directory inode is added to the directory hash table in fat_attach when mounted with nfs option. Since root dentry never expires until umount, fat_dget never fails to return the root inode for subdirectories of root inode.i.e., parent_logstart cannot be zero.

Signed-off-by Namjae Jeon <[email protected]>
Signed-off-by Ravishankar N <[email protected]>
Signed-off-by Amit Sahrawat <[email protected]>
---
 fs/fat/nfs.c |   51 ++++++++++++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 29 deletions(-)
fs/fat/nfs.c +22 -29
index a4e6ac7..671a75d 100644
@@ -274,36 +274,29 @@ struct dentry *fat_get_parent(struct dentry *child_dir)
 		parent_inode = fat_dget(sb, parent_logstart);
 		if (parent_inode || sbi->options.nfs != FAT_NFS_NOSTALE_RO)
 			goto out;
-		if (!parent_logstart)
-			/*logstart of dotdot entry is zero if
-			* if the directory's parent is root
-			*/
-			parent_inode = sb->s_root->d_inode;
-		else {
-			blknr = fat_clus_to_blknr(sbi, parent_logstart);
-			parent_bh = sb_bread(sb, blknr);
-			if (!parent_bh) {
-				fat_msg(sb, KERN_ERR,
-					"NFS:unable to read cluster of parent directory");
-				goto out;
-			}
-			de = (struct msdos_dir_entry *) parent_bh->b_data;
-			clus_to_match = fat_get_start(sbi, &de[0]);
-			search_clus = fat_get_start(sbi, &de[1]);
-			if (!search_clus)
-				search_clus = sbi->root_cluster;
-			brelse(parent_bh);
-			do {
-				parent_inode =  fat_traverse_cluster(sb,
-						search_clus, clus_to_match);
-				if (IS_ERR(parent_inode) || parent_inode)
-					break;
-				search_clus = fat_read_next_clus(sb,
-								search_clus);
-				if (search_clus < 0)
-					break;
-			} while (search_clus != FAT_ENT_EOF);
+		blknr = fat_clus_to_blknr(sbi, parent_logstart);
+		parent_bh = sb_bread(sb, blknr);
+		if (!parent_bh) {
+			fat_msg(sb, KERN_ERR,
+				"NFS:unable to read cluster of parent directory");
+			goto out;
 		}
+		de = (struct msdos_dir_entry *) parent_bh->b_data;
+		clus_to_match = fat_get_start(sbi, &de[0]);
+		search_clus = fat_get_start(sbi, &de[1]);
+		if (!search_clus)
+			search_clus = sbi->root_cluster;
+		brelse(parent_bh);
+		do {
+			parent_inode =  fat_traverse_cluster(sb,
+					search_clus, clus_to_match);
+			if (IS_ERR(parent_inode) || parent_inode)
+				break;
+			search_clus = fat_read_next_clus(sb,
+							search_clus);
+			if (search_clus < 0)
+				break;
+		} while (search_clus != FAT_ENT_EOF);
 	}
 out:
 	brelse(dotdot_bh);
Signature · 1 line
1.7.9.5
maintainer
#1 lore raw
Quoted 6 lines · Namjae Jeon

From: Namjae Jeon <[email protected]>

The root directory inode is added to the directory hash table in fat_attach when mounted with nfs option. Since root dentry never expires until umount, fat_dget never fails to return the root inode for subdirectories of root inode.i.e., parent_logstart cannot be zero.

Sorry, I will review at this weekend.

Thanks.

Quoted 70 lines
Signed-off-by Namjae Jeon <[email protected]>
Signed-off-by Ravishankar N <[email protected]>
Signed-off-by Amit Sahrawat <[email protected]>
---
 fs/fat/nfs.c |   51 ++++++++++++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 29 deletions(-)
fs/fat/nfs.c +22 -29
index a4e6ac7..671a75d 100644
@@ -274,36 +274,29 @@ struct dentry *fat_get_parent(struct dentry *child_dir)
 		parent_inode = fat_dget(sb, parent_logstart);
 		if (parent_inode || sbi->options.nfs != FAT_NFS_NOSTALE_RO)
 			goto out;
-		if (!parent_logstart)
-			/*logstart of dotdot entry is zero if
-			* if the directory's parent is root
-			*/
-			parent_inode = sb->s_root->d_inode;
-		else {
-			blknr = fat_clus_to_blknr(sbi, parent_logstart);
-			parent_bh = sb_bread(sb, blknr);
-			if (!parent_bh) {
-				fat_msg(sb, KERN_ERR,
-					"NFS:unable to read cluster of parent directory");
-				goto out;
-			}
-			de = (struct msdos_dir_entry *) parent_bh->b_data;
-			clus_to_match = fat_get_start(sbi, &de[0]);
-			search_clus = fat_get_start(sbi, &de[1]);
-			if (!search_clus)
-				search_clus = sbi->root_cluster;
-			brelse(parent_bh);
-			do {
-				parent_inode =  fat_traverse_cluster(sb,
-						search_clus, clus_to_match);
-				if (IS_ERR(parent_inode) || parent_inode)
-					break;
-				search_clus = fat_read_next_clus(sb,
-								search_clus);
-				if (search_clus < 0)
-					break;
-			} while (search_clus != FAT_ENT_EOF);
+		blknr = fat_clus_to_blknr(sbi, parent_logstart);
+		parent_bh = sb_bread(sb, blknr);
+		if (!parent_bh) {
+			fat_msg(sb, KERN_ERR,
+				"NFS:unable to read cluster of parent directory");
+			goto out;
 		}
+		de = (struct msdos_dir_entry *) parent_bh->b_data;
+		clus_to_match = fat_get_start(sbi, &de[0]);
+		search_clus = fat_get_start(sbi, &de[1]);
+		if (!search_clus)
+			search_clus = sbi->root_cluster;
+		brelse(parent_bh);
+		do {
+			parent_inode =  fat_traverse_cluster(sb,
+					search_clus, clus_to_match);
+			if (IS_ERR(parent_inode) || parent_inode)
+				break;
+			search_clus = fat_read_next_clus(sb,
+							search_clus);
+			if (search_clus < 0)
+				break;
+		} while (search_clus != FAT_ENT_EOF);
 	}
 out:
 	brelse(dotdot_bh);
Signature · 1 line
OGAWA Hirofumi <[email protected]>
author
#2 lore raw

2012/10/31, OGAWA Hirofumi <[email protected]>:

Quoted 10 lines
Quoted 6 lines · Namjae Jeon

From: Namjae Jeon <[email protected]>

The root directory inode is added to the directory hash table in fat_attach when mounted with nfs option. Since root dentry never expires until umount, fat_dget never fails to return the root inode for subdirectories of root inode.i.e., parent_logstart cannot be zero.

Sorry, I will review at this weekend.

No problem, Thank you.

Quoted 78 lines

Thanks.

Quoted 70 lines
Signed-off-by Namjae Jeon <[email protected]>
Signed-off-by Ravishankar N <[email protected]>
Signed-off-by Amit Sahrawat <[email protected]>
---
 fs/fat/nfs.c |   51 ++++++++++++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 29 deletions(-)
fs/fat/nfs.c +22 -29
index a4e6ac7..671a75d 100644
@@ -274,36 +274,29 @@ struct dentry *fat_get_parent(struct dentry
 *child_dir)
 		parent_inode = fat_dget(sb, parent_logstart);
 		if (parent_inode || sbi->options.nfs != FAT_NFS_NOSTALE_RO)
 			goto out;
-		if (!parent_logstart)
-			/*logstart of dotdot entry is zero if
-			* if the directory's parent is root
-			*/
-			parent_inode = sb->s_root->d_inode;
-		else {
-			blknr = fat_clus_to_blknr(sbi, parent_logstart);
-			parent_bh = sb_bread(sb, blknr);
-			if (!parent_bh) {
-				fat_msg(sb, KERN_ERR,
-					"NFS:unable to read cluster of parent directory");
-				goto out;
-			}
-			de = (struct msdos_dir_entry *) parent_bh->b_data;
-			clus_to_match = fat_get_start(sbi, &de[0]);
-			search_clus = fat_get_start(sbi, &de[1]);
-			if (!search_clus)
-				search_clus = sbi->root_cluster;
-			brelse(parent_bh);
-			do {
-				parent_inode =  fat_traverse_cluster(sb,
-						search_clus, clus_to_match);
-				if (IS_ERR(parent_inode) || parent_inode)
-					break;
-				search_clus = fat_read_next_clus(sb,
-								search_clus);
-				if (search_clus < 0)
-					break;
-			} while (search_clus != FAT_ENT_EOF);
+		blknr = fat_clus_to_blknr(sbi, parent_logstart);
+		parent_bh = sb_bread(sb, blknr);
+		if (!parent_bh) {
+			fat_msg(sb, KERN_ERR,
+				"NFS:unable to read cluster of parent directory");
+			goto out;
 		}
+		de = (struct msdos_dir_entry *) parent_bh->b_data;
+		clus_to_match = fat_get_start(sbi, &de[0]);
+		search_clus = fat_get_start(sbi, &de[1]);
+		if (!search_clus)
+			search_clus = sbi->root_cluster;
+		brelse(parent_bh);
+		do {
+			parent_inode =  fat_traverse_cluster(sb,
+					search_clus, clus_to_match);
+			if (IS_ERR(parent_inode) || parent_inode)
+				break;
+			search_clus = fat_read_next_clus(sb,
+							search_clus);
+			if (search_clus < 0)
+				break;
+		} while (search_clus != FAT_ENT_EOF);
 	}
 out:
Quoted 1 line

brelse(dotdot_bh);

--
OGAWA Hirofumi <[email protected]>

maintainer
#3 lore raw
In reply to Namjae Jeon
Quoted 16 lines · Namjae Jeon
fs/fat/nfs.c +15 -0
+		blknr = fat_clus_to_blknr(sbi, parent_logstart);
+		parent_bh = sb_bread(sb, blknr);
+		if (!parent_bh) {
+			fat_msg(sb, KERN_ERR,
+				"NFS:unable to read cluster of parent directory");
+			goto out;
 		}
+		de = (struct msdos_dir_entry *) parent_bh->b_data;
+		clus_to_match = fat_get_start(sbi, &de[0]);
+		search_clus = fat_get_start(sbi, &de[1]);
+		if (!search_clus)
+			search_clus = sbi->root_cluster;
+		brelse(parent_bh);
+		do {
+			parent_inode =  fat_traverse_cluster(sb,
+					search_clus, clus_to_match);

fs/fat/nfs.c struct dentry *fat_get_parent(struct dentry *child_dir) · line 302

Quoted 10 lines
fs/fat/nfs.c +7 -0
+			if (IS_ERR(parent_inode) || parent_inode)
+				break;
+			search_clus = fat_read_next_clus(sb,
+							search_clus);
+			if (search_clus < 0)
+				break;
+		} while (search_clus != FAT_ENT_EOF);
 	}
 out:
 	brelse(dotdot_bh);

I started to understand what is doing this with cleanup. It is really strange and duplicates many codes with bugs.

So, please replace whole FAT nfs patches in -mm to review whole at next patchset. And sorry, but please think whole more carefully.

Thanks.

Signature · 1 line
OGAWA Hirofumi <[email protected]>
author
#4 lore raw

2012/11/5, OGAWA Hirofumi <[email protected]>:

Quoted 32 lines
Quoted 16 lines · Namjae Jeon
fs/fat/nfs.c +15 -0
+		blknr = fat_clus_to_blknr(sbi, parent_logstart);
+		parent_bh = sb_bread(sb, blknr);
+		if (!parent_bh) {
+			fat_msg(sb, KERN_ERR,
+				"NFS:unable to read cluster of parent directory");
+			goto out;
 		}
+		de = (struct msdos_dir_entry *) parent_bh->b_data;
+		clus_to_match = fat_get_start(sbi, &de[0]);
+		search_clus = fat_get_start(sbi, &de[1]);
+		if (!search_clus)
+			search_clus = sbi->root_cluster;
+		brelse(parent_bh);
+		do {
+			parent_inode =  fat_traverse_cluster(sb,
+					search_clus, clus_to_match);

fs/fat/nfs.c struct dentry *fat_get_parent(struct dentry *child_dir) · line 302

Quoted 10 lines
fs/fat/nfs.c +7 -0
+			if (IS_ERR(parent_inode) || parent_inode)
+				break;
+			search_clus = fat_read_next_clus(sb,
+							search_clus);
+			if (search_clus < 0)
+				break;
+		} while (search_clus != FAT_ENT_EOF);
 	}
 out:
 	brelse(dotdot_bh);

I started to understand what is doing this with cleanup. It is really strange and duplicates many codes with bugs.

Quoted 2 lines

So, please replace whole FAT nfs patches in -mm to review whole at next patchset. And sorry, but please think whole more carefully.

Hi OGAWA.
Okay, I will rework all of patches as your comment. Thanks for review.

Quoted 5 lines

Thanks.
--
OGAWA Hirofumi <[email protected]>