On Thu 17-07-14 09:17:25, Dave Chinner wrote:
> On Wed, Jul 16, 2014 at 01:11:05AM -0700, Christoph Hellwig wrote:
> > On Wed, Jul 16, 2014 at 10:38:51AM +1000, Dave Chinner wrote:
> > > I added this:
> > >
> > > /*
> > > * The addition of spare space and the non-logged CRC format
> > > * fields to the AGF mean that the size that is logged is almost
> > > * always going to be smaller than the structure itself. Hence
> > > * we need to make sure that the buffer contains all the data we
> > > * want to print rather than just check against the structure
> > > * size.
> > > */
> > >
> > > Cheers,
> >
> > I'd prefer to mention v4 filesystems as well:
> >
> > /*
> > * v4 filesystems only contain the fields before the uuid, and
> > * even v5 filesystems don't usually log any field beneath it.
> > */
>
> It actually has nothing to do with v4 or v5 filesystems - it's to do
> with the fact that we do partial buffer logging but logprint is
> assuming the structure is always logging as a whole. There's
> mistakes like this all through logprint - we whack them like this
> with a big stick (i.e. refuse to parse the structure) when they are
> found.
>
> Did you notice the way that log_print_all.c handled these issues?
> For the AGI, it simply looks at the length of the region and sizes
> the output accordingly. And for the AGF, it just ignores the size of
> the region and assumes that it captured everything that is to be
> printed.
>
> IOWs, we've played whack-a-mole on this again while ignoring the
> fundamental issues ithat still remain:
>
> - that logprint has a lot of assumptions that simply aren't
> true; and
> - that logprint simply does not handle split region
> continuations like the kernel recovery code does.
>
> Both of these things lead to having to handle these strange "out of
> space" cases in multiple places, and simply not handling them in
> places that actually need to.
>
> These are just more reasons why logprint - as it says itself in a
> couple of comments - needs a complete rewrite.
Yeah, I've noticed that logprint definitely doesn't handle all possible
cases and the code is ... well, organic :). OTOH it worked for me (except
for this bug) when I needed so for a debug tool like that it seemed good
enough. I don't feel like rewriting it from scratch (at least in near
future) but if someone can find time for that, it would be surely welcome.
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
|