[Cluster-devel] [GFS2 PATCH v2] GFS2: Use local iopen glock holder in gfs2_evict_inode

[Bob Peterson <rpeterso@redhat.com>]

Hi,

I found a couple problems with the previous version of this patch
during testing. Here is my replacement, version 2.

Patch description:

Before this patch, function gfs2_evict_inode unlocked the iopen
glock (from SH), waited for completion, then locked it again in
EXclusive mode. That's all well and good except that other processes
(not in gfs2_evict_inode) can try to do lookups, and function
gfs2_inode_lookup tries to lock the iopen glock in SH again. This
second lookup can and does wipe out the holder's pid with getpid().
The first putpid (from glock_holder_uninit) will be successful, but
the second one will crash the kernel with:
BUG: unable to handle kernel paging request
This patch introduces a holder variable, io_gh, local to function
gfs2_evict_inode, which will keep its own getpid() and subsequent
putpid() from interfering with one another. So simultaneous inode
lookups won't change the value out from under gfs2_evict_inode.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/super.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 9b2ff353..21a8ba8 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1518,7 +1518,7 @@ static void gfs2_evict_inode(struct inode *inode)
 	struct super_block *sb = inode->i_sb;
 	struct gfs2_sbd *sdp = sb->s_fs_info;
 	struct gfs2_inode *ip = GFS2_I(inode);
-	struct gfs2_holder gh;
+	struct gfs2_holder gh, iopen_gh;
 	struct address_space *metamapping;
 	int error;
 
@@ -1527,6 +1527,7 @@ static void gfs2_evict_inode(struct inode *inode)
 		return;
 	}
 
+	memset(&iopen_gh, 0, sizeof(iopen_gh));
 	if (inode->i_nlink || (sb->s_flags & MS_RDONLY))
 		goto out;
 
@@ -1555,9 +1556,15 @@ static void gfs2_evict_inode(struct inode *inode)
 	    test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
 		ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
 		gfs2_glock_dq_wait(&ip->i_iopen_gh);
-		gfs2_holder_reinit(LM_ST_EXCLUSIVE, LM_FLAG_TRY_1CB | GL_NOCACHE,
-				   &ip->i_iopen_gh);
-		error = gfs2_glock_nq(&ip->i_iopen_gh);
+		/* This is subtle: Now we need to uninit the i_iopen_holder,
+		   but if we do that before we obtain the new reference with
+		   the local holder, the uninit's glock_put will free the
+		   glock, which causes the new ref. to crash. So we need to do
+		   this in a very specific order. Can't use glock_nq_init. */
+		gfs2_holder_init(ip->i_iopen_gh.gh_gl, LM_ST_EXCLUSIVE,
+				 LM_FLAG_TRY_1CB | GL_NOCACHE, &iopen_gh);
+		gfs2_holder_uninit(&ip->i_iopen_gh);
+		error = gfs2_glock_nq(&iopen_gh);
 		if (error)
 			goto out_truncate;
 	}
@@ -1610,12 +1617,12 @@ out_unlock:
 	if (gfs2_rs_active(&ip->i_res))
 		gfs2_rs_deltree(&ip->i_res);
 
-	if (ip->i_iopen_gh.gh_gl) {
-		if (test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
-			ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
-			gfs2_glock_dq_wait(&ip->i_iopen_gh);
+	if (iopen_gh.gh_gl) {
+		if (test_bit(HIF_HOLDER, &iopen_gh.gh_iflags)) {
+			iopen_gh.gh_flags |= GL_NOCACHE;
+			gfs2_glock_dq_wait(&iopen_gh);
 		}
-		gfs2_holder_uninit(&ip->i_iopen_gh);
+		gfs2_holder_uninit(&iopen_gh);
 	}
 	gfs2_glock_dq_uninit(&gh);
 	if (error && error != GLR_TRYFAILED && error != -EROFS)