Re: Proposed patch for xfsprogs

[Dave Chinner <david@fromorbit.com>]

On Sat, Feb 27, 2010 at 05:54:10PM -0500, C. Linus Hicks wrote:
> During my recent experience with having to reconstruct parts of an XFS
> filesystem that got corrupted as a result of several bad blocks, I found
> that some of the information displayed using "blockget -v" was pretty
> useless, and I am proposing the following code change to introduce a
> slight summarization.
> 
> Repeating lines of "setting block <foo> to <bar>" and "setting inode to
> <foo> for {,rt}block <bar>" will be summarized down to two lines.

Agreed, that would certainly help reduce the verbosity of the output.
However, I don't think the patch is correct.

> --- a/xfsprogs-3.1.1/db/check.c	2010-01-29 14:46:13.000000000 -0500
> +++ b/xfsprogs-3.1.1/db/check.c	2010-02-27 17:02:14.111418960 -0500
> @@ -1509,6 +1509,7 @@
>  {
>  	xfs_extlen_t	i;
>  	int		mayprint;
> +	int		isfirst = 1;
>  	char		*p;
>  
>  	if (!check_range(agno, agbno, len))  {
> @@ -1520,10 +1521,15 @@
>  	mayprint = verbose | blist_size;
>  	for (i = 0, p = &dbmap[agno][agbno]; i < len; i++, p++) {
>  		*p = (char)type2;
> -		if (mayprint && (verbose || CHECK_BLISTA(agno, agbno + i)))
> +		if (isfirst && mayprint && (verbose || CHECK_BLISTA(agno, agbno + i))) {
>  			dbprintf(_("setting block %u/%u to %s\n"), agno, agbno + i,
>  				typename[type2]);
> +			isfirst = 0;
> +		}
>  	}
> +	if ((len > 1) && mayprint && (verbose || CHECK_BLISTA(agno, agbno + i)))
> +		dbprintf(_("    ... until %u/%u\n"),
> +				agno, agbno + len - 1);
>  }

This doesn't take into account that not all blocks in the extent
range might be bad and the change is actually doing something. i.e.
only is CHECK_BLISTA() evaluating as true is there a printout
occurring (unless you specify verbose mode). Hence if only interior
portions are being changed or there are multiple ranges being
changed, this output won't reflect the errors being detected
accurately.

The other sections of the patch have the same issue.

I think that changing it to something like:

	int	last_print == -1;
.....
		if (mayprint && (verbose || CHECK_BLISTA(agno, agbno + i))) {
			if (i > 0 && last_print == i - 1) {
				last_print = i;
				continue;
			}
			if (last_print != i - 1) {
				dbprintf(_("    ... until %u/%u\n"),
					agno, agbno + last_print);
				last_print == -1;
			}
			if (last_print == -1) {
				dbprintf(_("setting block %u/%u to %s\n"),
					agno, agbno + i, typename[type2]);
				last_print = i;
			}
		}
	}
	if (len > 1 && last_print != -1)
		dbprintf(_("    ... until %u/%u\n"), agno, agbno + last_print);

would do the trick. What do you think?

Cheers,

Dave.



-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs