On Mon, 23 Jun 2008 19:37:18 +1000, Christoph Hellwig <hch@xxxxxxxxxxxxx>
wrote:
On Mon, Jun 23, 2008 at 06:44:58PM +1000, Barry Naujok wrote:
Along the same lines as the node-form directory patch last week,
the leaf-form is not handling directory buffers properly and
locks up.
Instead of comparing buffer pointers, compare buffer block numbers
and don't keep buffers hanging around.
Which might be a small performance penalty, but I think that's fine
for the CI lookup case.
The worst case performance hit is one extra read when there is a CI
match where the data entry in is a different block to the last
hash value in the leaf.
The patch looks good to me.
But one things in the original xfs_dir2_leaf_lookup_int that barely
touched by your patch really irks me:
- cbp = NULL;
- for (lep = &leaf->ents[index], dbp = NULL, curdb = -1;
+ for (lep = &leaf->ents[index], dbp = NULL, curdb = -1, cidb = -1;
index < be16_to_cpu(leaf->hdr.count) &&
be32_to_cpu(lep->hashval) == args->hashval;
lep++, index++) {
I'd really prefer to have not too much rather unrelated bits in the
for loop. In fact the use of the for construct here is more than odd
because there is no such things as a loop variable at all. I think
we'd be much better of in terms of readability with a simple while
loop with where all the initialization is moved out of the loop.
Probably not for this patch, though..
Too late :)
--- a/fs/xfs/xfs_dir2_leaf.c 2008-06-24 10:09:38.000000000 +1000
+++ b/fs/xfs/xfs_dir2_leaf.c 2008-06-24 10:07:30.382990266 +1000
@@ -1321,8 +1321,8 @@ xfs_dir2_leaf_lookup_int(
int *indexp, /* out: index in leaf block */
xfs_dabuf_t **dbpp) /* out: data buffer */
{
- xfs_dir2_db_t curdb; /* current data block number */
- xfs_dabuf_t *dbp; /* data buffer */
+ xfs_dir2_db_t curdb = -1; /* current data block number */
+ xfs_dabuf_t *dbp = NULL; /* data buffer */
xfs_dir2_data_entry_t *dep; /* data entry */
xfs_inode_t *dp; /* incore directory inode */
int error; /* error return code */
@@ -1333,7 +1333,7 @@ xfs_dir2_leaf_lookup_int(
xfs_mount_t *mp; /* filesystem mount point */
xfs_dir2_db_t newdb; /* new data block number */
xfs_trans_t *tp; /* transaction pointer */
- xfs_dabuf_t *cbp; /* case match data buffer */
+ xfs_dir2_db_t cidb = -1; /* case match data block no. */
enum xfs_dacmp cmp; /* name compare result */
dp = args->dp;
@@ -1358,9 +1357,7 @@ xfs_dir2_leaf_lookup_int(
* Loop over all the entries with the right hash value
* looking to match the name.
*/
- cbp = NULL;
- for (lep = &leaf->ents[index], dbp = NULL, curdb = -1;
- index < be16_to_cpu(leaf->hdr.count) &&
+ for (lep = &leaf->ents[index]; index < be16_to_cpu(leaf->hdr.count) &&
be32_to_cpu(lep->hashval) == args->hashval;
lep++, index++) {
/*
...
|