[Cluster-devel] [PATCH 0/5] fsck.gfs2: rindex checking improvements

[Cluster-devel] [PATCH 0/5] fsck.gfs2: rindex checking improvements

[Andrew Price]

These patches improve the rindex checking code in the fetch_rgrps() path and enable 3 more fsck.gfs2 test cases that now pass. There is more work to do in this area but these patches are worthwhile improvements on their own.

Andy

Andrew Price (5):
  fsck.gfs2: Refactor fetch_rgrps()
  fsck.gfs2: Call rindex_read() outside of ri_update()
  fsck.gfs2: Rename ri_update to read_rgrps
  fsck.gfs2: Fix bounds check in find_next_rgrp_dist()
  fsck.gfs2: Improve rindex consistency check

 gfs2/fsck/fsck.h       |  2 +-
 gfs2/fsck/initialize.c | 79 +++++++++++++++++++++++-------------------
 gfs2/fsck/rgrepair.c   |  5 ++-
 tests/fsck.at          | 28 ++++++++++++---
 4 files changed, 71 insertions(+), 43 deletions(-)

-- 
2.30.2

[Cluster-devel] [PATCH 1/5] fsck.gfs2: Refactor fetch_rgrps()

[Andrew Price]

Split out the code that fetches the rgrps at a trust level into its own
function and restructure it with gotos as subsequent changes would add
too much nesting otherwise.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/initialize.c | 59 ++++++++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 25 deletions(-)

diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
index 5f176dca..0cafeb95 100644
--- a/gfs2/fsck/initialize.c
+++ b/gfs2/fsck/initialize.c
@@ -730,13 +730,9 @@ static int ri_update(struct gfs2_sbd *sdp, int *rgcount, int *ok)
 	return -1;
 }
 
