gfs1 convert/fsck fixes

These two patches fix bugs identified during recent convert/fsck tests on gfs1 filesystems.

Abhi Das (2):
libgfs2: patch to update gfs1 superblock correctly
gfs2-utils: check and fix bad dinode pointers in gfs1 sb

 gfs2/convert/gfs2_convert.c | 29 ++++++++++++++++++
 gfs2/fsck/initialize.c      | 72 +++++++++++++++++++++++++++++++++++++++++++++
 gfs2/libgfs2/ondisk.c       |  5 ++++
 3 files changed, 106 insertions(+)

1.8.1.4

gfs2-utils uses the gfs2_sb structure as a template to operate on both gfs1 and gfs2 superblocks. Some fields of the gfs1 superblock are not in gfs2 and are replaced by padded fields. These padded fields were not being written out after modification and were instead being zeroed out, thereby causing corruption. This patch fixes gfs2_sb_out() to write out the padded fields.

Resolves: rhbz#1053668

---
 gfs2/libgfs2/ondisk.c | 5 +++++
 1 file changed, 5 insertions(+)

index dcc537e..3fbc63f 100644
@@ -132,15 +132,20 @@ void gfs2_sb_out(const struct gfs2_sb *sb, char *buf)
 
 	CPOUT_32(sb, str, sb_fs_format);
 	CPOUT_32(sb, str, sb_multihost_format);
+	CPOUT_32(sb, str, __pad0);                        /* gfs sb_flags */
 
 	CPOUT_32(sb, str, sb_bsize);
 	CPOUT_32(sb, str, sb_bsize_shift);
+	CPOUT_32(sb, str, __pad1);                        /* gfs sb_seg_size */
 
 	gfs2_inum_out(&sb->sb_master_dir, (char *)&str->sb_master_dir);
 	gfs2_inum_out(&sb->sb_root_dir, (char *)&str->sb_root_dir);
 
 	CPOUT_08(sb, str, sb_lockproto, GFS2_LOCKNAME_LEN);
 	CPOUT_08(sb, str, sb_locktable, GFS2_LOCKNAME_LEN);
+	gfs2_inum_out(&sb->__pad2, (char *)&str->__pad2); /* gfs rindex */
+	gfs2_inum_out(&sb->__pad3, (char *)&str->__pad3); /* gfs quota */
+	gfs2_inum_out(&sb->__pad4, (char *)&str->__pad4); /* gfs license */
 #ifdef GFS2_HAS_UUID
 	memcpy(str->sb_uuid, sb->sb_uuid, 16);
 #endif

1.8.1.4

Hi Abhi,

I'll defer to Bob's comments on patch #2 but this one looks good, thanks.

Cheers,
Andy

This patch makes gfs2_convert check for bad values of sb_seg_size, sb_quota_di and sb_license_di.

In fsck.gfs2, attempts are made to correct these erroneous values.

Resolves: rhbz#1053668

---
 gfs2/convert/gfs2_convert.c | 29 ++++++++++++++++++
 gfs2/fsck/initialize.c      | 72 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)

index 7a7d4df..9f7fa17 100644
@@ -1502,6 +1502,24 @@ static int read_gfs1_jiindex(struct gfs2_sbd *sdp)
 	return -1;
 }
 
+static int sanity_check(struct gfs2_sbd *sdp)
+{
+	int error = 0;
+	if (!raw_gfs1_ondisk_sb.sb_quota_di.no_addr) {
+		log_crit(_("Error: Superblock Quota inode address is NULL\n"));
+		error = 1;
+	}
+	if (!raw_gfs1_ondisk_sb.sb_license_di.no_addr) {
+		log_crit(_("Error: Superblock Statfs inode address is NULL\n"));
+		error = 1;
+	}
+	if (!raw_gfs1_ondisk_sb.sb_seg_size) {
+		log_crit(_("Error: Superblock segment size is zero\n"));
+		error = 1;
+	}
+	return error;
+}
+
 /* ------------------------------------------------------------------------- */
 /* init - initialization code                                                */
 /* Returns: 0 on success, -1 on failure                                      */

@@ -2152,6 +2170,17 @@ int main(int argc, char **argv)
 	process_parameters(argc, argv, &opts);
 	error = init(&sb2);
 
+	/*
+	 * Check for some common fs errors
+	 */
+	if (!error) {
+		if (sanity_check(&sb2)) {
+			log_crit(_("%s is not a clean gfs filesytem. Please use the"
+				   " fsck.gfs2 utility to correct these errors and"
+				   " try again.\n"), device);
+			exit(0);
+		}
+	}
 	/* ---------------------------------------------- */
 	/* Make them seal their fate.                     */
 	/* ---------------------------------------------- */

1.8.1.4

----- Original Message -----

