REVIEW: Fix CI lookup in leaf-form directories

REVIEW: Fix CI lookup in leaf-form directories

[Barry Naujok]

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.


--- a/fs/xfs/xfs_dir2_leaf.c	2008-06-23 18:35:54.000000000 +1000
+++ b/fs/xfs/xfs_dir2_leaf.c	2008-06-23 18:27:38.214277630 +1000
@@ -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;		/* case match data block no. */
  	enum xfs_dacmp		cmp;		/* name compare result */

  	dp = args->dp;
@@ -1358,8 +1358,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;
+	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++) {
@@ -1377,7 +1376,7 @@ xfs_dir2_leaf_lookup_int(
  		 * need to pitch the old one and read the new one.
  		 */
  		if (newdb != curdb) {
-			if (dbp != cbp)
+			if (dbp)
  				xfs_da_brelse(tp, dbp);
  			error = xfs_da_read_buf(tp, dp,
  						xfs_dir2_db_to_da(mp, newdb),
@@ -1403,35 +1402,39 @@ xfs_dir2_leaf_lookup_int(
  		if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
  			args->cmpresult = cmp;
  			*indexp = index;
-			/*
-			 * case exact match: release the stored CI buffer if it
-			 * exists and return the current buffer.
-			 */
+			/* case exact match: return the current buffer. */
  			if (cmp == XFS_CMP_EXACT) {
-				if (cbp && cbp != dbp)
-					xfs_da_brelse(tp, cbp);
  				*dbpp = dbp;
  				return 0;
  			}
-			cbp = dbp;
+			cidb = curdb;
  		}
  	}
  	ASSERT(args->op_flags & XFS_DA_OP_OKNOENT);
  	/*
-	 * Here, we can only be doing a lookup (not a rename or replace).
-	 * If a case-insensitive match was found earlier, release the current
-	 * buffer and return the stored CI matching buffer.
+	 * Here, we can only be doing a lookup (not a rename or remove).
+	 * If a case-insensitive match was found earlier, re-read the
+	 * appropriate data block if required and return it.
  	 */
  	if (args->cmpresult == XFS_CMP_CASE) {
-		if (cbp != dbp)
+		ASSERT(cidb != -1);
+		if (cidb != curdb) {
  			xfs_da_brelse(tp, dbp);
-		*dbpp = cbp;
+			error = xfs_da_read_buf(tp, dp,
+						xfs_dir2_db_to_da(mp, cidb),
+						-1, &dbp, XFS_DATA_FORK);
+			if (error) {
+				xfs_da_brelse(tp, lbp);
+				return error;
+			}
+		}
+		*dbpp = dbp;
  		return 0;
  	}
  	/*
  	 * No match found, return ENOENT.
  	 */
-	ASSERT(cbp == NULL);
+	ASSERT(cidb == -1);
  	if (dbp)
  		xfs_da_brelse(tp, dbp);
  	xfs_da_brelse(tp, lbp);

Re: REVIEW: Fix CI lookup in leaf-form directories

[Christoph Hellwig]

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 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..

Re: REVIEW: Fix CI lookup in leaf-form directories

[Barry Naujok]

On Mon, 23 Jun 2008 19:37:18 +1000, Christoph Hellwig <hch@infradead.org>  
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++) {
  		/*

...