[4/4] fs/dcache: Avoid the try_lock loops in dentry_kill()
diff mbox

Message ID 20180216150933.971-5-john.ogness@linutronix.de
State New
Headers show

Commit Message

John Ogness Feb. 16, 2018, 3:09 p.m. UTC
dentry_kill() holds dentry->d_lock and needs to acquire both
dentry->d_inode->i_lock and dentry->d_parent->d_lock. This cannot be
done with spin_lock() operations because it's the reverse of the
regular lock order. To avoid ABBA deadlocks it is done with two
trylock loops.

Trylock loops are problematic in two scenarios:

  1) PREEMPT_RT converts spinlocks to 'sleeping' spinlocks, which are
     preemptible. As a consequence the i_lock holder can be preempted
     by a higher priority task. If that task executes the trylock loop
     it will do so forever and live lock.

  2) In virtual machines trylock loops are problematic as well. The
     VCPU on which the i_lock holder runs can be scheduled out and a
     task on a different VCPU can loop for a whole time slice. In the
     worst case this can lead to starvation. Commits 47be61845c77
     ("fs/dcache.c: avoid soft-lockup in dput()") and 046b961b45f9
     ("shrink_dentry_list(): take parent's d_lock earlier") are
     addressing exactly those symptoms.

Avoid the trylock loops by using dentry_lock_inode() and lock_parent()
which take the locks in the appropriate order. As both functions drop
dentry->lock briefly, this requires rechecking of the dentry content
as it might have changed after dropping the lock.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 fs/dcache.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 60 insertions(+), 16 deletions(-)

Comments

Linus Torvalds Feb. 16, 2018, 6:03 p.m. UTC | #1
On Fri, Feb 16, 2018 at 7:09 AM, John Ogness <john.ogness@linutronix.de> wrote:
> dentry_kill() holds dentry->d_lock and needs to acquire both
> dentry->d_inode->i_lock and dentry->d_parent->d_lock. This cannot be
> done with spin_lock() operations because it's the reverse of the
> regular lock order. To avoid ABBA deadlocks it is done with two
> trylock loops.
>
> Trylock loops are problematic in two scenarios:

I don't mind this patch series per se (although I would really like Al
to ack it), but this particular patch I hate.

Why?

> Avoid the trylock loops by using dentry_lock_inode() and lock_parent()
> which take the locks in the appropriate order. As both functions drop
> dentry->lock briefly, this requires rechecking of the dentry content
> as it might have changed after dropping the lock.

I think the trylock should be done first, and then you don't need that
recheck for the common case.

I realize that the recheck itself isn't expensive, but it's mostly
about the code flow and the comment:

> +                * Recheck refcount as it might have been incremented while
> +                * d_lock was dropped.

the thing is, 99.9% of the time the d_lock wasn't dropped, so that
"while d_lock was dropped" comment is misleading.

Re-organizing it to do the trylock fastpath explicitly here and not
bothering with the re-check etc crid for the common case is the rioght
thing to do.

And the old code was actually organized exactly that way, with a

        if (inode && unlikely(!spin_trylock(&inode->i_lock)))
                goto failed;

at the top.

But instead of having that unlikely "failed" case do the complex
thing, you made the *normal* case do the complex thing.

So NAK on this.

It should be fairly trivial to fix, and make the "failed" thing do it right.

             Linus
John Ogness Feb. 16, 2018, 10:32 p.m. UTC | #2
On 2018-02-16, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Fri, Feb 16, 2018 at 7:09 AM, John Ogness <john.ogness@linutronix.de> wrote:
>> dentry_kill() holds dentry->d_lock and needs to acquire both
>> dentry->d_inode->i_lock and dentry->d_parent->d_lock. This cannot be
>> done with spin_lock() operations because it's the reverse of the
>> regular lock order. To avoid ABBA deadlocks it is done with two
>> trylock loops.
>>
>> Trylock loops are problematic in two scenarios:
>
> I don't mind this patch series per se (although I would really like Al
> to ack it), but this particular patch I hate.
>
> Why?
>
>> Avoid the trylock loops by using dentry_lock_inode() and lock_parent()
>> which take the locks in the appropriate order. As both functions drop
>> dentry->lock briefly, this requires rechecking of the dentry content
>> as it might have changed after dropping the lock.
>
> I think the trylock should be done first, and then you don't need that
> recheck for the common case.
>
> I realize that the recheck itself isn't expensive, but it's mostly
> about the code flow and the comment:
>
>> +                * Recheck refcount as it might have been incremented while
>> +                * d_lock was dropped.
>
> the thing is, 99.9% of the time the d_lock wasn't dropped, so that
> "while d_lock was dropped" comment is misleading.
>
> Re-organizing it to do the trylock fastpath explicitly here and not
> bothering with the re-check etc crid for the common case is the rioght
> thing to do.
>
> And the old code was actually organized exactly that way, with a
>
>         if (inode && unlikely(!spin_trylock(&inode->i_lock)))
>                 goto failed;
>
> at the top.
>
> But instead of having that unlikely "failed" case do the complex
> thing, you made the *normal* case do the complex thing.
>
> So NAK on this.

