xfs
[Top] [All Lists]

Re: [PATCH 16/21] implement generic xfs_btree_lshift

To: Christoph Hellwig <hch@xxxxxx>
Subject: Re: [PATCH 16/21] implement generic xfs_btree_lshift
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 30 Jul 2008 16:24:22 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20080729193132.GQ19104@xxxxxx>
Mail-followup-to: Christoph Hellwig <hch@xxxxxx>, xfs@xxxxxxxxxxx
References: <20080729193132.GQ19104@xxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.18 (2008-05-17)
On Tue, Jul 29, 2008 at 09:31:32PM +0200, Christoph Hellwig wrote:
> Make the btree left 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.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> Index: linux-2.6-xfs/fs/xfs/xfs_btree.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_btree.c     2008-07-28 16:13:33.000000000 
> +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_btree.c  2008-07-28 16:16:22.000000000 +0200
> @@ -976,6 +976,21 @@ xfs_btree_move_ptrs(
>       }
>  }
>  
> +STATIC void
> +xfs_btree_copy_ptrs(
> +     struct xfs_btree_cur    *cur,
> +     union xfs_btree_ptr     *src_ptr,
> +     union xfs_btree_ptr     *dst_ptr,
> +     int                     numptrs)
> +{
> +     ASSERT(numptrs > 0);
> +
> +     if (cur->bc_flags & XFS_BTREE_LONG_PTRS)
> +             memcpy(dst_ptr, src_ptr, numptrs * sizeof(__be64));
> +     else
> +             memcpy(dst_ptr, src_ptr, numptrs * sizeof(__be32));
> +}

These should really use memmove, not memcpy. There is no guarantee
the source and destination do not overlap.

At minimum, we need comments to say this must only be used to
copy between blocks, and xfs_btree_move_ptrs() must be used to
copy within a block. I note the original patchset of mine
commented on this distinction when defining the ->move_* and
->copy_* operations.

FWIW, that also helps explain why they have different interfaces...

> +
>  /*
>   * Log block pointer fields from a btree block (nonleaf).
>   */
> @@ -1597,6 +1612,188 @@ error0:
>  }
>  
>  /*
> + * Move 1 record left from cur/level if possible.
> + * Update cur to reflect the new path.
> + */
> +int                                  /* error */
> +xfs_btree_lshift(
> +     struct xfs_btree_cur    *cur,
> +     int                     level,
> +     int                     *stat)          /* success/failure */
> +{
> +     union xfs_btree_key     key;            /* btree key */
> +     struct xfs_buf          *lbp;           /* left buffer pointer */
> +     struct xfs_btree_block  *left;          /* left btree block */
> +     int                     lrecs;          /* left record count */
> +     struct xfs_buf          *rbp;           /* right buffer pointer */
> +     struct xfs_btree_block  *right;         /* right btree block */
> +     int                     rrecs;          /* right record count */
> +     union xfs_btree_ptr     lptr;           /* left btree pointer */
> +     union xfs_btree_key     *rkp = NULL;    /* right btree key */
> +     union xfs_btree_ptr     *rpp = NULL;    /* right address pointer */
> +     union xfs_btree_rec     *rrp = NULL;    /* right record pointer */
> +     int                     error;          /* error return value */
> +#ifdef DEBUG
> +     int                     i;              /* loop index */
> +#endif

This can be moved inside the branch it is used in.

> +     lrecs++;
> +     rrecs--;
> +
> +     XFS_BTREE_STATS_INC(cur, lshift);
> +
> +     /*
> +      * If non-leaf, copy a key and a ptr to the left block.
> +      * Log the changes to the left block.
> +      */
> +     XFS_BTREE_STATS_ADD(cur, moves, 1);

Move the XFS_BTREE_STATS_ADD() above the comment to match the rshift
code.

Otherwise looks good.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx


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