[Cluster-devel] [GFS2] fsck.gfs2: addendum to fix broken i_goal values in inodes

[Abhi Das <adas@redhat.com>]

This patch moves some code around and fixes some corner cases that
the previous patches did not address.
This patch also fixes some trailing whitespace and removes a test
that is no longer valid from test/fsck.at

Resolves: rhbz#1149516
Signed-off-by: Abhi Das <adas@redhat.com>
---
 gfs2/fsck/metawalk.c   | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
 gfs2/fsck/metawalk.h   |  2 ++
 gfs2/fsck/pass1.c      | 54 ++++----------------------------------
 gfs2/libgfs2/libgfs2.h |  1 +
 tests/fsck.at          |  1 -
 5 files changed, 78 insertions(+), 50 deletions(-)

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index 217bb07..5f432d6 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -1549,6 +1549,9 @@ int check_metatree(struct gfs2_inode *ip, struct metawalk_fxns *pass)
 	uint64_t error_blk = 0;
 	int hit_error_blk = 0;
 
+	if (!height && pass->check_i_goal)
+		pass->check_i_goal(ip, ip->i_di.di_num.no_addr,
+				   pass->private);
 	if (!height && !is_dir(&ip->i_di, ip->i_sbd->gfs1))
 		return 0;
 
@@ -1945,6 +1948,72 @@ static int alloc_leaf(struct gfs2_inode *ip, uint64_t block, void *private)
 	return 0;
 }
 
