[PATCH] XFS: Fix returning case-preserved name with CI node form directories

[PATCH] XFS: Fix returning case-preserved name with CI node form directories

[Barry Naujok]

[-- Attachment #1: Type: text/plain, Size: 5035 bytes --]

xfs_dir2_node_lookup() calls xfs_da_node_lookup_int() which iterates
through leaf blocks containing the matching hash value for the name
being looked up. Inside xfs_da_node_lookup_int(), it calls the
xfs_dir2_leafn_lookup_for_entry() for each leaf block.
xfs_dir2_leafn_lookup_for_entry() iterates through each matching
hash/offset pair doing a name comparison to find the matching
dirent.

For CI mode, the state->extrablk retains the details of the block
that has the CI match so xfs_dir2_node_lookup() can return the
case-preserved name.

The original implementation didn't retain the xfs_da_buf_t properly,
so the lookup was returning a bogus name to be stored in the dentry.

In the case of unlink, the bad name was passed and in debug mode,
ASSERTed when it can't find the entry.


---
  fs/xfs/xfs_dir2_node.c |   69  
++++++++++++++++++++++++++++++-------------------
  1 file changed, 43 insertions(+), 26 deletions(-)

Index: 2.6.x-xfs/fs/xfs/xfs_dir2_node.c
===================================================================
--- 2.6.x-xfs.orig/fs/xfs/xfs_dir2_node.c
+++ 2.6.x-xfs/fs/xfs/xfs_dir2_node.c
@@ -549,7 +549,6 @@ xfs_dir2_leafn_lookup_for_entry(
  	xfs_dir2_data_entry_t	*dep;		/* data block entry */
  	xfs_inode_t		*dp;		/* incore directory inode */
  	int			error;		/* error return value */
-	int			di = -1;	/* data entry index */
  	int			index;		/* leaf entry index */
  	xfs_dir2_leaf_t		*leaf;		/* leaf structure */
  	xfs_dir2_leaf_entry_t	*lep;		/* leaf entry */
@@ -577,7 +576,6 @@ xfs_dir2_leafn_lookup_for_entry(
  	if (state->extravalid) {
  		curbp = state->extrablk.bp;
  		curdb = state->extrablk.blkno;
-		di = state->extrablk.index;
  	}
  	/*
  	 * Loop over leaf entries with the right hash value.
@@ -602,17 +600,27 @@ xfs_dir2_leafn_lookup_for_entry(
  		 */
  		if (newdb != curdb) {
  			/*
-			 * If we had a block before, drop it.
+			 * If we had a block before that we aren't saving
+			 * for a CI name, drop it
  			 */
-			if (curbp)
+			if (curbp && (args->cmpresult == XFS_CMP_DIFFERENT ||
+						curdb != state->extrablk.blkno))
  				xfs_da_brelse(tp, curbp);
  			/*
-			 * Read the data block.
+			 * If needing the block that is saved with a CI match,
+			 * use it otherwise read in the new data block.
  			 */
-			error = xfs_da_read_buf(tp, dp, xfs_dir2_db_to_da(mp,
-					newdb), -1, &curbp, XFS_DATA_FORK);
-			if (error)
-				return error;
+			if (args->cmpresult != XFS_CMP_DIFFERENT &&
+					newdb == state->extrablk.blkno) {
+				ASSERT(state->extravalid);
+				curbp = state->extrablk.bp;
+			} else {
+				error = xfs_da_read_buf(tp, dp,
+						xfs_dir2_db_to_da(mp, newdb),
+						-1, &curbp, XFS_DATA_FORK);
+				if (error)
+					return error;
+			}
  			xfs_dir2_data_check(dp, curbp);
  			curdb = newdb;
  		}
@@ -624,38 +632,47 @@ xfs_dir2_leafn_lookup_for_entry(
  		/*
  		 * Compare the entry and if it's an exact match, return
  		 * EEXIST immediately. If it's the first case-insensitive
-		 * match, store the inode number and continue looking.
+		 * match, store the block & inode number and continue looking.
  		 */
  		cmp = mp->m_dirnameops->compname(args, dep->name, dep->namelen);
  		if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
+			/* If there is a CI match block, drop it */
+			if (args->cmpresult != XFS_CMP_DIFFERENT &&
+						curdb != state->extrablk.blkno)
+				xfs_da_brelse(tp, state->extrablk.bp);
  			args->cmpresult = cmp;
  			args->inumber = be64_to_cpu(dep->inumber);
-			di = (int)((char *)dep - (char *)curbp->data);
-			error = EEXIST;
+			*indexp = index;
+			state->extravalid = 1;
+			state->extrablk.bp = curbp;
+			state->extrablk.blkno = curdb;
+			state->extrablk.index = (int)((char *)dep -
+							(char *)curbp->data);
+			state->extrablk.magic = XFS_DIR2_DATA_MAGIC;
  			if (cmp == XFS_CMP_EXACT)
-				goto out;
+				return XFS_ERROR(EEXIST);
  		}
  	}
-	/* Didn't find an exact match. */
-	error = ENOENT;
  	ASSERT(index == be16_to_cpu(leaf->hdr.count) ||
  					(args->op_flags & XFS_DA_OP_OKNOENT));
-out:
  	if (curbp) {
-		/* Giving back a data block. */
-		state->extravalid = 1;
-		state->extrablk.bp = curbp;
-		state->extrablk.index = di;
-		state->extrablk.blkno = curdb;
-		state->extrablk.magic = XFS_DIR2_DATA_MAGIC;
+		if (args->cmpresult == XFS_CMP_DIFFERENT) {
+			/* Giving back last used data block. */
+			state->extravalid = 1;
+			state->extrablk.bp = curbp;
+			state->extrablk.index = -1;
+			state->extrablk.blkno = curdb;
+			state->extrablk.magic = XFS_DIR2_DATA_MAGIC;
+		} else {
+			/* If the curbp is not the CI match block, drop it */
+			if (state->extrablk.bp != curbp)
+				xfs_da_brelse(tp, curbp);
+		}
  	} else {
  		state->extravalid = 0;
  	}
-	/*
-	 * Return the index, that will be the deletion point for remove/replace.
-	 */
  	*indexp = index;
-	return XFS_ERROR(error);
+	return XFS_ERROR(ENOENT);
  }

  /*

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix_node_form_dir_ci_name_return.patch --]
[-- Type: text/x-patch; name=fix_node_form_dir_ci_name_return.patch, Size: 5067 bytes --]

XFS: Fix returning case-preserved name with CI node form directories

xfs_dir2_node_lookup() calls xfs_da_node_lookup_int() which iterates
through leaf blocks containing the matching hash value for the name
being looked up. Inside xfs_da_node_lookup_int(), it calls the 
xfs_dir2_leafn_lookup_for_entry() for each leaf block. 
xfs_dir2_leafn_lookup_for_entry() iterates through each matching
hash/offset pair doing a name comparison to find the matching 
dirent. 

For CI mode, the state->extrablk retains the details of the block
that has the CI match so xfs_dir2_node_lookup() can return the
case-preserved name.

The original implementation didn't retain the xfs_da_buf_t properly,
so the lookup was returning a bogus name to be stored in the dentry.

In the case of unlink, the bad name was passed and in debug mode,
ASSERTed when it can't find the entry.


---
 fs/xfs/xfs_dir2_node.c |   69 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 26 deletions(-)

Index: 2.6.x-xfs/fs/xfs/xfs_dir2_node.c
===================================================================
--- 2.6.x-xfs.orig/fs/xfs/xfs_dir2_node.c
+++ 2.6.x-xfs/fs/xfs/xfs_dir2_node.c
@@ -549,7 +549,6 @@ xfs_dir2_leafn_lookup_for_entry(
 	xfs_dir2_data_entry_t	*dep;		/* data block entry */
 	xfs_inode_t		*dp;		/* incore directory inode */
 	int			error;		/* error return value */
-	int			di = -1;	/* data entry index */
 	int			index;		/* leaf entry index */
 	xfs_dir2_leaf_t		*leaf;		/* leaf structure */
 	xfs_dir2_leaf_entry_t	*lep;		/* leaf entry */
@@ -577,7 +576,6 @@ xfs_dir2_leafn_lookup_for_entry(
 	if (state->extravalid) {
 		curbp = state->extrablk.bp;
 		curdb = state->extrablk.blkno;
-		di = state->extrablk.index;
 	}
 	/*
 	 * Loop over leaf entries with the right hash value.
@@ -602,17 +600,27 @@ xfs_dir2_leafn_lookup_for_entry(
 		 */
 		if (newdb != curdb) {
 			/*
-			 * If we had a block before, drop it.
+			 * If we had a block before that we aren't saving
+			 * for a CI name, drop it
 			 */
-			if (curbp)
+			if (curbp && (args->cmpresult == XFS_CMP_DIFFERENT ||
+						curdb != state->extrablk.blkno))
 				xfs_da_brelse(tp, curbp);
 			/*
-			 * Read the data block.
+			 * If needing the block that is saved with a CI match,
+			 * use it otherwise read in the new data block.
 			 */
-			error = xfs_da_read_buf(tp, dp, xfs_dir2_db_to_da(mp,
-					newdb), -1, &curbp, XFS_DATA_FORK);
-			if (error)
-				return error;
+			if (args->cmpresult != XFS_CMP_DIFFERENT &&
+					newdb == state->extrablk.blkno) {
+				ASSERT(state->extravalid);
+				curbp = state->extrablk.bp;
+			} else {
+				error = xfs_da_read_buf(tp, dp,
+						xfs_dir2_db_to_da(mp, newdb),
+						-1, &curbp, XFS_DATA_FORK);
+				if (error)
+					return error;
+			}
 			xfs_dir2_data_check(dp, curbp);
 			curdb = newdb;
 		}
@@ -624,38 +632,47 @@ xfs_dir2_leafn_lookup_for_entry(
 		/*
 		 * Compare the entry and if it's an exact match, return
 		 * EEXIST immediately. If it's the first case-insensitive
-		 * match, store the inode number and continue looking.
+		 * match, store the block & inode number and continue looking.
 		 */
 		cmp = mp->m_dirnameops->compname(args, dep->name, dep->namelen);
 		if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
+			/* If there is a CI match block, drop it */
+			if (args->cmpresult != XFS_CMP_DIFFERENT &&
+						curdb != state->extrablk.blkno)
+				xfs_da_brelse(tp, state->extrablk.bp);
 			args->cmpresult = cmp;
 			args->inumber = be64_to_cpu(dep->inumber);
-			di = (int)((char *)dep - (char *)curbp->data);
-			error = EEXIST;
+			*indexp = index;
+			state->extravalid = 1;
+			state->extrablk.bp = curbp;
+			state->extrablk.blkno = curdb;
+			state->extrablk.index = (int)((char *)dep -
+							(char *)curbp->data);
+			state->extrablk.magic = XFS_DIR2_DATA_MAGIC;
 			if (cmp == XFS_CMP_EXACT)
-				goto out;
+				return XFS_ERROR(EEXIST);
 		}
 	}
-	/* Didn't find an exact match. */
-	error = ENOENT;
 	ASSERT(index == be16_to_cpu(leaf->hdr.count) ||
 					(args->op_flags & XFS_DA_OP_OKNOENT));
-out:
 	if (curbp) {
-		/* Giving back a data block. */
-		state->extravalid = 1;
-		state->extrablk.bp = curbp;
-		state->extrablk.index = di;
-		state->extrablk.blkno = curdb;
-		state->extrablk.magic = XFS_DIR2_DATA_MAGIC;
+		if (args->cmpresult == XFS_CMP_DIFFERENT) {
+			/* Giving back last used data block. */
+			state->extravalid = 1;
+			state->extrablk.bp = curbp;
+			state->extrablk.index = -1;
+			state->extrablk.blkno = curdb;
+			state->extrablk.magic = XFS_DIR2_DATA_MAGIC;
+		} else {
+			/* If the curbp is not the CI match block, drop it */
+			if (state->extrablk.bp != curbp)
+				xfs_da_brelse(tp, curbp);
+		}
 	} else {
 		state->extravalid = 0;
 	}
-	/*
-	 * Return the index, that will be the deletion point for remove/replace.
-	 */
 	*indexp = index;
-	return XFS_ERROR(error);
+	return XFS_ERROR(ENOENT);
 }
 
 /*

Re: [PATCH] XFS: Fix returning case-preserved name with CI node form directories

[Christoph Hellwig]

On Wed, Jun 18, 2008 at 07:07:59PM +1000, Barry Naujok wrote:
> xfs_dir2_node_lookup() calls xfs_da_node_lookup_int() which iterates
> through leaf blocks containing the matching hash value for the name
> being looked up. Inside xfs_da_node_lookup_int(), it calls the
> xfs_dir2_leafn_lookup_for_entry() for each leaf block.
> xfs_dir2_leafn_lookup_for_entry() iterates through each matching
> hash/offset pair doing a name comparison to find the matching
> dirent.
>
> For CI mode, the state->extrablk retains the details of the block
> that has the CI match so xfs_dir2_node_lookup() can return the
> case-preserved name.
>
> The original implementation didn't retain the xfs_da_buf_t properly,
> so the lookup was returning a bogus name to be stored in the dentry.
>
> In the case of unlink, the bad name was passed and in debug mode,
> ASSERTed when it can't find the entry.

Looks good to me, although I don't really like how much of a mess
xfs_dir2_leafn_lookup_for_entry has become. No idea for a nice why
to write it, though.

Btw, any chance we could get some CI tests in xfsqa?  Looks like you
have quite a few testcases that found issues so far.

Re: [PATCH] XFS: Fix returning case-preserved name with CI node form directories

[Barry Naujok]

On Thu, 19 Jun 2008 18:32:43 +1000, Christoph Hellwig <hch@infradead.org>  
wrote:

> On Wed, Jun 18, 2008 at 07:07:59PM +1000, Barry Naujok wrote:
>> xfs_dir2_node_lookup() calls xfs_da_node_lookup_int() which iterates
>> through leaf blocks containing the matching hash value for the name
>> being looked up. Inside xfs_da_node_lookup_int(), it calls the
>> xfs_dir2_leafn_lookup_for_entry() for each leaf block.
>> xfs_dir2_leafn_lookup_for_entry() iterates through each matching
>> hash/offset pair doing a name comparison to find the matching
>> dirent.
>>
>> For CI mode, the state->extrablk retains the details of the block
>> that has the CI match so xfs_dir2_node_lookup() can return the
>> case-preserved name.
>>
>> The original implementation didn't retain the xfs_da_buf_t properly,
>> so the lookup was returning a bogus name to be stored in the dentry.
>>
>> In the case of unlink, the bad name was passed and in debug mode,
>> ASSERTed when it can't find the entry.
>
> Looks good to me, although I don't really like how much of a mess
> xfs_dir2_leafn_lookup_for_entry has become. No idea for a nice why
> to write it, though.

Yeah, bit of a problem with that. I can un-optimise the original code
by not keeping the current block between calls to
xfs_dir2_leafn_lookup_for_entry() and always freeing it. That will
clean it up a lot.

> Btw, any chance we could get some CI tests in xfsqa?  Looks like you
> have quite a few testcases that found issues so far.

Yes, they are coming.

Barry.