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

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: Jan Kara <jack@suse.cz>, xfs@oss.sgi.com
Subject: Re: [PATCH] logprint: Fix printing of AGF buffers
Date: Thu, 17 Jul 2014 09:17:25 +1000	[thread overview]
Message-ID: <20140716231725.GF4453@dastard> (raw)
In-Reply-To: <20140716081105.GB29924@infradead.org>

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.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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