Re: [PATCH 13/27] xfs: factor out xfs_dir2_leaf_find_entry
From: Dave Chinner <david@fromorbit.com>
Date: 2011-06-30 06:11:09
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@lst.de>
Looks sane - a couple of minor whitespacy comments, otherwise: Reviewed-by: Dave Chinner <redacted>
quoted hunk ↗ jump to hunk
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...
quoted hunk ↗ jump to hunk
+ 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@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs