Re: [PATCH] logprint: Fix printing of AGF buffers

[Jan Kara <jack@suse.cz>]

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@suse.cz>
SUSE Labs, CR

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