[Top] [All Lists]

Re: [PATCH 00/26] generic btree implementation, version 3

To: Christoph Hellwig <hch@xxxxxx>
Subject: Re: [PATCH 00/26] generic btree implementation, version 3
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 4 Aug 2008 11:54:25 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20080804013158.GA8819@xxxxxx>
Mail-followup-to: Christoph Hellwig <hch@xxxxxx>, xfs@xxxxxxxxxxx
References: <20080804013158.GA8819@xxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.18 (2008-05-17)
On Mon, Aug 04, 2008 at 03:31:58AM +0200, Christoph Hellwig wrote:
> Firstly all but one of Dave's review comments are addressed. There is also
> a new first patch to clean up the defintion of struct xfs_btree_block which
> I would consider an ASAP merge candidate.
> But there are also some bigger changes.  For reimplemented the newroot and
> killroot methods.  In both cases one method was used for block rooted and
> inode rooted btree but doing something quite different.  This patchset
> gets rid of these two method completely and implements both cases in
> xfs_btree.c.  For new_root that was as easy as just calling
> xfs_btree_new_root directly for the !XFS_BTREE_ROOT_IN_INODE and moving
> xfs_bmbt_newroot into the common code and calling it for the other case.
> The XFS_BTREE_ROOT_IN_INODE case for kill_root was similarly easily solved
> by just moving the code around and making it block/rec/key/ptr size
> agnostic, but the !XFS_BTREE_ROOT_IN_INODE needed a little care.
> The alloc and inobt trees were in fact using the almost exact sequence
> built from ->set_root and ->free_block but the odd way to pass the
> block to be freed made this non-obvious.  By changing the calling
> convention to the normal one this one could be unified.  I've left
> in some nasty asserts to ensure the assumptions about these blocks
> wasn't wrong.

Ok. I'll comment on it when the patches come through....

> The other big news is the last patch in the series, which adds rec_len
> and key_len members to the btree_ops structure and uses this to make the
> ptr_addr, key_addr, rec_addr, set_key, set_rec, move_keys, move_recs,
> copy_keys, copy_recsm log_keys and log_recs methods generic.  This
> patch by itselfs saves a few hundred lines of code and more than two
> kilobytes in the binary image.  With this eventual worse generated
> code that needs to multiply by variables instead of constants should
> easily be offset by the reduced instruction cache footprint.  The patch
> is so far not really comments and more a proof of concept - if everyone
> agrees with this approach the changes will be merged into the earlier
> patches.

I think this is a good approach to take. It certainly makes the
move/copy/logging interfaces simpler and easier for future btrees
to implement....

> Now even with this move the set_*, move_* and copy_* interfaces are
> left as-is.  All the old discussion still applies except that things
> might be a little more clear now that there's just one implementation
> each and everything is contained in one file.  I think keeping the
> copy_* interfaces is a good idea, they are now basically just typed
> memcpy wrappers - although we should switch the order of the src
> and dst arguments to be the same as memcpy.


> The set_* interfaces
> can probably go away - over copy_ they just add the index based
> addressing which most callers don't use anyway. 

Yes, they could call the copy interface directly.

> I'm not sure
> what to do with move_* - these are the most ugly helpers, so maybe
> we should just make them memmove wrappers in the style of copy_
> and leave all addressing to the callers.

Yes, it would be nice to have them use the same interface. If
we do that, then there's no real point for having a copy vs move
distinction - we could just make everything use the
memmove version and drop one of the interfaces altogether....

> With all these changes the stats for this series are now:
>  37 files changed, 6309 insertions(+), 7873 deletions(-)
>    text          data     bss     dec     hex filename
>  631577          4227    3092  638896   9bfb0 fs/xfs/xfs.ko.base
>  614298          4435    3124  621857   97d21 fs/xfs/xfs.ko.btree

FWIW, this aggregate doesn't show the real picture of the savings.
What is really interesting is how small the individual
implementations become....


Dave Chinner

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