Messages in this thread Patch in this message | | | Date | Sun, 9 May 2004 15:27:20 -0700 | From | Andrew Morton <> | Subject | Re: dentry bloat. |
| |
viro@parcelfarce.linux.theplanet.co.uk 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |