[Cluster-devel] [GFS2 PATCH 2/3] gfs2: Fix broken freeze_go_xmote_bh

[Bob Peterson <rpeterso@redhat.com>]

The freeze glock was used in several places whenever a gfs2 file system
was frozen, thawed, mounted, unmounted, remounted, or withdrawn. It was
used to prevent those things from clashing with one another.
Function freeze_go_xmote_bh only checked if the journal was clean in
cases where the journal was LIVE. That's wrong because if the journal is
really live, it's a moving target and almost guaranteed to be dirty.
If the journal was not live, the check for a clean journal was skipped.
That's also wrong because sometimes the journal would be LIVE and
sometimes not: It was LIVE when gfs2_reconfigure locks the freeze lock
for "remount -o remount,ro." It was not LIVE when gfs2_fill_super called
gfs2_freeze_lock before gfs2_make_fs_rw sets the LIVE bit.
The problem was never noticed because gfs2_make_fs_rw had redundant code
for the exact same checks as freeze_go_xmote_bh.

This patch attempts to clean up the mess by removing the redundant code
from gfs2_make_fs_rw and changing the callers of gfs2_freeze_lock to
specify whether they need the journal checked for consistency:
If the journal consistency check is unwanted, they specify GL_SKIP in
the holder. If the check is required, they do not specify GL_SKIP.
Most callers determine if the consistency check is needed based on if
the journal is being transitioned from "not live" to "live": If it is
becoming live, the check is needed, otherwise it is not.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glops.c      | 31 ++++++++++++++++++++-----------
 fs/gfs2/ops_fstype.c |  5 +++--
 fs/gfs2/recovery.c   |  3 ++-
 fs/gfs2/super.c      | 34 ++++++++++++----------------------
 4 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index 6d0770564493..8ae2f33e014e 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -609,18 +609,27 @@ static int freeze_go_xmote_bh(struct gfs2_holder *gh)
 	struct gfs2_log_header_host head;
 	int error;
 
-	if (test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags)) {
-		j_gl->gl_ops->go_inval(j_gl, DIO_METADATA);
+	if (gh->gh_flags & GL_SKIP)
+		return 0;
 
-		error = gfs2_find_jhead(sdp->sd_jdesc, &head, false);
-		if (gfs2_assert_withdraw_delayed(sdp, !error))
-			return error;
-		if (gfs2_assert_withdraw_delayed(sdp, head.lh_flags &
-						 GFS2_LOG_HEAD_UNMOUNT))
-			return -EIO;
-		sdp->sd_log_sequence = head.lh_sequence + 1;
-		gfs2_log_pointers_init(sdp, head.lh_blkno);
-	}
+	/*
+	 * If our journal is truly live, rw, it is guaranteed to be dirty.
+	 * If our journal is ro, and we are soon to make it live, we need to
+	 * check whether it was cleanly unmounted. If not, we withdraw.
+	 */
+	if (gfs2_assert_withdraw_delayed(sdp,
+				 !test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags)))
+		return -EIO;
+	j_gl->gl_ops->go_inval(j_gl, DIO_METADATA);
+
+	error = gfs2_find_jhead(sdp->sd_jdesc, &head, false);
+	if (gfs2_assert_withdraw_delayed(sdp, !error))
+		return error;
+	if (gfs2_assert_withdraw_delayed(sdp, head.lh_flags &
+					 GFS2_LOG_HEAD_UNMOUNT))
+		return -EIO;
+	sdp->sd_log_sequence = head.lh_sequence + 1;
+	gfs2_log_pointers_init(sdp, head.lh_blkno);
 	return 0;
 }
 
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 7f8410d8fdc1..6f4be312bd34 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -1263,7 +1263,7 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc)
 		}
 	}
 
-	error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
+	error = gfs2_freeze_lock(sdp, &freeze_gh, sb_rdonly(sb) ? GL_SKIP : 0);
 	if (error)
 		goto fail_per_node;
 
