Skip to contentSkip to conversation

Obtaining no_formal_ino from directory entry

PATCH New S. Wendy Cheng 2 participants 8 messages Jun 26, 2007 – Jun 27, 2007

lore
Share & tools
lore

S. Wendy Cheng

author
#0 lore raw

PATCH 1/3 Obtaining no_formal_ino from directory entry

GFS2 lookup code doesn't ask for inode shared glock. This implies during in-memory inode creation for existing file, GFS2 will not disk-read in the inode contents. This leaves no_formal_ino un-initialized during lookup time. The un-initialized no_formal_ino is subsequently encoded into file handle. Clients will get ESTALE error whenever it tries to access these files.

-- Wendy

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02_gfs2_specsfs.patch
Type: text/x-patch
Size: 6104 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20070625/99c4bdb0/attachment.bin>

author
#1 lore raw

PATCH 2/3 Obtaining no_formal_ino from directory entry

Sorry, correct subject line from [GFS2 PATCH 1/3] to [GFS2 PATCH 2/3] ...

Quoted 189 lines

GFS2 lookup code doesn't ask for inode shared glock. This implies during in-memory inode creation for existing file, GFS2 will not disk-read in the inode contents. This leaves no_formal_ino un-initialized during lookup time. The un-initialized no_formal_ino is subsequently encoded into file handle. Clients will get ESTALE error whenever it tries to access these files.

-- Wendy

------------------------------------------------------------------------

Signed-off-by S. Wendy Cheng <[email protected]>
dir.c        |    6 ++++--
inode.c      |   13 ++++++++-----
inode.h      |    3 ++-
ops_export.c |    2 +-
ops_fstype.c |   19 ++++++++++++-------
rgrp.c       |   10 +++++-----
6 files changed, 32 insertions(+), 21 deletions(-)
fs/gfs2/inode.h +2 -1
@@ -47,7 +47,8 @@ static inline void gfs2_inum_out(const s
 
 
 void gfs2_inode_attr_in(struct gfs2_inode *ip);
-struct inode *gfs2_inode_lookup(struct super_block *sb, u64 no_addr, unsigned type);
+struct inode *gfs2_inode_lookup(struct super_block *sb, 
+				struct gfs2_inum_host *ino, unsigned type);
 struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr);
 
 int gfs2_inode_refresh(struct gfs2_inode *ip);