lock_parent() already has the problem you are referring to. Callers are
required to recheck the dentry contents and check the returned parent
because they do not know if the trylock succeeded. See
d_prune_aliases(), for example.

Would you like my v2 to fixup lock_parent() semantics to address your
concerns there as well?

John Ogness
Linus Torvalds Feb. 16, 2018, 10:42 p.m. UTC | #3
On Fri, Feb 16, 2018 at 2:32 PM, John Ogness <john.ogness@linutronix.de> wrote:
>
> lock_parent() already has the problem you are referring to. Callers are
> required to recheck the dentry contents and check the returned parent
> because they do not know if the trylock succeeded. See
> d_prune_aliases(), for example.

What are you talking about?

lock_parent() does the nice "spin_trylock succeeded" special case.

Yes, it will then do the "unlock dentry, do the parent first, then
re-check" too, and callers may need to worry about it.

But that's not what I'm complaining about in your patch. You remove
the simple case, and make dentry_kill() do the "recheck in case I
dropped" every single time.

It's the "turn a simple case into a complex case" that I absolutely detest.

The fact that there are _other_ complex cases doesn't make it any
better. The whole "but Bobby does it too" thing is not a defense.
Would you jump off a bridge just because your friend did it?

               Linus
John Ogness Feb. 16, 2018, 11:05 p.m. UTC | #4
On 2018-02-16, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>> lock_parent() already has the problem you are referring to. Callers
>> are required to recheck the dentry contents and check the returned
>> parent because they do not know if the trylock succeeded. See
>> d_prune_aliases(), for example.
>
> What are you talking about?
>
> lock_parent() does the nice "spin_trylock succeeded" special case.
>
> Yes, it will then do the "unlock dentry, do the parent first, then
> re-check" too, and callers may need to worry about it.
>
> But that's not what I'm complaining about in your patch. You remove
> the simple case, and make dentry_kill() do the "recheck in case I
> dropped" every single time.

dentry_lock_inode() uses the same semantics as lock_parent(). The caller
does not know if the trylock succeeded. Any caller using lock_parent()
must "recheck in case I dropped", just as with dentry_lock_inode(). This
is what you have pointed out.

> The fact that there are _other_ complex cases doesn't make it any
> better. The whole "but Bobby does it too" thing is not a defense.
> Would you jump off a bridge just because your friend did it?

dentry_kill() calls both dentry_lock_inode() and lock_parent() in the
common case. So by changing the semantics of lock_parent(), I am
removing two "recheck in case I dropped" in the common case rather than
just the one you pointed out.

John Ogness
Linus Torvalds Feb. 16, 2018, 11:31 p.m. UTC | #5
On Fri, Feb 16, 2018 at 3:05 PM, John Ogness <john.ogness@linutronix.de> wrote:
>
> dentry_kill() calls both dentry_lock_inode() and lock_parent() in the
> common case. So by changing the semantics of lock_parent(), I am
> removing two "recheck in case I dropped" in the common case rather than
> just the one you pointed out.

Ok, that would be lovely, but doesn't that end up being a nasty patch?
You can't just move the trylock into the caller, since then you need
to move all the other stuff too?

Or were you planning on splitting lock_parent() into two, for the
"fast case vs compex case"?

Or maybe I'm entirely missing something and we're miscommunicating.

I'm actually not so much worried about the cost of re-checking (the
real cost tends to be the locked cycle itself) as I am about the code
looking understandable. Your d_delete() patch didn't make me  go "that
looks more complicated", probably partl ybecause of the nice helper
function.

So it may be that my dislike of the "re-check after possibly dropping
the lock" is not really about the re-checking, but about just how it
made that function look much more complicated.

             Linus
John Ogness Feb. 16, 2018, 11:49 p.m. UTC | #6
On 2018-02-17, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>> dentry_kill() calls both dentry_lock_inode() and lock_parent() in the
>> common case. So by changing the semantics of lock_parent(), I am
>> removing two "recheck in case I dropped" in the common case rather
>> than just the one you pointed out.
>
> Ok, that would be lovely, but doesn't that end up being a nasty patch?

After reading your initial feedback my idea was to change both
lock_parent() and dentry_lock_inode() to not only communicate _if_ the
lock was successful, but also if d_lock was dropped in the process. (For
example, with a tristate rather than boolean return value.) Then callers
would know if they needed to recheck the dentry contents.

> So it may be that my dislike of the "re-check after possibly dropping
> the lock" is not really about the re-checking, but about just how it
> made that function look much more complicated.

I understand what you are saying and I appreciate the comments. I will
code up some variations for myself and try to pick the one that is the
least complicated for my v2.

John Ogness
Linus Torvalds Feb. 17, 2018, 12:06 a.m. UTC | #7
On Fri, Feb 16, 2018 at 3:49 PM, John Ogness <john.ogness@linutronix.de> wrote:
>
> After reading your initial feedback my idea was to change both
> lock_parent() and dentry_lock_inode() to not only communicate _if_ the
> lock was successful, but also if d_lock was dropped in the process. (For
> example, with a tristate rather than boolean return value.) Then callers
> would know if they needed to recheck the dentry contents.