-/**
- * fetch_rgrps - fetch the resource groups from disk, and check their integrity
- */
-static int fetch_rgrps(struct gfs2_sbd *sdp)
+static int fetch_rgrps_level(struct gfs2_sbd *sdp, enum rgindex_trust_level lvl, int *count, int *ok)
 {
-	enum rgindex_trust_level trust_lvl;
-	int rgcount, ok = 1;
+	int ret = 1;
 
 	const char *level_desc[] = {
 		_("Checking if all rgrp and rindex values are good"),
@@ -752,30 +748,43 @@ static int fetch_rgrps(struct gfs2_sbd *sdp)
 		_("Too many rgrp misses: rgrps must be unevenly spaced"),
 		_("Too much damage found: we cannot rebuild this rindex"),
 	};
-	/*******************************************************************
-	 ********  Validate and read in resource group information  ********
-	 *******************************************************************/
+
+	log_notice(_("Level %d resource group check: %s.\n"), lvl + 1, level_desc[lvl]);
+
+	if (rg_repair(sdp, lvl, count, ok) != 0)
+		goto fail;
+
+	ret = ri_update(sdp, count, ok);
+	if (ret != 0)
+		goto fail;
+
+	log_notice(_("(level %d passed)\n"), lvl + 1);
+	return 0;
+fail:
+	if (ret == -1)
+		log_err(_("(level %d failed: %s)\n"), lvl + 1, fail_desc[lvl]);
+	else
+		log_err(_("(level %d failed at block %d (0x%x): %s)\n"), lvl + 1,
+		        ret, ret, fail_desc[lvl]);
+	return ret;
+}
+
+/**
+ * fetch_rgrps - fetch the resource groups from disk, and check their integrity
+ */
+static int fetch_rgrps(struct gfs2_sbd *sdp)
+{
+	enum rgindex_trust_level trust_lvl;
+	int rgcount;
+	int ok = 1;
+
 	log_notice(_("Validating resource group index.\n"));
 	for (trust_lvl = blind_faith; trust_lvl <= indignation; trust_lvl++) {
 		int ret = 0;
 
-		log_notice(_("Level %d resource group check: %s.\n"), trust_lvl + 1,
-			  level_desc[trust_lvl]);
-		if ((rg_repair(sdp, trust_lvl, &rgcount, &ok) == 0) &&
-		    ((ret = ri_update(sdp, &rgcount, &ok)) == 0)) {
-			log_notice(_("(level %d passed)\n"), trust_lvl + 1);
+		ret = fetch_rgrps_level(sdp, trust_lvl, &rgcount, &ok);
+		if (ret == 0)
 			break;
-		} else {
-			if (ret == -1)
-				log_err( _("(level %d failed: %s)\n"),
-					 trust_lvl + 1, fail_desc[trust_lvl]);
-			else
-				log_err( _("(level %d failed@block %lld "
-					   "(0x%llx): %s)\n"), trust_lvl + 1,
-					 (unsigned long long)ret,
-					 (unsigned long long)ret,
-					 fail_desc[trust_lvl]);
-		}
 		if (fsck_abort)
 			break;
 	}
-- 
2.30.2

[Cluster-devel] [PATCH 2/5] fsck.gfs2: Call rindex_read() outside of ri_update()

[Andrew Price]

This will allow us to make better decisions about which checks failed
down the line.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/fsck.h       |  2 +-
 gfs2/fsck/initialize.c | 26 +++++++++++++-------------
 gfs2/fsck/rgrepair.c   |  3 +--
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/gfs2/fsck/fsck.h b/gfs2/fsck/fsck.h
index a5159d4b..d23326ec 100644
--- a/gfs2/fsck/fsck.h
+++ b/gfs2/fsck/fsck.h
@@ -130,7 +130,7 @@ extern int pass2(struct gfs2_sbd *sdp);
 extern int pass3(struct gfs2_sbd *sdp);
 extern int pass4(struct gfs2_sbd *sdp);
 extern int pass5(struct gfs2_sbd *sdp, struct gfs2_bmap *bl);
-extern int rg_repair(struct gfs2_sbd *sdp, int trust_lvl, int *rg_count, int *ok);
+extern int rg_repair(struct gfs2_sbd *sdp, int trust_lvl, int *ok);
 extern int fsck_query(const char *format, ...)
 	__attribute__((format(printf,1,2)));
 extern struct dir_info *dirtree_find(uint64_t block);
diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
index 0cafeb95..5be561f8 100644
--- a/gfs2/fsck/initialize.c
+++ b/gfs2/fsck/initialize.c
@@ -677,18 +677,18 @@ static unsigned gfs2_rgrp_reada(struct gfs2_sbd *sdp, unsigned cur_window,
 /**
  * ri_update - attach rgrps to the super block
  * @sdp: incore superblock data
- * @rgcount: returned count of rgs
+ * @expected: number of resource groups expected (rindex entries)
  *
  * Given the rgrp index inode, link in all rgrps into the super block
  * and be sure that they can be read.
  *
  * Returns: 0 on success, -1 on failure.
  */
-static int ri_update(struct gfs2_sbd *sdp, int *rgcount, int *ok)
+static int ri_update(struct gfs2_sbd *sdp, uint64_t expected)
 {
 	struct rgrp_tree *rgd;
 	struct gfs2_rindex *ri;
-	uint64_t count1 = 0, count2 = 0;
+	uint64_t count = 0;
 	uint64_t errblock = 0;
 	uint64_t rmax = 0;
 	struct osi_node *n, *next = NULL;
@@ -697,8 +697,6 @@ static int ri_update(struct gfs2_sbd *sdp, int *rgcount, int *ok)
 	/* Turn off generic readhead */
 	posix_fadvise(sdp->device_fd, 0, 0, POSIX_FADV_RANDOM);
 
-	if (rindex_read(sdp, &count1, ok))
-		goto fail;
 	for (n = osi_first(&sdp->rgtree); n; n = next) {
 		next = osi_next(n);
 		rgd = (struct rgrp_tree *)n;
@@ -710,15 +708,14 @@ static int ri_update(struct gfs2_sbd *sdp, int *rgcount, int *ok)
 		if (errblock)
 			return errblock;
 		ra_window--;
-		count2++;
+		count++;
 		ri = &rgd->ri;
 		if (ri->ri_data0 + ri->ri_data - 1 > rmax)
 			rmax = ri->ri_data0 + ri->ri_data - 1;
 	}
 
 	sdp->fssize = rmax;
-	*rgcount = count1;
-	if (count1 != count2)
+	if (count != expected)
 		goto fail;
 
 	posix_fadvise(sdp->device_fd, 0, 0, POSIX_FADV_NORMAL);
@@ -730,7 +727,7 @@ static int ri_update(struct gfs2_sbd *sdp, int *rgcount, int *ok)
 	return -1;
 }
 
-static int fetch_rgrps_level(struct gfs2_sbd *sdp, enum rgindex_trust_level lvl, int *count, int *ok)
+static int fetch_rgrps_level(struct gfs2_sbd *sdp, enum rgindex_trust_level lvl, uint64_t *count, int *ok)
 {
 	int ret = 1;
 
@@ -751,10 +748,13 @@ static int fetch_rgrps_level(struct gfs2_sbd *sdp, enum rgindex_trust_level lvl,
 
 	log_notice(_("Level %d resource group check: %s.\n"), lvl + 1, level_desc[lvl]);
 
-	if (rg_repair(sdp, lvl, count, ok) != 0)
+	if (rg_repair(sdp, lvl, ok) != 0)
+		goto fail;
+
+	if (rindex_read(sdp, count, ok) != 0)
 		goto fail;
 
-	ret = ri_update(sdp, count, ok);
+	ret = ri_update(sdp, *count);
 	if (ret != 0)
 		goto fail;
 
@@ -775,7 +775,7 @@ fail:
 static int fetch_rgrps(struct gfs2_sbd *sdp)
 {
 	enum rgindex_trust_level trust_lvl;
-	int rgcount;
+	uint64_t rgcount;
 	int ok = 1;
 
 	log_notice(_("Validating resource group index.\n"));
@@ -793,7 +793,7 @@ static int fetch_rgrps(struct gfs2_sbd *sdp)
 			   "this file system.\n"));
 		return -1;
 	}
-	log_info( _("%u resource groups found.\n"), rgcount);
+	log_info( _("%"PRIu64" resource groups found.\n"), rgcount);
 
 	check_rgrps_integrity(sdp);
 	return 0;
diff --git a/gfs2/fsck/rgrepair.c b/gfs2/fsck/rgrepair.c
index 7f0854ca..2bddd91f 100644
--- a/gfs2/fsck/rgrepair.c
+++ b/gfs2/fsck/rgrepair.c
@@ -1015,7 +1015,7 @@ static int expect_rindex_sanity(struct gfs2_sbd *sdp, int *num_rgs)
  *             was converted from GFS via gfs2_convert, and its rgrps are
  *             not on nice boundaries thanks to previous gfs_grow ops. Lovely.
  */
-int rg_repair(struct gfs2_sbd *sdp, int trust_lvl, int *rg_count, int *ok)
+int rg_repair(struct gfs2_sbd *sdp, int trust_lvl, int *ok)
 {
 	struct osi_node *n, *next = NULL, *e, *enext;
 	int error, discrepancies, percent;
@@ -1283,7 +1283,6 @@ int rg_repair(struct gfs2_sbd *sdp, int trust_lvl, int *rg_count, int *ok)
 			i++;
 		} while (i < rgd->ri.ri_length);
 	}
-	*rg_count = rg;
 	gfs2_rgrp_free(sdp, &sdp->rgcalc);
 	gfs2_rgrp_free(sdp, &sdp->rgtree);
 	/* We shouldn't need to worry about getting the user's permission to
-- 
2.30.2

[Cluster-devel] [PATCH 3/5] fsck.gfs2: Rename ri_update to read_rgrps

[Andrew Price]

Give it a name that matches what it does.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/initialize.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
index 5be561f8..4c266d9a 100644
--- a/gfs2/fsck/initialize.c
+++ b/gfs2/fsck/initialize.c
@@ -675,7 +675,7 @@ static unsigned gfs2_rgrp_reada(struct gfs2_sbd *sdp, unsigned cur_window,
 }
 
 /**
- * ri_update - attach rgrps to the super block
+ * read_rgrps - attach rgrps to the super block
  * @sdp: incore superblock data
  * @expected: number of resource groups expected (rindex entries)
  *
@@ -684,7 +684,7 @@ static unsigned gfs2_rgrp_reada(struct gfs2_sbd *sdp, unsigned cur_window,
  *
  * Returns: 0 on success, -1 on failure.
  */
-static int ri_update(struct gfs2_sbd *sdp, uint64_t expected)
+static int read_rgrps(struct gfs2_sbd *sdp, uint64_t expected)
 {
 	struct rgrp_tree *rgd;
 	struct gfs2_rindex *ri;
@@ -754,7 +754,7 @@ static int fetch_rgrps_level(struct gfs2_sbd *sdp, enum rgindex_trust_level lvl,
 	if (rindex_read(sdp, count, ok) != 0)
 		goto fail;
 
-	ret = ri_update(sdp, *count);
+	ret = read_rgrps(sdp, *count);
 	if (ret != 0)
 		goto fail;
 
-- 
2.30.2

[Cluster-devel] [PATCH 4/5] fsck.gfs2: Fix bounds check in find_next_rgrp_dist()

[Andrew Price]

We're reading block address (next_block + b) so the bounds check should
make sure that address is valid.

Add some tests that now pass (and some commented-out ones that don't).

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/rgrepair.c |  2 +-
 tests/fsck.at        | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/gfs2/fsck/rgrepair.c b/gfs2/fsck/rgrepair.c
index 2bddd91f..637e5271 100644
--- a/gfs2/fsck/rgrepair.c
+++ b/gfs2/fsck/rgrepair.c
@@ -392,7 +392,7 @@ static uint64_t find_next_rgrp_dist(struct gfs2_sbd *sdp, uint64_t blk,
 		next_block = prevrgd->ri.ri_addr + rgrp_dist;
 		/* Now we account for block rounding done by mkfs.gfs2 */
 		for (b = 0; b <= length + GFS2_NBBY; b++) {
-			if (next_block >= sdp->device.length)
+			if (next_block + b >= sdp->device.length)
 				break;
 			bh = bread(sdp, next_block + b);
 			gfs2_meta_header_in(&mh, bh->b_data);
diff --git a/tests/fsck.at b/tests/fsck.at
index 727108f4..3b1c5af3 100644
--- a/tests/fsck.at
+++ b/tests/fsck.at
@@ -46,6 +46,26 @@ AT_KEYWORDS(fsck.gfs2 fsck)
 GFS_NUKERG_CHECK([mkfs.gfs2 -O -p lock_nolock $GFS_TGT], [-i 1])
 AT_CLEANUP
 
+AT_SETUP([2G RGs: Fix bad resource group #0])
+AT_KEYWORDS(fsck.gfs2 fsck)
+GFS_NUKERG_CHECK([mkfs.gfs2 -O -p lock_nolock -r 2048 $GFS_TGT], [-r 0])
+AT_CLEANUP
+
+AT_SETUP([2G RGs: Fix bad resource group #1])
+AT_KEYWORDS(fsck.gfs2 fsck)
+GFS_NUKERG_CHECK([mkfs.gfs2 -O -p lock_nolock -r 2048 $GFS_TGT], [-r 1])
+AT_CLEANUP
+
+#AT_SETUP([2G RGs: Fix bad rindex entry #0])
+#AT_KEYWORDS(fsck.gfs2 fsck)
+#GFS_NUKERG_CHECK([mkfs.gfs2 -O -p lock_nolock -r 2048 $GFS_TGT], [-i 0])
+#AT_CLEANUP
+
+#AT_SETUP([2G RGs: Fix bad rindex entry #1])
+#AT_KEYWORDS(fsck.gfs2 fsck)
+#GFS_NUKERG_CHECK([mkfs.gfs2 -O -p lock_nolock -r 2048 $GFS_TGT], [-i 1])
+#AT_CLEANUP
+
 AT_SETUP([Rebuild bad journal])
 AT_KEYWORDS(fsck.gfs2 fsck)
 GFS_TGT_REGEN
-- 
2.30.2

[Cluster-devel] [PATCH 5/5] fsck.gfs2: Improve rindex consistency check

[Andrew Price]

Before we were ignoring the flag set by rindex_read to tell whether
there are bad entries and the rindex was (sometimes) fixed by subsequent
checks failing. Check that flag and enable an rindex repair test that
previously failed.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/initialize.c | 2 +-
 tests/fsck.at          | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
index 4c266d9a..58dd23d3 100644
--- a/gfs2/fsck/initialize.c
+++ b/gfs2/fsck/initialize.c
@@ -751,7 +751,7 @@ static int fetch_rgrps_level(struct gfs2_sbd *sdp, enum rgindex_trust_level lvl,
 	if (rg_repair(sdp, lvl, ok) != 0)
 		goto fail;
 
-	if (rindex_read(sdp, count, ok) != 0)
+	if (rindex_read(sdp, count, ok) != 0 || !*ok)
 		goto fail;
 
 	ret = read_rgrps(sdp, *count);
diff --git a/tests/fsck.at b/tests/fsck.at
index 3b1c5af3..538d41ad 100644
--- a/tests/fsck.at
+++ b/tests/fsck.at
@@ -36,10 +36,10 @@ AT_KEYWORDS(fsck.gfs2 fsck)
 GFS_NUKERG_CHECK([mkfs.gfs2 -O -p lock_nolock $GFS_TGT], [-r 1])
 AT_CLEANUP
 
-#AT_SETUP([Fix bad rindex entry #0])
-#AT_KEYWORDS(fsck.gfs2 fsck)
-#GFS_NUKERG_CHECK([mkfs.gfs2 -O -p lock_nolock $GFS_TGT], [-i 0])
-#AT_CLEANUP
+AT_SETUP([Fix bad rindex entry #0])
+AT_KEYWORDS(fsck.gfs2 fsck)
+GFS_NUKERG_CHECK([mkfs.gfs2 -O -p lock_nolock $GFS_TGT], [-i 0])
+AT_CLEANUP
 
 AT_SETUP([Fix bad rindex entry #1])
 AT_KEYWORDS(fsck.gfs2 fsck)
-- 
2.30.2