Re: [PATCH 15/21] implement generic xfs_btree_rshift

[Dave Chinner <david@fromorbit.com>]

On Tue, Jul 29, 2008 at 09:31:25PM +0200, Christoph Hellwig wrote:
> Make the btree right shift code generic.  Based on a patch from David Chinner
> with lots of changes to follow the original btree implementations more
> closely.  While this loses some of the generic helper routines for
> inserting/moving/removing records it also solves some of the one off
> bugs in the original code and makes it easier to verify.

Looks good. A few comments below.

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> 
> Index: linux-2.6-xfs/fs/xfs/xfs_btree.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_btree.c	2008-07-27 17:40:52.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_btree.c	2008-07-28 16:13:33.000000000 +0200
> @@ -34,6 +34,7 @@
>  #include "xfs_attr_sf.h"
>  #include "xfs_dinode.h"
>  #include "xfs_inode.h"
> +#include "xfs_inode_item.h"
>  #include "xfs_btree.h"
>  #include "xfs_btree_trace.h"
>  #include "xfs_ialloc.h"
> @@ -952,6 +953,123 @@ xfs_btree_read_buf_block(
>  	return xfs_btree_check_block(cur, *block, level, *bpp);
>  }
>  
> +STATIC void
> +xfs_btree_move_ptrs(
> +	struct xfs_btree_cur	*cur,
> +	union xfs_btree_ptr	*base,
> +	int			from,
> +	int			to,
> +	int			numptrs)
> +{
> +	ASSERT(from >= 0 && from <= 1000);
> +	ASSERT(to >= 0 && to <= 1000);
> +	ASSERT(numptrs >= 0);

Those numbers are not safe. I plucked them out of thin air to verify
validity on 4k block size filesystem which had (IIRC) a max of about
500 ptrs to a block. It was throwaway debug code to find a problem.
Larger block sizes can well exceed 1000. So realistically, the only
valid assert there is this one:

	ASSERT(numptrs >= 0);

> +/*
> + * Log block pointer fields from a btree block (nonleaf).
> + */
> +STATIC void
> +xfs_btree_log_ptrs(
> +	struct xfs_btree_cur	*cur,	/* btree cursor */
> +	struct xfs_buf		*bp,	/* buffer containing btree block */
> +	int			pfirst,	/* index of first pointer to log */
> +	int			plast)	/* index of last pointer to log */

I'd call these first/last (being indexes), and these:

> +{
> +
> +	XFS_BTREE_TRACE_CURSOR(cur, XBT_ENTRY);
> +	XFS_BTREE_TRACE_ARGBII(cur, bp, pfirst, plast);
> +
> +	if (bp) {
> +		struct xfs_btree_block	*block = XFS_BUF_TO_BLOCK(bp);
> +		union xfs_btree_ptr	*pp;
> +		int			first;	/* first byte offset logged */
> +		int			last;	/* last byte offset logged */

start/end because they are byte range identifiers.

> +
> +		pp = cur->bc_ops->ptr_addr(cur, 1, block);
> +
> +		if (cur->bc_flags & XFS_BTREE_LONG_PTRS) {
> +			__be64		*lpp = &pp->l;
> +
> +			first = (int)((xfs_caddr_t)&lpp[pfirst - 1] -
> +					(xfs_caddr_t)block);
> +			last = (int)(((xfs_caddr_t)&lpp[plast] - 1) -
> +					(xfs_caddr_t)block);
> +		} else {
> +			__be32		*spp = &pp->s;
> +
> +			first = (int)((xfs_caddr_t)&spp[pfirst - 1] -
> +					(xfs_caddr_t)block);
> +			last = (int)(((xfs_caddr_t)&spp[plast] - 1) -
> +					(xfs_caddr_t)block);

Hmmmm. That's not very nice with all those casts. It's clear that
it's pointer arithmetic, but rather messy.

> +		}
> +
> +		xfs_trans_log_buf(cur->bc_tp, bp, first, last);

How about:

		union xfs_btree_ptr	*pp;
		xfs_caddr_t		*block = XFS_BUF_TO_BLOCK(bp);
		xfs_caddr_t		start;	/* first byte offset logged */
		xfs_caddr_t		end;	/* last byte offset logged */

		pp = cur->bc_ops->ptr_addr(cur, 1, XFS_BUF_TO_BLOCK(bp));

		if (cur->bc_flags & XFS_BTREE_LONG_PTRS) {
			__be64		*lpp = &pp->l;

			start = (xfs_caddr_t)&lpp[first - 1] - block;
			end  = ((xfs_caddr_t)&lpp[last] - 1) - block;
		} else {
			__be32		*spp = &pp->s;

			start = (xfs_caddr_t)&spp[first - 1] - block;
			end  = ((xfs_caddr_t)&spp[last] - 1) - block;
		}

		xfs_trans_log_buf(cur->bc_tp, bp, (int)start, (int)end);

That makes it much easier to read (to me, anyway).

> +	XFS_BTREE_TRACE_CURSOR(cur, XBT_ENTRY);
> +	XFS_BTREE_TRACE_ARGBI(cur, bp, fields);
> +
> +	if (bp) {
> +		xfs_btree_offsets(fields,
> +				  (cur->bc_flags & XFS_BTREE_LONG_PTRS) ?
> +				  	loffsets : soffsets,
                                ^^
Some stray whitespace there.

> +				  XFS_BB_NUM_BITS, &first, &last);
> +		xfs_trans_log_buf(cur->bc_tp, bp, first, last);
> +	} else {
> +		/* XXX(hch): maybe factor out into a method? */
> +		xfs_trans_log_inode(cur->bc_tp, cur->bc_private.b.ip,
> +			XFS_ILOG_FBROOT(cur->bc_private.b.whichfork));

I don't think it is necessary at this point.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com