Subject: Re: Proposed patch for xfsprogs
From: C Linus Hicks <linush@verizon.net>
To: Dave Chinner <david@fromorbit.com>
Cc: xfs@oss.sgi.com
In-reply-to: <20100301031642.GG22370@discord.disaster>
References: <1267311250.28691.40.camel@lh10.linush.net>
	<20100301031642.GG22370@discord.disaster>
Content-type: text/plain
Date: Mon, 01 Mar 2010 09:37:59 -0500
Message-id: <1267454279.28691.45.camel@lh10.linush.net>
MIME-version: 1.0
Content-transfer-encoding: 7bit

On Mon, 2010-03-01 at 14:16 +1100, Dave Chinner wrote:
> 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?

You're quite right, I'm glad you looked close enough to spot that. Sorry
I missed it. There's one case your code doesn't handle correctly, where
len > 0 and last_print == -1 and i == len - 1. Let me rework the code
and do some testing then I will re-submit.

Linus