| This patch makes gfs2_convert check for bad values of sb_seg_size,
| sb_quota_di and sb_license_di.
| 
| In fsck.gfs2, attempts are made to correct these erroneous values.
| 
| Resolves: rhbz#1053668
| Signed-off-by: Abhi Das <[email protected]>
(snip)
| diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
| index 0f33aa6..24e5de2 100644
| --- a/gfs2/fsck/initialize.c
| +++ b/gfs2/fsck/initialize.c
| @@ -727,6 +727,13 @@ static int init_system_inodes(struct gfs2_sbd *sdp)
|  	}
|  
|  	if (sdp->gfs1) {
| +		/* In gfs1, the license_di is always 3 blocks after the jindex_di */
| +		if (sbd1->sb_license_di.no_addr != sbd1->sb_jindex_di.no_addr + 3) {
| +			sbd1->sb_license_di.no_addr = sbd1->sb_license_di.no_formal_ino
| +				= sbd1->sb_jindex_di.no_addr + 3;
| +			log_err(_("Reset statfs inode block address to: %llu\n"),
| +				sbd1->sb_license_di.no_addr);
| +		}

This resolves the problem in memory, but doesn't fix it on disk. We should probably fix it on disk and also use function query() to get their permission. Same for the quota file.

| +static int reset_journal_seg_size(unsigned int jsize, unsigned int nsegs,
| +					     unsigned int bsize)
| +{
| +	unsigned int seg_size = jsize / (nsegs * bsize);
| +	if (!seg_size)
| +		seg_size = 16; /* The default with 128MB journal and 4K bsize */
| +	if (seg_size != sbd1->sb_seg_size) {
| +		sbd1->sb_seg_size = seg_size;
| +		if (!query(_("Computed correct journal segment size to %u."
| +			     " Reset it? (y/n) "), seg_size)) {
| +			log_crit(_("Error: Cannot proceed without a valid journal"
| +				   " segment size value.\n"));
| +			return -1;
| +		}
| +		log_err(_("Resetting journal segment size to %u\n"), sbd1->sb_seg_size);

Is the superblock being rewritten with the new value anywhere? If the value is only used for fsck purposes, then no problem. If there are places in gfs2_convert that may reference it (possibly through libgfs2) then we should fix it on disk as well.

| +static int correct_journal_seg_size(struct gfs2_sbd *sdp)
| +{
| +	int count;
| +	struct gfs_jindex ji_0, ji_1;
| +	char buf[sizeof(struct gfs_jindex)];
| +	unsigned int jsize = GFS2_DEFAULT_JSIZE * 1024 * 1024;
| +
| +	count = gfs2_readi(sdp->md.jiinode, buf, 0, sizeof(struct gfs_jindex));
| +	if (count != sizeof(struct gfs_jindex))
| +		return -1;

We should probably log an error message here.

| + return reset_journal_seg_size(jsize, ji_0.ji_nsegment, sbd1->sb_bsize);

ALso, make sure to test with a clean file system and "-n" to make sure it doesn't try to make changes. I think it's okay, but I always check it (I've been burned too many times in the past!).

Regards,

Bob Peterson
Red Hat File Systems

Hi Bob,

Thanks for the review.

I took a quick look at the code and I only found lgfs2_sb_write() being called in block_mounters()... which happens before init_system_inodes. But I could've sworn the changes propagated to disk. I remember seeing it with gfs2_edit after the fsck.

I'll look into this and also add the permission check before changing the value.

Same thing here. I'll take a look.

Yeah. I'll do that.

Cheers!
--Abhi

This patch makes gfs2_convert check for bad values of sb_seg_size, sb_quota_di and sb_license_di.

In fsck.gfs2, attempts are made to correct these erroneous values. This v2 patch checks with the user before making changes to the superblock in fsck.gfs2 and also adds some error messaging as per Bob's comments.

Resolves: rhbz#1053668

index 7a7d4df..9f7fa17 100644
@@ -1502,6 +1502,24 @@ static int read_gfs1_jiindex(struct gfs2_sbd *sdp)
 	return -1;
 }
 
+static int sanity_check(struct gfs2_sbd *sdp)
+{
+	int error = 0;
+	if (!raw_gfs1_ondisk_sb.sb_quota_di.no_addr) {
+		log_crit(_("Error: Superblock Quota inode address is NULL\n"));
+		error = 1;
+	}
+	if (!raw_gfs1_ondisk_sb.sb_license_di.no_addr) {
+		log_crit(_("Error: Superblock Statfs inode address is NULL\n"));
+		error = 1;
+	}
+	if (!raw_gfs1_ondisk_sb.sb_seg_size) {
+		log_crit(_("Error: Superblock segment size is zero\n"));
+		error = 1;
+	}
+	return error;
+}
+
 /* ------------------------------------------------------------------------- */
 /* init - initialization code                                                */
 /* Returns: 0 on success, -1 on failure                                      */

@@ -2152,6 +2170,17 @@ int main(int argc, char **argv)
 	process_parameters(argc, argv, &opts);
 	error = init(&sb2);
 
+	/*
+	 * Check for some common fs errors
+	 */
+	if (!error) {
+		if (sanity_check(&sb2)) {
+			log_crit(_("%s is not a clean gfs filesytem. Please use the"
+				   " fsck.gfs2 utility to correct these errors and"
+				   " try again.\n"), device);
+			exit(0);
+		}
+	}
 	/* ---------------------------------------------- */
 	/* Make them seal their fate.                     */
 	/* ---------------------------------------------- */