@@ -1584,7 +1584,8 @@ static int gfs2_reconfigure(struct fs_context *fc)
 	if ((sb->s_flags ^ fc->sb_flags) & SB_RDONLY) {
 		struct gfs2_holder freeze_gh;
 
-		error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
+		error = gfs2_freeze_lock(sdp, &freeze_gh,
+				 fc->sb_flags & SB_RDONLY ? GL_SKIP : 0);
 		if (error)
 			return -EINVAL;
 
diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
index 016ed1b2ca1d..be0037da3bb4 100644
--- a/fs/gfs2/recovery.c
+++ b/fs/gfs2/recovery.c
@@ -472,7 +472,8 @@ void gfs2_recover_func(struct work_struct *work)
 
 		/* Acquire a shared hold on the freeze lock */
 
-		error = gfs2_freeze_lock(sdp, &thaw_gh, LM_FLAG_PRIORITY);
+		error = gfs2_freeze_lock(sdp, &thaw_gh, LM_FLAG_PRIORITY |
+					 GL_SKIP);
 		if (error)
 			goto fail_gunlock_ji;
 
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 6e00d15ef0a8..fe6cea188199 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -128,28 +128,8 @@ int gfs2_jdesc_check(struct gfs2_jdesc *jd)
 
 int gfs2_make_fs_rw(struct gfs2_sbd *sdp)
 {
-	struct gfs2_inode *ip = GFS2_I(sdp->sd_jdesc->jd_inode);
-	struct gfs2_glock *j_gl = ip->i_gl;
-	struct gfs2_log_header_host head;
 	int error;
 
-	j_gl->gl_ops->go_inval(j_gl, DIO_METADATA);
-	if (gfs2_withdrawn(sdp))
-		return -EIO;
-
-	error = gfs2_find_jhead(sdp->sd_jdesc, &head, false);
-	if (error || gfs2_withdrawn(sdp))
-		return error;
-
-	if (!(head.lh_flags & GFS2_LOG_HEAD_UNMOUNT)) {
-		gfs2_consist(sdp);
-		return -EIO;
-	}
-
-	/*  Initialize some head of the log stuff  */
-	sdp->sd_log_sequence = head.lh_sequence + 1;
-	gfs2_log_pointers_init(sdp, head.lh_blkno);
-
 	error = gfs2_quota_init(sdp);
 	if (!error && !gfs2_withdrawn(sdp))
 		set_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
@@ -346,7 +326,8 @@ static int gfs2_lock_fs_check_clean(struct gfs2_sbd *sdp)
 	}
 
 	error = gfs2_glock_nq_init(sdp->sd_freeze_gl, LM_ST_EXCLUSIVE,
-				   LM_FLAG_NOEXP, &sdp->sd_freeze_gh);
+				   LM_FLAG_NOEXP | GL_SKIP,
+				   &sdp->sd_freeze_gh);
 	if (error)
 		goto out;
 
@@ -654,7 +635,16 @@ void gfs2_freeze_func(struct work_struct *work)
 	struct super_block *sb = sdp->sd_vfs;
 
 	atomic_inc(&sb->s_active);
-	error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
+	/*
+	 * Here we take the freeze lock in SH to wait until a freeze operation
+	 * (that began with gfs2_freeze's call to gfs2_lock_fs_check_clean
+	 * which takes the freeze gl in EX) has been thawed. Function
+	 * gfs2_lock_fs_check_clean checks that all the journals are clean,
+	 * so we don't need to do it again with this gfs2_freeze_lock.
+	 * In fact, our journal is live when this glock is granted, so the
+	 * freeze_go_xmote_bh will withdraw unless we specify GL_SKIP.
+	 */
+	error = gfs2_freeze_lock(sdp, &freeze_gh, GL_SKIP);
 	if (error) {
 		gfs2_assert_withdraw(sdp, 0);
 	} else {
-- 
2.31.1