Re: [PATCH 2/9] xfs: add support for large btree blocks

[Ben Myers <bpm@sgi.com>]

Hi Dave,

On Sat, Feb 23, 2013 at 01:27:22PM +1100, Dave Chinner wrote:
> On Fri, Feb 22, 2013 at 12:34:56PM +1100, Dave Chinner wrote:
> > On Fri, Feb 15, 2013 at 03:20:15PM -0600, Ben Myers wrote:
> > > On Tue, Jan 22, 2013 at 12:25:53AM +1100, Dave Chinner wrote:
> > > > From: Christoph Hellwig <hch@lst.de>
> > > > 
> > > > Add support for larger btree blocks that contains a CRC32C checksum,
> > > > a filesystem uuid and block number for detecting filesystem
> > > > consistency and out of place writes.
> > > > 
> > > > [dchinner@redhat.com] Also include an owner field to allow reverse
> > > > mappings to be implemented for improved repairability and a LSN
> > > > field to so that log recovery can easily determine the last
> > > > modification that made it to disk for each buffer.
> > > > 
> > > > [dchinner@redhat.com] Add buffer log format flags to indicate the
> > > > type of buffer to recovery so that we don't have to do blind magic
> > > > number tests to determine what the buffer is.
> > > > 
> > > > [dchinner@redhat.com] Modified to fit into the verifier structure.
> > > 
> > > This patch is far too large for a good review.  It needs to be split up into
> > > it's various ideas which you outlined in patch 0.  If you need to add dead code
> > > in each piece and then enable it at the end, that's fine with me.
> > 
> > You want it broken up into 7 or 8 separate patches - what does it
> > gain us? It'll take a week for me to do the patch monkeying and to
> > retest and validate the resulting stack (i.e. it is bisectable, each
> > patch passes xfstests, etc), and in the end the code will be
> > identical.
> > 
> > As I've said before, there's a point where the tradeoff for
> > splitting patches up doesn't make sense. Asking a developer to do
> > days of work to end up with exactly the same code to save the
> > reviewer an hour or two is *not* a good tradeoff. Especially for the
> > first patch of a much larger 15-20 patch series which contains
> > several larger and more complex patches....
> 
> Ben, reading this back it comes across as unnecessarily negative and
> narky. I was just about at the end of the attribute leaf changes and
> it's been a mind-numbing slog making mechanical changes to lots of
> code.
> 
> This is after having to do the same slog through all the directory
> code. The block, leaf, node, freespace and da_btree code all had to
> get the same treatment, and it's worn me down.  My wrists are
> starting to hurt from the last three months of slogging through this
> stuff (roughly +20,000/-10,000 lines modified according to a quick
> diffstat) and that doesn't make me a happy camper.
> 
> So I'm not meaning to be narky or nasty - I just want to get this
> stuff done ASAP before it burns me out. Hence the prospect of having
> to go back and redo significant chunks to split out trivial pieces
> of code (i.e. a mind-numbing slog making mechanical changes) hit a
> bit of a raw nerve.
> 
> All I'm asking is that you take into account the extra load that the
> rework you ask me to do adds and whether it is absolutely necessary
> to be able to review the code. The last thing I want is be burnt out
> by this stuff....

Once I get back to it I'll consider whether to pull it apart myself.  I had a
hard time keeping track of everything that was going on in that patch.  And it
seems like whenever that happens I miss something important in my review.
Working through the rest of the series may also help to get more comfortable
with this patch.

-Ben

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