xfs
[Top] [All Lists]

Re: Proposed patch for xfsprogs

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: Proposed patch for xfsprogs
From: C Linus Hicks <linush@xxxxxxxxxxx>
Date: Mon, 01 Mar 2010 09:37:59 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100301031642.GG22370@xxxxxxxxxxxxxxxx>
References: <1267311250.28691.40.camel@xxxxxxxxxxxxxxx> <20100301031642.GG22370@xxxxxxxxxxxxxxxx>
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


<Prev in Thread] Current Thread [Next in Thread>