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

[Dave Chinner <david@fromorbit.com>]

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@fromorbit.com