So I think that would work well for your dentry_lock_inode() helper,
and might indeed solve my reaction to your dentry_kill() patch.

I suspect it doesn't work for lock_parent(), because that has to also
return the parent itself. So you'd have to add another way to say
"didn't need to drop dentry lock". I suspect it gets ugly real fast.

But yes, making dentry_lock_inode() return 0/1/2 for "fail/fast/slow"
(or whatever) sounds doable. And then your dentry_kill() patch can use
a "switch ()" to handle the cases, and the whole "need to revalidate"
might become pretty clear and clean.

I'd suggest you ignore lock_parent() for now. Unless you come up with
something clever.

        Linus
Al Viro Feb. 22, 2018, 5:40 a.m. UTC | #8
On Fri, Feb 16, 2018 at 10:03:15AM -0800, Linus Torvalds wrote:
> On Fri, Feb 16, 2018 at 7:09 AM, John Ogness <john.ogness@linutronix.de> wrote:
> > dentry_kill() holds dentry->d_lock and needs to acquire both
> > dentry->d_inode->i_lock and dentry->d_parent->d_lock. This cannot be
> > done with spin_lock() operations because it's the reverse of the
> > regular lock order. To avoid ABBA deadlocks it is done with two
> > trylock loops.
> >
> > Trylock loops are problematic in two scenarios:
> 
> I don't mind this patch series per se (although I would really like Al
> to ack it), but this particular patch I hate.

I'm not happy about the previous patch either.  Why do we need the users
of that thing to deal with retries?  And I'm not even sure we want to
bother with retries on inode change inside dentry_kill() itself - just
unlock, return dentry and let the caller handle that.  Callers *must*
handle "need to drop another one" anyway, so...

Patch
diff mbox

diff --git a/fs/dcache.c b/fs/dcache.c
index 2cd252f88c5d..b557679c14c9 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -673,27 +673,73 @@  static bool dentry_lock_inode(struct dentry *dentry)
 static struct dentry *dentry_kill(struct dentry *dentry)
 	__releases(dentry->d_lock)
 {
-	struct inode *inode = dentry->d_inode;
-	struct dentry *parent = NULL;
+	struct dentry *parent;
+	struct inode *inode;
+
+again:
+	parent = NULL;
+	inode = dentry->d_inode;
+	if (inode) {
+		/*
+		 * Lock the inode. Might drop dentry->d_lock temporarily
+		 * which allows inode to change. Start over if that happens.
+		 */
+		if (!dentry_lock_inode(dentry))
+			goto again;
 
-	if (inode && unlikely(!spin_trylock(&inode->i_lock)))
-		goto failed;
+		/*
+		 * Recheck refcount as it might have been incremented while
+		 * d_lock was dropped.
+		 */
+		if (unlikely(dentry->d_lockref.count != 1))
+			goto drop_ref;
+	}
 
-	if (!IS_ROOT(dentry)) {
-		parent = dentry->d_parent;
-		if (unlikely(!spin_trylock(&parent->d_lock))) {
-			if (inode)
-				spin_unlock(&inode->i_lock);
-			goto failed;
-		}
+	parent = lock_parent(dentry);
+
+	/*
+	 * Check refcount because it might have changed
+	 * while d_lock was dropped.
+	 */
+	if (unlikely(dentry->d_lockref.count != 1))
+		goto drop_ref;
+
+	/*
+	 * The inode may have changed in the window where the
+	 * dentry was unlocked. If that happens it is necessary
+	 * to start over because the wrong inode lock is held.
+	 */
+	if (unlikely(inode != dentry->d_inode)) {
+		if (parent)
+			spin_unlock(&parent->d_lock);
+		if (inode)
+			spin_unlock(&inode->i_lock);
+		goto again;
 	}
 
 	__dentry_kill(dentry);
 	return parent;
 
-failed:
+drop_ref:
+	/*
+	 * If refcount is > 1 it was incremented while dentry->d_lock was
+	 * dropped. Just decrement the refcount, unlock and tell the caller
+	 * to stop the directory walk.
+	 *
+	 * For paranoia reasons check whether the refcount is < 1. If so,
+	 * report the detection and avoid the decrement which would just
+	 * cause a problem in some other place. The warning might be
+	 * helpful to decode the root cause of the refcounting bug.
+	 */
+	if (!WARN_ON(dentry->d_lockref.count < 1))
+		dentry->d_lockref.count--;
+
 	spin_unlock(&dentry->d_lock);
-	return dentry; /* try again with same dentry */
+	if (parent)
+		spin_unlock(&parent->d_lock);
+	if (inode)
+		spin_unlock(&inode->i_lock);
+	return NULL;
 }
 
 /*
@@ -865,10 +911,8 @@  void dput(struct dentry *dentry)
 
 kill_it:
 	dentry = dentry_kill(dentry);
-	if (dentry) {
-		cond_resched();
+	if (dentry)
 		goto repeat;
-	}
 }
 EXPORT_SYMBOL(dput);