xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] logprint: Fix printing of AGF buffers
From: Jan Kara <jack@xxxxxxx>
Date: Thu, 17 Jul 2014 10:39:05 +0200
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, Jan Kara <jack@xxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140716231725.GF4453@dastard>
References: <1405349100-19734-1-git-send-email-jack@xxxxxxx> <20140715101931.GC30363@xxxxxxxxxxxxx> <20140715140938.GA1733@xxxxxxxxxxxxx> <20140715153922.GB5369@xxxxxxxxxxxxx> <20140716003851.GO22339@dastard> <20140716081105.GB29924@xxxxxxxxxxxxx> <20140716231725.GF4453@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
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

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