On Thu, May 29, 2014 at 12:14:51PM -0700, Linus Torvalds wrote:
> Yeah, I don't think you can reproduce that, but I guess renaming
> directories into each other (two renames needed) could trigger an ABBA
> deadlock by changing the topological order of dentry/parent.
>
> I suspect there's no way in hell that tiny race will ever happen in
> practice, but let's not risk it.
>
> And your solution (to re-check after just taking the parent lock)
> seems sufficient and sane, since dentry_lock_for_move() will always
> take the parent lock(s) before we move a dentry.
>
> So that looks good to me.
BTW, how serious is the problem with __lockref_is_dead(&dentry->d_lockref)
with only ->d_parent->d_lock held? From my reading of lib/lockref.c it
should be safe - we only do lockref_mark_dead() with ->d_parent->d_lock
held, and it'll provide all the serialization and barriers we need.
If I'm right, we could get rid of DCACHE_DENTRY_KILLED completely and replace
checking for it with checking for negative ->d_lockref.count. There are two
places where we check for it; in shrink_dentry_list() we definitely can go
that way (we are holding ->d_lock there) and it simplifies the code nicely.
In d_walk(), though (in the bit that used to be try_to_ascend() we only hold
->d_parent->d_lock. It looks like that ought to be safe to replace
if (this_parent != child->d_parent ||
(child->d_flags & DCACHE_DENTRY_KILLED) ||
need_seqretry(&rename_lock, seq)) {
with
if (this_parent != child->d_parent ||
__lockref_is_dead(&child->d_lockref) ||
need_seqretry(&rename_lock, seq)) {
and remove DCACHE_DENTRY_KILLED completely...
The other user (in shrink_dentry_list()) simplifies to
if (dentry->d_lockref.count != 0) {
bool can_free = dentry->d_flags & DCACHE_MAY_FREE;
spin_unlock(&dentry->d_lock);
if (parent)
spin_unlock(&parent->d_lock);
if (can_free)
dentry_free(dentry);
continue;
}
taking care of both the DCACHE_DENTRY_KILLED case and simple "lazy dget"
one, and that one's definitely safe and worth doing.
Would be nice if we could switch d_walk() one as well and kill that flag off,
though...
Basically, we have
spin_lock(&A);
spin_lock(&R.lock);
V = 1;
lockref_mark_dead(&R);
...
as the only place where R goes dead and we want to replace
spin_lock(&A);
if (V)
...
with
spin_lock(&A);
if (__lockref_is_dead(&R))
...
Unless I'm missing something subtle in lockref.c, that should be safe...
Comments?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