+/**
+ * rgrp_contains_block - Check if the rgrp provided contains the
+ * given block. Taken directly from the gfs2 kernel code
+ * @rgd: The rgrp to search within
+ * @block: The block to search for
+ *
+ * Returns: 1 if present, 0 if not.
+ */
+static inline int rgrp_contains_block(struct rgrp_tree *rgd, uint64_t block)
+{
+	uint64_t first = rgd->ri.ri_data0;
+	uint64_t last = first + rgd->ri.ri_data;
+	return first <= block && block < last;
+}
+
+/**
+ * check_i_goal
+ * @ip
+ * @goal_blk: What the goal block should be for this inode
+ *
+ * The goal block for a regular file is typically the last
+ * data block of the file. If we can't get the right value,
+ * the inode metadata block is the next best thing.
+ *
+ * Returns: 0 if corrected, 1 if not corrected
+ */
+int check_i_goal(struct gfs2_inode *ip, uint64_t goal_blk,
+			void *private)
+{
+	struct gfs2_sbd *sdp = ip->i_sbd;
+	uint64_t i_block = ip->i_di.di_num.no_addr;
+
+	/* Don't fix gfs1 inodes, system inodes or inodes whose goal blocks are
+	 * set to the inode blocks themselves. */
+	if (sdp->gfs1 || ip->i_di.di_flags & GFS2_DIF_SYSTEM ||
+		ip->i_di.di_goal_meta == i_block)
+		return 0;
+	/* We default to the inode block */
+	if (!goal_blk)
+		goal_blk = i_block;
+
+	if (ip->i_di.di_goal_meta != goal_blk) {
+		/* If the existing goal block is in the same rgrp as the inode,
+		 * we give the benefit of doubt and assume the value is correct */
+		if (ip->i_rgd &&
+		    rgrp_contains_block(ip->i_rgd, ip->i_di.di_goal_meta))
+			goto skip;
+		log_err( _("Error: inode %llu (0x%llx) has invalid "
+			   "allocation goal block %llu (0x%llx). Should"
+			   " be %llu (0x%llx)\n"),
+			 (unsigned long long)i_block, (unsigned long long)i_block,
+			 (unsigned long long)ip->i_di.di_goal_meta,
+			 (unsigned long long)ip->i_di.di_goal_meta,
+			 (unsigned long long)goal_blk, (unsigned long long)goal_blk);
+		if (query( _("Fix the invalid goal block? (y/n) "))) {
+			ip->i_di.di_goal_meta = ip->i_di.di_goal_data = goal_blk;
+			bmodified(ip->i_bh);
+		} else {
+			log_err(_("Invalid goal block not fixed.\n"));
+			return 1;
+		}
+	}
+skip:
+	return 0;
+}
+
 struct metawalk_fxns alloc_fxns = {
 	.private = NULL,
 	.check_leaf = alloc_leaf,
@@ -1955,6 +2024,7 @@ struct metawalk_fxns alloc_fxns = {
 	.check_dentry = NULL,
 	.check_eattr_entry = NULL,
 	.check_eattr_extentry = NULL,
+	.check_i_goal = check_i_goal,
 	.finish_eattr_indir = NULL,
 };
 
diff --git a/gfs2/fsck/metawalk.h b/gfs2/fsck/metawalk.h
index 0d9de3f..aae9121 100644
--- a/gfs2/fsck/metawalk.h
+++ b/gfs2/fsck/metawalk.h
@@ -51,6 +51,8 @@ extern int _fsck_blockmap_set(struct gfs2_inode *ip, uint64_t bblock,
 extern int check_n_fix_bitmap(struct gfs2_sbd *sdp, uint64_t blk,
 			      int error_on_dinode,
 			      enum gfs2_mark_block new_blockmap_state);
+extern int check_i_goal(struct gfs2_inode *ip, uint64_t goal_blk,
+			void *private);
 extern void reprocess_inode(struct gfs2_inode *ip, const char *desc);
 extern struct duptree *dupfind(uint64_t block);
 extern struct gfs2_inode *fsck_system_inode(struct gfs2_sbd *sdp,
diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index 73b054c..2841b8c 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -62,7 +62,6 @@ static int check_extended_leaf_eattr(struct gfs2_inode *ip, uint64_t *data_ptr,
 				     struct gfs2_ea_header *ea_hdr,
 				     struct gfs2_ea_header *ea_hdr_prev,
 				     void *private);
-static int check_i_goal(struct gfs2_inode *ip, uint64_t goal_blk, void *private);
 static int finish_eattr_indir(struct gfs2_inode *ip, int leaf_pointers,
 			      int leaf_pointer_errors, void *private);
 static int invalidate_metadata(struct gfs2_inode *ip, uint64_t block,
@@ -100,7 +99,7 @@ struct metawalk_fxns pass1_fxns = {
 	.check_dentry = NULL,
 	.check_eattr_entry = check_eattr_entries,
 	.check_eattr_extentry = check_extended_leaf_eattr,
-	.check_i_goal = NULL,
+	.check_i_goal = check_i_goal,
 	.finish_eattr_indir = finish_eattr_indir,
 	.big_file_msg = big_file_comfort,
 	.repair_leaf = pass1_repair_leaf,
@@ -799,48 +798,6 @@ static int check_extended_leaf_eattr(struct gfs2_inode *ip, uint64_t *data_ptr,
 	return error;
 }
 
-/**
- * check_i_goal
- * @ip
- * @goal_blk: What the goal block should be for this inode
- *
- * The goal block for a regular file is typically the last
- * data block of the file. If we can't get the right value,
- * the inode metadata block is the next best thing.
- *
- * Returns: 0 if corrected, 1 if not corrected
- */
-static int check_i_goal(struct gfs2_inode *ip, uint64_t goal_blk,
-			void *private)
-{
-	struct gfs2_sbd *sdp = ip->i_sbd;
-
-	if (fsck_system_inode(sdp, ip->i_di.di_num.no_addr))
-		return 0;
-	if (!goal_blk)
-		goal_blk = ip->i_di.di_num.no_addr;
-	if (ip->i_di.di_goal_meta != goal_blk ||
-	    ip->i_di.di_goal_data != goal_blk) {
-		log_err( _("Error: inode %llu (0x%llx) has invalid "
-			   "allocation goal block %llu (0x%llx). Should"
-			   " be %llu (0x%llx)\n"),
-			 (unsigned long long)ip->i_di.di_num.no_addr,
-			 (unsigned long long)ip->i_di.di_num.no_addr,
-			 (unsigned long long)ip->i_di.di_goal_meta,
-			 (unsigned long long)ip->i_di.di_goal_meta,
-			 (unsigned long long)goal_blk,
-			 (unsigned long long)goal_blk);
-		if (query( _("Fix the invalid goal block? (y/n) "))) {
-			ip->i_di.di_goal_meta = ip->i_di.di_goal_data = goal_blk;
-			bmodified(ip->i_bh);
-		} else {
-			log_err(_("Invalid goal block not fixed.\n"));
-			return 1;
-		}
-	}
-	return 0;
-}
-
 static int check_eattr_leaf(struct gfs2_inode *ip, uint64_t block,
 			    uint64_t parent, struct gfs2_buffer_head **bh,
 			    void *private)
@@ -1227,7 +1184,8 @@ bad_dinode:
  * handle_di - This is now a wrapper function that takes a gfs2_buffer_head
  *             and calls handle_ip, which takes an in-code dinode structure.
  */
-static int handle_di(struct gfs2_sbd *sdp, struct gfs2_buffer_head *bh)
+static int handle_di(struct gfs2_sbd *sdp, struct gfs2_buffer_head *bh,
+		     struct rgrp_tree *rgd)
 {
 	int error = 0;
 	uint64_t block = bh->b_blocknr;
@@ -1269,6 +1227,7 @@ static int handle_di(struct gfs2_sbd *sdp, struct gfs2_buffer_head *bh)
 				 (unsigned long long)block,
 				 (unsigned long long)block);
 	}
+	ip->i_rgd = rgd;
 	error = handle_ip(sdp, ip);
 	fsck_inode_put(&ip);
 	return error;
@@ -1595,7 +1554,7 @@ static int pass1_process_bitmap(struct gfs2_sbd *sdp, struct rgrp_tree *rgd, uin
 				 (unsigned long long)block,
 				 (unsigned long long)block);
 			check_n_fix_bitmap(sdp, block, 0, gfs2_block_free);
-		} else if (handle_di(sdp, bh) < 0) {
+		} else if (handle_di(sdp, bh, rgd) < 0) {
 			stack;
 			brelse(bh);
 			gfs2_special_free(&gfs1_rindex_blks);
@@ -1678,9 +1637,6 @@ int pass1(struct gfs2_sbd *sdp)
 	/* Make sure the system inodes are okay & represented in the bitmap. */
 	check_system_inodes(sdp);
 
-	/* Turn the check_i_goal function ON for the non-system inodes */
-	pass1_fxns.check_i_goal = check_i_goal;
-
 	/* So, do we do a depth first search starting@the root
 	 * inode, or use the rg bitmaps, or just read every fs block
 	 * to find the inodes?  If we use the depth first search, why
diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
index 5bffaed..2907e8c 100644
--- a/gfs2/libgfs2/libgfs2.h
+++ b/gfs2/libgfs2/libgfs2.h
@@ -233,6 +233,7 @@ struct gfs2_inode {
 	struct gfs2_dinode i_di;
 	struct gfs2_buffer_head *i_bh;
 	struct gfs2_sbd *i_sbd;
+	struct rgrp_tree *i_rgd; /* The rgrp this inode is in */
 };
 
 /* FIXME not sure that i want to keep a record of the inodes or the
diff --git a/tests/fsck.at b/tests/fsck.at
index afe26db..e3b82bd 100644
--- a/tests/fsck.at
+++ b/tests/fsck.at
@@ -11,5 +11,4 @@ AT_CLEANUP
 
 AT_SETUP([Fix invalid goal blocks])
 GFS_LANG_CHECK([mkfs.gfs2 -O -p lock_nolock $GFS_TGT], [set '/' { di_goal_meta: 0 }])
-GFS_LANG_CHECK([mkfs.gfs2 -O -p lock_nolock $GFS_TGT], [set '/' { di_goal_data: 0 }])
 AT_CLEANUP
-- 
1.8.1.4