Re: dentry bloat.

[Andrew Morton]

viro@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx wrote:
>
> On Sun, May 09, 2004 at 09:03:16PM +0530, Dipankar Sarma wrote:
>  
> > Actually, what may happen is that since the dentries are added
> > in the front, a double move like that would result in hash chain
> > traversal looping. Timing dependent and unlikely, but d_move_count
> > avoided that theoritical possibility. It is not about skipping
> > dentries which is safe because a miss would result in a real_lookup()
> 
> Not really.  A miss could result in getting another dentry allocated
> for the same e.g. directory, which is *NOT* harmless at all.

The d_bucket logic does look a bit odd.

		dentry = hlist_entry(node, struct dentry, d_hash);

		/* if lookup ends up in a different bucket 
		 * due to concurrent rename, fail it
		 */
		if (unlikely(dentry->d_bucket != head))
			break;

		/*
		 * We must take a snapshot of d_move_count followed by
		 * read memory barrier before any search key comparison 
		 */
		move_count = dentry->d_move_count;

There is a window between the d_bucket test and sampling of d_move_count. 
What happens if the dentry gets moved around in there?

Anyway, regardless of that, it is more efficient to test d_bucket _after_
performing the hash comparison.  And it seems saner to perform the d_bucket
check when things are pinned down by d_lock.



 25-akpm/fs/dcache.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff -puN fs/dcache.c~dentry-d_bucket-fix fs/dcache.c
--- 25/fs/dcache.c~dentry-d_bucket-fix	2004-05-09 14:06:51.816554824 -0700
+++ 25-akpm/fs/dcache.c	2004-05-09 14:07:41.932935976 -0700
@@ -975,12 +975,6 @@ struct dentry * __d_lookup(struct dentry
 		smp_read_barrier_depends();
 		dentry = hlist_entry(node, struct dentry, d_hash);
 
-		/* if lookup ends up in a different bucket 
-		 * due to concurrent rename, fail it
-		 */
-		if (unlikely(dentry->d_bucket != head))
-			break;
-
 		smp_rmb();
 
 		if (dentry->d_name.hash != hash)
@@ -991,6 +985,13 @@ struct dentry * __d_lookup(struct dentry
 		spin_lock(&dentry->d_lock);
 
 		/*
+		 * If lookup ends up in a different bucket due to concurrent
+		 * rename, fail it
+		 */
+		if (unlikely(dentry->d_bucket != head))
+			goto terminate;
+
+		/*
 		 * Recheck the dentry after taking the lock - d_move may have
 		 * changed things.  Don't bother checking the hash because we're
 		 * about to compare the whole name anyway.
@@ -1014,6 +1015,7 @@ struct dentry * __d_lookup(struct dentry
 			atomic_inc(&dentry->d_count);
 			found = dentry;
 		}
+terminate:
 		spin_unlock(&dentry->d_lock);
 		break;
 next:

_

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/