From mboxrd@z Thu Jan  1 00:00:00 1970
From: Abhijith Das <adas@redhat.com>
Date: Wed, 12 Mar 2014 13:47:02 -0400 (EDT)
Subject: [Cluster-devel] [PATCH 2/2] gfs2-utils: check and fix bad
 dinode	pointers in gfs1 sb
In-Reply-To: <1282492035.13086337.1394627758854.JavaMail.zimbra@redhat.com>
References: <1394597673-27787-1-git-send-email-adas@redhat.com>
	<1394597673-27787-3-git-send-email-adas@redhat.com>
	<1282492035.13086337.1394627758854.JavaMail.zimbra@redhat.com>
Message-ID: <1736769291.13354388.1394646422906.JavaMail.zimbra@redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

Hi Bob,

Thanks for the review.

> ----- 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 <adas@redhat.com>
> (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.

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.

> 
> | +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.

Same thing here. I'll take a look.

> 
> | +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!).
> 

Yeah. I'll do that.

Cheers!
--Abhi
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
>