xfs
[Top] [All Lists]

Re: [PATCH 13/27] xfs: factor out xfs_dir2_leaf_find_entry

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 13/27] xfs: factor out xfs_dir2_leaf_find_entry
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 30 Jun 2011 16:11:02 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20110629140339.086201354@xxxxxxxxxxxxxxxxxxxxxx>
References: <20110629140109.003209430@xxxxxxxxxxxxxxxxxxxxxx> <20110629140339.086201354@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Wed, Jun 29, 2011 at 10:01:22AM -0400, Christoph Hellwig wrote:
> Add a new xfs_dir2_leaf_find_entry helper to factor out some duplicate code
> from xfs_dir2_leaf_addname xfs_dir2_leafn_add.  Found by Eric Sandeen using
> an automated code duplication checked.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Looks sane - a couple of minor whitespacy comments, otherwise:

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

> 
> Index: xfs/fs/xfs/xfs_dir2_leaf.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_dir2_leaf.c   2011-06-22 21:56:26.102462981 +0200
> +++ xfs/fs/xfs/xfs_dir2_leaf.c        2011-06-23 12:41:51.716439911 +0200
> @@ -152,6 +152,118 @@ xfs_dir2_block_to_leaf(
>       return 0;
>  }
>  
> +xfs_dir2_leaf_entry_t *
> +xfs_dir2_leaf_find_entry(
> +     xfs_dir2_leaf_t         *leaf,          /* leaf structure */
> +     int                     index,          /* leaf table position */
> +     int                     compact,        /* need to compact leaves */
> +     int                     lowstale,       /* index of prev stale leaf */
> +     int                     highstale,      /* index of next stale leaf */
> +     int                     *lfloglow,      /* low leaf logging index */
> +     int                     *lfloghigh)     /* high leaf logging index */
> +{
> +     xfs_dir2_leaf_entry_t   *lep;           /* leaf entry table pointer */
> +
> +     if (!leaf->hdr.stale) {
> +             /*
> +              * Now we need to make room to insert the leaf entry.
> +              *
> +              * If there are no stale entries, just insert a hole at index.
> +              */
> +             lep = &leaf->ents[index];
> +             if (index < be16_to_cpu(leaf->hdr.count))
> +                     memmove(lep + 1, lep,
> +                             (be16_to_cpu(leaf->hdr.count) - index) *
> +                              sizeof(*lep));
> +
> +             /*
> +              * Record low and high logging indices for the leaf.
> +              */
> +             *lfloglow = index;
> +             *lfloghigh = be16_to_cpu(leaf->hdr.count);
> +             be16_add_cpu(&leaf->hdr.count, 1);

You could probably just return here, and that would remove the:

> +     } else {

and the indenting that the else branch causes.

> +             /*
> +              * There are stale entries.
> +              *
> +              * We will use one of them for the new entry.  It's probably
> +              * not at the right location, so we'll have to shift some up
> +              * or down first.
> +              *
> +              * If we didn't compact before, we need to find the nearest
> +              * stale entries before and after our insertion point.
> +              */
> +             if (compact == 0) {
> +                     /*
> +                      * Find the first stale entry before the insertion
> +                      * point, if any.
> +                      */
> +                     for (lowstale = index - 1;
> +                          lowstale >= 0 &&
> +                             be32_to_cpu(leaf->ents[lowstale].address) !=
> +                             XFS_DIR2_NULL_DATAPTR;
> +                          lowstale--)
> +                             continue;
> +                     /*
> +                      * Find the next stale entry at or after the insertion
> +                      * point, if any.   Stop if we go so far that the
> +                      * lowstale entry would be better.
> +                      */
> +                     for (highstale = index;
> +                          highstale < be16_to_cpu(leaf->hdr.count) &&
> +                             be32_to_cpu(leaf->ents[highstale].address) !=
> +                             XFS_DIR2_NULL_DATAPTR &&
> +                             (lowstale < 0 ||
> +                              index - lowstale - 1 >= highstale - index);
> +                          highstale++)
> +                             continue;
> +             }
> +             /*
> +              * If the low one is better, use it.
> +              */

Line of whitespace before the comment.

> +             if (lowstale >= 0 &&
> +                 (highstale == be16_to_cpu(leaf->hdr.count) ||
> +                  index - lowstale - 1 < highstale - index)) {
> +                     ASSERT(index - lowstale - 1 >= 0);
> +                     ASSERT(be32_to_cpu(leaf->ents[lowstale].address) ==
> +                            XFS_DIR2_NULL_DATAPTR);
> +                     /*
> +                      * Copy entries up to cover the stale entry
> +                      * and make room for the new entry.
> +                      */
> +                     if (index - lowstale - 1 > 0)
> +                             memmove(&leaf->ents[lowstale],
> +                                     &leaf->ents[lowstale + 1],
> +                                     (index - lowstale - 1) * sizeof(*lep));
> +                     lep = &leaf->ents[index - 1];
> +                     *lfloglow = MIN(lowstale, *lfloglow);
> +                     *lfloghigh = MAX(index - 1, *lfloghigh);
> +
> +             /*
> +              * The high one is better, so use that one.
> +              */
> +             } else {

I prefer comments inside the else branch...

> +                     ASSERT(highstale - index >= 0);
> +                     ASSERT(be32_to_cpu(leaf->ents[highstale].address) ==
> +                            XFS_DIR2_NULL_DATAPTR);
> +                     /*
> +                      * Copy entries down to cover the stale entry
> +                      * and make room for the new entry.
> +                      */
> +                     if (highstale - index > 0)
> +                             memmove(&leaf->ents[index + 1],
> +                                     &leaf->ents[index],
> +                                     (highstale - index) * sizeof(*lep));
> +                     lep = &leaf->ents[index];
> +                     *lfloglow = MIN(index, *lfloglow);
> +                     *lfloghigh = MAX(highstale, *lfloghigh);
> +             }
> +             be16_add_cpu(&leaf->hdr.stale, -1);
> +     }
> +
> +     return lep;
> +}
> +
>  /*
>   * Add an entry to a leaf form directory.
>   */
> @@ -430,102 +542,11 @@ xfs_dir2_leaf_addname(
.....
> -             }
> -             be16_add_cpu(&leaf->hdr.stale, -1);
> -     }
> +
> +
> +     lep = xfs_dir2_leaf_find_entry(leaf, index, compact, lowstale,
> +                                    highstale, &lfloglow, &lfloghigh);
> +

Only need one line of whitespace before the function call.

.....
> -                     lep = &leaf->ents[index];
> -                     lfloglow = MIN(index, lfloglow);
> -                     lfloghigh = MAX(highstale, lfloghigh);
> -             }
> -             be16_add_cpu(&leaf->hdr.stale, -1);
> -     }
> +
> +
>       /*
>        * Insert the new entry, log everything.
>        */
> +     lep = xfs_dir2_leaf_find_entry(leaf, index, compact, lowstale,
> +                                    highstale, &lfloglow, &lfloghigh);
> +

Same for the whitespace before the comment.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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