lkml.org 
[lkml]   [2003]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] First casuality of hlist poisoning in 2.5.70

On Wed, 11 Jun 2003, Trond Myklebust wrote:
>
> This patch removes the Oops that occurs when either the source or
> the target of a d_move() operation is unhashed. It is currently
> triggered by the NFS sillyrename code.

Btw, thinking more about this, the patch can't be right.

> + if (!hlist_unhashed(&dentry->d_hash))
> + hlist_del_rcu(&dentry->d_hash);

This leaves the d_hash pointers as crapola.

> + if (!hlist_unhashed(&target->d_hash)) {
> + hlist_add_head_rcu(&dentry->d_hash, target->d_bucket);
> + dentry->d_vfs_flags &= ~DCACHE_UNHASHED;
> + } else
> + dentry->d_vfs_flags |= DCACHE_UNHASHED;

And this will fix it only if target is hashed.

So either the patch depends on the target being hashed (and if so, it just
shouldn't have the test, or have a BUG_ON() instead), or the patch is
buggy.

I have this suspicion that the logical patch would be something like this:

if (hlist_unhashed(&target->d_hash)) {
/* Unhash the dentry too */
__d_drop(dentry);
} else {
/* Rehash the dentry onto the same hash as the target */
hlist_del_rcu(&dentry->d_hash);
hlist_add_head_rcu(&dentry->d_hash, target->d_bucket);
dentry->d_vfs_flags &= ~DCACHE_UNHASHED;
}

At least this would make slightly more sense than the patch.

HOWEVER, I actually suspect that the target really _cannot_ be unhashed,
and that the test makes no sense, and the sequence should just be

/* Rehash the dentry onto the same hash as the target */
hlist_del_rcu(&dentry->d_hash);
hlist_add_head_rcu(&dentry->d_hash, target->d_bucket);
dentry->d_vfs_flags &= ~DCACHE_UNHASHED;

this is actually identical to what we have right now, except for one small
detail: the current 2.5.x tree does not clear the DCACHE_UNHASHED bit in
d_move().

Also, if a unhashed dentry really is ok, then I think the test for

/* Move the dentry to the target hash queue, if on different bucket */
if (dentry->d_bucket != target->d_bucket) {
...

should really be something like

if (!d_hashed(dentry) || dentry->d_bucket != target->d_bucket) {
...

because otherwise the dentry may well share the same bucket as the target,
but if it wasn't hashed before, it won't be hashed after either.

But I suspect that neither dentry nor target should really ever be
unhashed by the time we call d_move(). That's reinforced by the fact that
it looks like a unhashed dentry in d_move() would have been a silent bug
previously - staying unhashed if it just shared the bucket.

Al, I'll be really happy having you go over this code too. And whatever we
decide is right (enforcing hashedness or whatever), we should assert it,
because clearly d_move() has been a bit too subtle for us so far.

Linus

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

\
 
 \ /
  Last update: 2005-03-22 13:36    [W:0.061 / U:2.540 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site