xfs
[Top] [All Lists]

Re: [PATCH 03/11] xfs: factor out xfs_dir2_leaf_find_stale

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 03/11] xfs: factor out xfs_dir2_leaf_find_stale
From: Alex Elder <aelder@xxxxxxx>
Date: Mon, 11 Jul 2011 17:32:53 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <20110710205017.293539533@xxxxxxxxxxxxxxxxxxxxxx>
References: <20110710204916.856267100@xxxxxxxxxxxxxxxxxxxxxx> <20110710205017.293539533@xxxxxxxxxxxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Sun, 2011-07-10 at 16:49 -0400, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

OK, I guess this is fine.  I have a suggestion below
but I see nothing truly wrong with what you have.

Reviewed-by: Alex Elder <aelder@xxxxxxx>

> Index: xfs/fs/xfs/xfs_dir2_leaf.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_dir2_leaf.c   2011-07-09 12:07:49.518499801 +0200
> +++ xfs/fs/xfs/xfs_dir2_leaf.c        2011-07-09 12:19:48.171796748 +0200
> @@ -148,6 +148,36 @@ xfs_dir2_block_to_leaf(
>       return 0;
>  }
>  
> +STATIC void
> +xfs_dir2_leaf_find_stale(
> +     struct xfs_dir2_leaf    *leaf,
> +     int                     index,
> +     int                     *lowstale,
> +     int                     *highstale)
> +{
> +     /*
> +      * Find the first stale entry before our index, if any.
> +      */
> +     for (*lowstale = index - 1;
> +          *lowstale >= 0 &&
> +             leaf->ents[*lowstale].address !=
> +             cpu_to_be32(XFS_DIR2_NULL_DATAPTR);
> +          --*lowstale)
> +             continue;
> +
> +     /*
> +      * Find the first stale entry at or after our index, if any.
> +      * Stop if the answer would be worse than lowstale.

              Stop if the result would require moving more
              entries than using lowstale.

(I realize you didn't change this comment, you just moved
it into this helper function.)

Actually it seems like this searching of the stale
entries and moving things around among them could
be broken into a few even finer-grained utility
routines.  It just seems like what this code is
doing is simpler than what the code complexity
suggests (though I haven't really looked at this
stuff much before).

> +      */
> +     for (*highstale = index;
> +          *highstale < be16_to_cpu(leaf->hdr.count) &&
> +             leaf->ents[*highstale].address !=
> +             cpu_to_be32(XFS_DIR2_NULL_DATAPTR) &&
> +             (*lowstale < 0 || index - *lowstale > *highstale - index);
> +          ++*highstale)
> +             continue;
> +}
> +
>  struct xfs_dir2_leaf_entry *
>  xfs_dir2_leaf_find_entry(
>       xfs_dir2_leaf_t         *leaf,          /* leaf structure */

. . .

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