fs/gfs2/dir.c +4 -2
@@ -1493,13 +1493,15 @@ struct inode *gfs2_dir_search(struct ino
 	struct buffer_head *bh;
 	struct gfs2_dirent *dent;
 	struct inode *inode;
+	struct gfs2_inum_host ino;
 
 	dent = gfs2_dirent_search(dir, name, gfs2_dirent_find, &bh);
 	if (dent) {
 		if (IS_ERR(dent))
 			return ERR_PTR(PTR_ERR(dent));
-		inode = gfs2_inode_lookup(dir->i_sb,
-					  be64_to_cpu(dent->de_inum.no_addr),
+		ino.no_formal_ino = be64_to_cpu(dent->de_inum.no_formal_ino);
+		ino.no_addr = be64_to_cpu(dent->de_inum.no_addr);
+		inode = gfs2_inode_lookup(dir->i_sb, &ino,
 					  be16_to_cpu(dent->de_type));
 		brelse(bh);
 		return inode;
fs/gfs2/inode.c +8 -5
@@ -86,9 +86,11 @@ static struct inode *gfs2_iget(struct su
 * Returns: A VFS inode, or an error
 */
 
-struct inode *gfs2_inode_lookup(struct super_block *sb, u64 no_addr, unsigned int type)
+struct inode *gfs2_inode_lookup(struct super_block *sb, 
+				struct gfs2_inum_host *ino,
+				unsigned int type)
 {
-	struct inode *inode = gfs2_iget(sb, no_addr);
+	struct inode *inode = gfs2_iget(sb, ino->no_addr);
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_glock *io_gl;
 	int error;

@@ -100,13 +102,14 @@ struct inode *gfs2_inode_lookup(struct s
 		struct gfs2_sbd *sdp = GFS2_SB(inode);
 		umode_t mode;
 		inode->i_private = ip;
+		ip->i_no_formal_ino = ino->no_formal_ino;
 
-		error = gfs2_glock_get(sdp, no_addr, &gfs2_inode_glops, CREATE, &ip->i_gl);
+		error = gfs2_glock_get(sdp, ino->no_addr, &gfs2_inode_glops, CREATE, &ip->i_gl);
 		if (unlikely(error))
 			goto fail;
 		ip->i_gl->gl_object = ip;
 
-		error = gfs2_glock_get(sdp, no_addr, &gfs2_iopen_glops, CREATE, &io_gl);
+		error = gfs2_glock_get(sdp, ino->no_addr, &gfs2_iopen_glops, CREATE, &io_gl);
 		if (unlikely(error))
 			goto fail_put;
 

@@ -915,7 +918,7 @@ struct inode *gfs2_createi(struct gfs2_h
 	if (error)
 		goto fail_gunlock2;
 
-	inode = gfs2_inode_lookup(dir->i_sb, inum.no_addr, IF2DT(mode));
+	inode = gfs2_inode_lookup(dir->i_sb, &inum, IF2DT(mode));
 	if (IS_ERR(inode))
 		goto fail_gunlock2;
fs/gfs2/ops_export.c +1 -1
@@ -245,7 +245,7 @@ static struct dentry *gfs2_get_dentry(st
 	gfs2_glock_dq_uninit(&rgd_gh);
 	gfs2_glock_dq_uninit(&ri_gh);
 
-	inode = gfs2_inode_lookup(sb, inum->no_addr, fh_obj->imode);
+	inode = gfs2_inode_lookup(sb, inum, fh_obj->imode);
 	if (!inode)
 		goto fail;
 	if (IS_ERR(inode)) {
fs/gfs2/ops_fstype.c +12 -7
@@ -234,16 +234,16 @@ fail:
 }
 
 static inline struct inode *gfs2_lookup_root(struct super_block *sb,
-					     u64 no_addr)
+					     struct gfs2_inum_host *ino)
 {
-	return gfs2_inode_lookup(sb, no_addr, DT_DIR);
+	return gfs2_inode_lookup(sb, ino, DT_DIR);
 }
 
 static int init_sb(struct gfs2_sbd *sdp, int silent, int undo)
 {
 	struct super_block *sb = sdp->sd_vfs;
 	struct gfs2_holder sb_gh;
-	u64 no_addr;
+	struct gfs2_inum_host ino;
 	struct inode *inode;
 	int error = 0;
 

@@ -286,10 +286,12 @@ static int init_sb(struct gfs2_sbd *sdp,
 	sb_set_blocksize(sb, sdp->sd_sb.sb_bsize);
 
 	/* Get the root inode */
-	no_addr = sdp->sd_sb.sb_root_dir.no_addr;
+	ino.no_formal_ino = 0;
 	if (sb->s_type == &gfs2meta_fs_type)
-		no_addr = sdp->sd_sb.sb_master_dir.no_addr;
-	inode = gfs2_lookup_root(sb, no_addr);
+		ino.no_addr = sdp->sd_sb.sb_master_dir.no_addr;
+	else
+		ino.no_addr = sdp->sd_sb.sb_root_dir.no_addr;
+	inode = gfs2_lookup_root(sb, &ino);
 	if (IS_ERR(inode)) {
 		error = PTR_ERR(inode);
 		fs_err(sdp, "can't read in root inode: %d\n", error);

@@ -442,11 +444,14 @@ static int init_inodes(struct gfs2_sbd *
 	int error = 0;
 	struct gfs2_inode *ip;
 	struct inode *inode;
+	struct gfs2_inum_host ino;
 
 	if (undo)
 		goto fail_qinode;
 
-	inode = gfs2_lookup_root(sdp->sd_vfs, sdp->sd_sb.sb_master_dir.no_addr);
+	ino.no_formal_ino = 0;
+	ino.no_addr = sdp->sd_sb.sb_master_dir.no_addr;
+	inode = gfs2_lookup_root(sdp->sd_vfs, &ino);
 	if (IS_ERR(inode)) {
 		error = PTR_ERR(inode);
 		fs_err(sdp, "can't read in master directory: %d\n", error);
fs/gfs2/rgrp.c +5 -5
@@ -861,18 +861,18 @@ static struct inode *try_rgrp_unlink(str
 {
 	struct inode *inode;
 	u32 goal = 0;
-	u64 ino;
+	struct gfs2_inum_host ino = {.no_formal_ino = 0};
 
 	for(;;) {
 		goal = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED,
 				    GFS2_BLKST_UNLINKED);
 		if (goal == 0)
 			return 0;
-		ino = goal + rgd->rd_data0;
-		if (ino <= *last_unlinked)
+		ino.no_addr = goal + rgd->rd_data0;
+		if (ino.no_addr <= *last_unlinked)
 			continue;
-		*last_unlinked = ino;
-		inode = gfs2_inode_lookup(rgd->rd_sbd->sd_vfs, ino, DT_UNKNOWN);
+		*last_unlinked = ino.no_addr;
+		inode = gfs2_inode_lookup(rgd->rd_sbd->sd_vfs, &ino, DT_UNKNOWN);
 		if (!IS_ERR(inode))
 			return inode;
 	}
#2 lore raw

PATCH 1/3 Obtaining no_formal_ino from directory entry

In reply to S. Wendy Cheng

Hi,

Quoted 9 lines · S. Wendy Cheng

GFS2 lookup code doesn't ask for inode shared glock. This implies during in-memory inode creation for existing file, GFS2 will not disk-read in the inode contents. This leaves no_formal_ino un-initialized during lookup time. The un-initialized no_formal_ino is subsequently encoded into file handle. Clients will get ESTALE error whenever it tries to access these files.

-- Wendy

Generally this looks good. Please don't add back the _host structure though - just add an extra no_formal_ino argument to the lookup function which is a u64,

Steve.

author
#3 lore raw

PATCH 2/3 Obtaining no_formal_ino from directory entry

Quoted 12 lines · Steven Whitehouse

Hi,

Quoted 7 lines · S. Wendy Cheng

GFS2 lookup code doesn't ask for inode shared glock. This implies during in-memory inode creation for existing file, GFS2 will not disk-read in the inode contents. This leaves no_formal_ino un-initialized during lookup time. The un-initialized no_formal_ino is subsequently encoded into file handle. Clients will get ESTALE error whenever it tries to access these files.

Quoted 10 lines
Quoted 4 lines

-- Wendy

Generally this looks good. Please don't add back the _host structure though - just add an extra no_formal_ino argument to the lookup function which is a u64,

The gfs2_inode_lookup() parameter set would be too long. How about this ... we will never use DT_WHT (bit 14) anyway so let's define:

struct inode *gfs2_inode_lookup(struct super_block *sb,
unsigned type, // if invoked from
gfs2_get_dentry by NFSD

                                                          // type would 
be DT_WHT.
                                u64 no_addr,
                                u64 no_formal_ino);

Then we would save one parameter in patch #3 that will use DT_WHT as nfsbypass param ? Patch enclosed.

-- Wendy

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02_gfs2_specsfs.patch
Type: text/x-patch
Size: 4356 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20070627/ec89c0f3/attachment.bin>

author
#4 lore raw

PATCH 2/3 Obtaining no_formal_ino from directory entry

Grr... there is a typo ... If we all agree with DT_WHT type, will re-submit the patch early tomorrow morning.. maybe we should hop on #sistina irc around 9:30 am EST to speed up the discussion ? Need to get this done and tested out (on RHEL 5.1) by noon tomorrow..

Kicked off the new kernel build now. G'night !

-- Wendy

Quoted 172 lines
Quoted 12 lines · Steven Whitehouse

Hi,

Quoted 7 lines · S. Wendy Cheng

GFS2 lookup code doesn't ask for inode shared glock. This implies during in-memory inode creation for existing file, GFS2 will not disk-read in the inode contents. This leaves no_formal_ino un-initialized during lookup time. The un-initialized no_formal_ino is subsequently encoded into file handle. Clients will get ESTALE error whenever it tries to access these files.

Quoted 10 lines
Quoted 3 lines

-- Wendy

Generally this looks good. Please don't add back the _host structure though - just add an extra no_formal_ino argument to the lookup function which is a u64,

The gfs2_inode_lookup() parameter set would be too long. How about this ... we will never use DT_WHT (bit 14) anyway so let's define:

struct inode *gfs2_inode_lookup(struct super_block *sb,
unsigned type, // if invoked from
gfs2_get_dentry by NFSD

                                                         // type would 
be DT_WHT.
                               u64 no_addr,
                               u64 no_formal_ino);

Then we would save one parameter in patch #3 that will use DT_WHT as nfsbypass param ? Patch enclosed.

-- Wendy

------------------------------------------------------------------------

Signed-off-by S. Wendy Cheng <[email protected]>
dir.c        |    7 ++++---
inode.c      |   12 +++++++++---
inode.h      |    3 ++-
ops_export.c |    4 +++-
ops_fstype.c |    2 +-
rgrp.c       |   11 ++++++-----
6 files changed, 25 insertions(+), 14 deletions(-)
fs/gfs2/inode.h +2 -1
@@ -47,7 +47,8 @@ static inline void gfs2_inum_out(const s
 
 
 void gfs2_inode_attr_in(struct gfs2_inode *ip);
-struct inode *gfs2_inode_lookup(struct super_block *sb, u64 no_addr, unsigned type);
+struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned type, 
+				u64 no_addr, u64 no_formal_ino);
 struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr);
 
 int gfs2_inode_refresh(struct gfs2_inode *ip);
fs/gfs2/inode.c +9 -3
@@ -86,9 +86,12 @@ static struct inode *gfs2_iget(struct su
 * Returns: A VFS inode, or an error
 */
 
-struct inode *gfs2_inode_lookup(struct super_block *sb, u64 no_addr, unsigned int type)
+struct inode *gfs2_inode_lookup(struct super_block *sb, 
+				unsigned int type,
+				u64 no_addr,
+				u64 no_formal_ino)
 {
-	struct inode *inode = gfs2_iget(sb, no_addr);
+	struct inode *inode = gfs2_iget(sb, ino->no_addr);
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_glock *io_gl;
 	int error;

@@ -100,6 +103,7 @@ struct inode *gfs2_inode_lookup(struct s
 		struct gfs2_sbd *sdp = GFS2_SB(inode);
 		umode_t mode;
 		inode->i_private = ip;
+		ip->i_no_formal_ino = no_formal_ino;
 
 		error = gfs2_glock_get(sdp, no_addr, &gfs2_inode_glops, CREATE, &ip->i_gl);
 		if (unlikely(error))

@@ -915,7 +919,9 @@ struct inode *gfs2_createi(struct gfs2_h
 	if (error)
 		goto fail_gunlock2;
 
-	inode = gfs2_inode_lookup(dir->i_sb, inum.no_addr, IF2DT(mode));
+	inode = gfs2_inode_lookup(dir->i_sb, IF2DT(mode),
+					inum.no_addr,
+					inum.no_formal_ino);
 	if (IS_ERR(inode))
 		goto fail_gunlock2;
fs/gfs2/dir.c +4 -3
@@ -1498,9 +1498,10 @@ struct inode *gfs2_dir_search(struct ino
 	if (dent) {
 		if (IS_ERR(dent))
 			return ERR_PTR(PTR_ERR(dent));
-		inode = gfs2_inode_lookup(dir->i_sb,
-					  be64_to_cpu(dent->de_inum.no_addr),
-					  be16_to_cpu(dent->de_type));
+		inode = gfs2_inode_lookup(dir->i_sb, 
+				be16_to_cpu(dent->de_type),
+				be64_to_cpu(dent->de_inum.no_addr),
+				be64_to_cpu(dent->de_inum.no_formal_ino);
 		brelse(bh);
 		return inode;
 	}
fs/gfs2/ops_export.c +3 -1
@@ -245,7 +245,9 @@ static struct dentry *gfs2_get_dentry(st
 	gfs2_glock_dq_uninit(&rgd_gh);
 	gfs2_glock_dq_uninit(&ri_gh);
 
-	inode = gfs2_inode_lookup(sb, inum->no_addr, fh_obj->imode);
+	inode = gfs2_inode_lookup(sb, fh_obj->imode,
+					inum->no_addr,
+					inum->no_formal_ino);
 	if (!inode)
 		goto fail;
 	if (IS_ERR(inode)) {
fs/gfs2/ops_fstype.c +1 -1
@@ -236,7 +236,7 @@ fail:
 static inline struct inode *gfs2_lookup_root(struct super_block *sb,
 					     u64 no_addr)
 {
-	return gfs2_inode_lookup(sb, no_addr, DT_DIR);
+	return gfs2_inode_lookup(sb, DT_DIR, no_addr, 0);
 }
 
 static int init_sb(struct gfs2_sbd *sdp, int silent, int undo)
fs/gfs2/rgrp.c +6 -5
@@ -861,18 +861,19 @@ static struct inode *try_rgrp_unlink(str
 {
 	struct inode *inode;
 	u32 goal = 0;
-	u64 ino;
+	u64 no_addr;
 
 	for(;;) {
 		goal = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED,
 				    GFS2_BLKST_UNLINKED);
 		if (goal == 0)
 			return 0;
-		ino = goal + rgd->rd_data0;
-		if (ino <= *last_unlinked)
+		no_addr = goal + rgd->rd_data0;
+		if (no_addr <= *last_unlinked)
 			continue;
-		*last_unlinked = ino;
-		inode = gfs2_inode_lookup(rgd->rd_sbd->sd_vfs, ino, DT_UNKNOWN);
+		*last_unlinked = ino.no_addr;
+		inode = gfs2_inode_lookup(rgd->rd_sbd->sd_vfs, DT_UNKNOWN,
+					no_addr, 0);
 		if (!IS_ERR(inode))
 			return inode;
 	}
#5 lore raw

PATCH 2/3 Obtaining no_formal_ino from directory entry

In reply to S. Wendy Cheng

Hi,

Quoted 31 lines · S. Wendy Cheng
Quoted 12 lines · Steven Whitehouse

Hi,

Quoted 7 lines · S. Wendy Cheng

GFS2 lookup code doesn't ask for inode shared glock. This implies during in-memory inode creation for existing file, GFS2 will not disk-read in the inode contents. This leaves no_formal_ino un-initialized during lookup time. The un-initialized no_formal_ino is subsequently encoded into file handle. Clients will get ESTALE error whenever it tries to access these files.

Quoted 10 lines
Quoted 4 lines

-- Wendy

Generally this looks good. Please don't add back the _host structure though - just add an extra no_formal_ino argument to the lookup function which is a u64,

The gfs2_inode_lookup() parameter set would be too long. How about this ... we will never use DT_WHT (bit 14) anyway so let's define:

struct inode *gfs2_inode_lookup(struct super_block *sb,
unsigned type, // if invoked from
gfs2_get_dentry by NFSD

Quoted 8 lines
                                                          // type would 
be DT_WHT.
                                u64 no_addr,
                                u64 no_formal_ino);

Then we would save one parameter in patch #3 that will use DT_WHT as nfsbypass param ? Patch enclosed.

Ugh! I'd rather not misuse the DT_ types. We can just use DT_UNKNOWN I think as we can tell the difference between NFS and unlinked recovery simply by looking to see if no_formal_ino is non-zero or not (since zero can never be a valid no_formal_ino). Do you think thats ok?

Steve.

Quoted 4 lines

-- Wendy

#7 lore raw

PATCH 2/3 Obtaining no_formal_ino from directory entry

Hi,

Quoted 4 lines · Wendy Cheng

Revised patch .... Wendy

Both patches applied. Please check your patches to ensure you don't add trailing whitespace in future. Thanks,

Steve.