Re: [PATCH 26/26] add rec_len and key_len fields to struct xfs_btree_ops

[Dave Chinner <david@xxxxxxxxxxxxx>]

On Mon, Aug 04, 2008 at 03:36:08AM +0200, Christoph Hellwig wrote:
> XXX: add detailed patch explanation here

heh ;)

.....

> Index: linux-2.6-xfs/fs/xfs/xfs_btree.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_btree.c     2008-08-04 01:48:35.000000000 
> +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_btree.c  2008-08-04 01:49:13.000000000 +0200
> @@ -1037,6 +1037,82 @@ xfs_btree_read_buf_block(
>       return error;
>  }
>  
> +static inline size_t xfs_btree_block_len(struct xfs_btree_cur *cur)
> +{
> +     return (cur->bc_flags & XFS_BTREE_LONG_PTRS) ?
> +             sizeof(struct xfs_btree_lblock) :
> +             sizeof(struct xfs_btree_sblock);
> +}

That's really the block header length, not the block length.
Can you change the name to reflect that?

> +
> +static inline size_t xfs_btree_ptr_len(struct xfs_btree_cur *cur)
> +{
> +     return (cur->bc_flags & XFS_BTREE_LONG_PTRS) ?
> +             sizeof(__be64) : sizeof(__be32);
> +}
> +
> +static size_t
> +xfs_btree_ptr_offset(
> +     struct xfs_btree_cur    *cur,
> +     int                     index,
> +     int                     level)
> +{
> +     return xfs_btree_block_len(cur) +
> +             cur->bc_ops->get_maxrecs(cur, level) * cur->bc_ops->key_len +
> +             (index - 1) * xfs_btree_ptr_len(cur);
> +}

I'd suggest a comment here just reminding the reader about the
key/ptr block structure. i.e. key space is at the start of the
block, ptr space is after the key space.

Also a comment here reminding that btree indexes are 1-numbered,
not 0-numbered and hence the need for 'index - 1' in the
offset calculations.

>  STATIC void
> +xfs_btree_set_key(
> +     struct xfs_btree_cur    *cur,
> +     union xfs_btree_key     *key_addr,
> +     int                     index,
> +     union xfs_btree_key     *newkey)
> +{
> +     char                    *kp;
> +
> +     kp = (char *)key_addr + (index * cur->bc_ops->key_len);
> +
> +     memcpy(kp, newkey, cur->bc_ops->key_len);
> +}

And then for these set functions, the index has already been
converted from 1-numbered to 0-numbered by the caller, hence the
direct use of 'index' without correction.

> @@ -1079,6 +1183,44 @@ xfs_btree_move_ptrs(
>  }
>  
>  STATIC void
> +xfs_btree_move_keys(
> +     struct xfs_btree_cur    *cur,
> +     union xfs_btree_key     *key,
> +     int                     from,
> +     int                     to,
> +     int                     numkeys)
> +{
> +     char                    *base = (char *)key;
> +
> +     ASSERT(from >= 0);
> +     ASSERT(to >= 0);
> +     ASSERT(numkeys >= 0);
> +
> +     memmove(base + (to * cur->bc_ops->key_len),
> +             base + (from * cur->bc_ops->key_len),
> +             numkeys * cur->bc_ops->key_len);
> +}

Comment about from and to being logical offsets from the base,
not byte counts.

Hmmm - seeing how frequently the functions like xfs_btree_key_addr()
are called, should we prevent them from being inlined automatically
by the compiler (i.e. make them STATIC or explictly noinline)?

Otherwise seems sane to me.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx