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
I think this is a good approach to take. It certainly makes the
move/copy/logging interfaces simpler and easier for future btrees
> 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