[Cluster-devel] [libgfs2 PATCH] libgfs2: Make rg repair do better sanity checks on rindex

[Andrew Price]

Hi Bob,

On 13/01/17 13:40, Bob Peterson wrote:
> Hi,
>
> This patch changes the checks done on the rindex file. Before, if
> the rindex addresses weren't perfectly aligned and equally spaced,
> it considered the rindex not sane. However, in most cases it should
> be sane. Rather than flag this as a violation, I use it to trigger
> a read from disk to see if the rindex really DOES point to an rgrp
> in these cases, and if not, THEN flag the rindex as not sane.
> I also added some basic checks on the rindex values, such as
> checking if the number of bitmaps is sane, and if most of the fields
> have values we'd expect.
>
> Signed-off-by: Bob Peterson <rpeterso at redhat.com>

Thanks for working on this. Unfortunately it makes one of the nukerg 
tests fail:

<snip>
Nuking rindex entry 1.
./fsck.at:35: fsck.gfs2 -y $GFS_TGT
stderr:
(level 1 failed: Some damage was found; we need to take remedial measures)
Block #-1 (0xffffffffffffffff) (-17 of 0) is not GFS2_METATYPE_RB.
Attempting to repair the rgrp.
bad read: Invalid argument from rewrite_rg_block:892: block 
18446744073709551615 (0xffffffffffffffff) count: 1 size: 4096 ret: -1
<snip>

You can reproduce this with: make check TOPTS='-k fsck' and the log will 
be in tests/testsuite.dir/21/testsuite.log

Or, manually, the test is:

$ truncate -s 10G testvol
$ gfs2/mkfs/mkfs.gfs2 -Oplock_nolock testvol
$ tests/nukerg -i 1 testvol
$ gfs2/fsck/fsck.gfs2 -y testvol
$ gfs2/fsck/fsck.gfs2 -n testvol

Andy

> ---
> diff --git a/gfs2/libgfs2/super.c b/gfs2/libgfs2/super.c
> index 81834b3..2983f35 100644
> --- a/gfs2/libgfs2/super.c
> +++ b/gfs2/libgfs2/super.c
> @@ -127,6 +127,59 @@ int read_sb(struct gfs2_sbd *sdp)
>  	return 0;
>  }
>
> +/* rgd_seems_sane - check some general things about the rindex entry
> + *
> + * If rg lengths are not consistent, it's not sane (or it's converted from
> + * gfs1). The first RG will be a different length due to space reserved for
> + * the superblock, so we can't detect this until we check rgrp 3, when we
> + * can compare the distance between rgrp 1 and rgrp 2.
> + *
> + * Returns: 1 if the rgd seems relatively sane
> + */
> +static int rgd_seems_sane(struct gfs2_sbd *sdp, struct rgrp_tree *rgd)
> +{
> +	uint32_t most_bitmaps_possible;
> +
> +	/* rg length must be at least 1 */
> +	if (rgd->ri.ri_length == 0)
> +		return 0;
> +
> +	/* A max rgrp, 2GB, divided into blocksize, divided by blocks/byte
> +	   represented in the bitmap, NBBY. Rough approximation only, due to
> +	   metadata headers. I'm doing the math this way to avoid overflow. */
> +	most_bitmaps_possible = (GFS2_MAX_RGSIZE * 1024 * 256) / sdp->bsize;
> +	if (rgd->ri.ri_length > most_bitmaps_possible)
> +		return 0;
> +
> +	if (rgd->ri.ri_data0 != rgd->ri.ri_addr + rgd->ri.ri_length)
> +		return 0;
> +
> +	if (rgd->ri.ri_bitbytes != rgd->ri.ri_data / GFS2_NBBY)
> +		return 0;
> +
> +	return 1;
> +}
> +
> +/* good_on_disk - check if the rindex points to what looks like an rgrp on disk
> + *
> + * This is only called when the rindex pointers aren't spaced evenly, which
> + * isn't often. The rindex is pointing to an unexpected location, so we
> + * check if the block it is pointing to is really an rgrp. If so, we count the
> + * rindex entry as "sane" (after all, it did pass the previous checks above.)
> + * If not, we count it as not sane, and therefore, the whole rindex is not to
> + * be trusted by fsck.gfs2.
> + */
> +static int good_on_disk(struct gfs2_sbd *sdp, struct rgrp_tree *rgd)
> +{
> +	struct gfs2_buffer_head *bh;
> +	int is_rgrp;
> +
> +	bh = bread(sdp, rgd->ri.ri_addr);
> +	is_rgrp = (gfs2_check_meta(bh, GFS2_METATYPE_RG) == 0);
> +	brelse(bh);
> +	return is_rgrp;
> +}
> +
>  /**
>   * rindex_read - read in the rg index file
>   * @sdp: the incore superblock pointer
> @@ -182,17 +235,12 @@ int rindex_read(struct gfs2_sbd *sdp, int fd, uint64_t *count1, int *sane)
>  			if (!sdp->gfs1) {
>  				if (prev_rgd->start >= rgd->start)
>  					*sane = 0;
> -				/* If rg lengths are not consistent, it's not
> -				   sane (or it's converted from gfs1).  The
> -				   first RG will be a different length due to
> -				   space allocated for the superblock, so we
> -				   can't detect this until we check rgrp 3,
> -				   when we can compare the distance between
> -				   rgrp 1 and rgrp 2. */
> -				if (rg > 2 && prev_length &&
> +				else if (!rgd_seems_sane(sdp, rgd))
> +					*sane = 0;
> +				else if (rg > 2 && prev_length &&
>  				    prev_length != rgd->start -
>  				    prev_rgd->start)
> -					*sane = 0;
> +					*sane = good_on_disk(sdp, rgd);
>  			}
>  			prev_length = rgd->start - prev_rgd->start;
>  			prev_rgd->length = rgrp_size(prev_rgd);
>