From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Torvalds Subject: Re: [PATCH v7 1/4] spinlock: A new lockref structure for lockless update of refcount Date: Sun, 8 Sep 2013 14:45:32 -0700 Message-ID: References: <1375758759-29629-1-git-send-email-Waiman.Long@hp.com> <1375758759-29629-2-git-send-email-Waiman.Long@hp.com> <1377751465.4028.20.camel@pasglop> <20130829070012.GC27322@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: Benjamin Herrenschmidt , Waiman Long , Alexander Viro , Jeff Layton , Miklos Szeredi , Ingo Molnar , Thomas Gleixner , linux-fsdevel , Linux Kernel Mailing List , Peter Zijlstra , Steven Rostedt , Andi Kleen , "Chandramouleeswaran, Aswin" , "Norton, Scott J" To: Ingo Molnar Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org Al, this is mainly a rambling question for you, but others left on the Cc just FYI.. On Thu, Aug 29, 2013 at 4:42 PM, Linus Torvalds wrote: > > So I fixed the VFS layer instead. With dput() and friends using > lockrefs, the only thing remaining in the hot RCU dentry lookup path > was the nasty __d_rcu_to_refcount() thing in complete_walk(). I > rewrote that to locklessly increment the refcount when it was nonzero, > and get the lock if it was zero, and that all seems to work fine. Today I realized that my new simplified d_rcu_to_refcount() was fundamentally buggy in a really annoying way. I had worried mainly about the races with the dentry being invalidated, and how we must not touch the dentry reference count if it might be dead, because once the dentry has been killed, it cannot be killed again. My lockref_get_or_lock() avoided all that by making sure that it always got the reference on a dentry that was still live, and this everything was fine. If it turns out that the sequence count says that the lookup wasn't valid, we could just dput() that dentry, and everything was fine. And you know what? That's even true. It's true from a dentry standpoint. But today I was - for totally unrelated reasons, just as a result of looking at performance profiles - looking at dput(), and realized that dput() really *can* sleep. Not for any actual dentry reasons, but because the inode associated with the dentry is being released. And the reason I noticed this from the performance profile was that the dput code did if (dentry->d_lockref.count == 1) might_sleep(); where reading that lockref count showed up in the profile. But that test never triggered any of my thinking, and it's racy anyway, and the condition we care about is another race, which means that it not only didn't trigger my thinking, it doesn't trigger in any normal testing either. The fact is, dput() can always sleep, regardless of count, because of the race (ok, the exception being that you actually have multiple references to the same dentry _yourself_, but who does that? Nobody). Anyway, the reason the new d_rcu_to_refcount() is buggy isn't because it does bad things to dentries - the dentry reference counts and lifetimes are perfectly fine. No, the reason it does bad things is that it does an otherwise perfectly fine dput() in a context where it definitely is _not_ fine to sleep. We are in RCU lookup, so we are in a RCU-locked region, we hold the percpu vfsmount_lock spinlock, and in fact in unlazy_walk() we also potentially hold the "fs->lock" spinlock. Ugh, ugh, ugh. I really like the much simplified d_rcu_to_refcount() conceptually, though. So my current plan is to keep it, but to just delay the "dput()" it does until after we've done the "unlock_rcu_walk()". "complete_walk()" is actually quite simple to fix, because it does the unlock_rcu_walk() itself - so it's fairly trivial to just move the dput() to be after it. In preparation for this, I already ended up committing my "dead lockref" dentry code, because that simplifies d_rcu_to_refcount() to the degree that splitting the code up into the callers and then moving the dput() looks like the right thing to do. The problem I see right now is unlazy_walk(). It does *not* do the unlock_rcu_walk() itself for the error case (and it's the error case that needs this), but instead just returns -ECHILD and expects the caller to do it. Even then it happens fairly obscurely in "terminate_walk()". So Al, any ideas? I currently have two: - always do that unlock_rcu_walk() in unlazy_walk(), and exit RCU mode even for the error case (similar to how complete_walk() does it). That makes solving this for unlazy_walk() as straightforward as it is for complete_walk(), and this is, I think, the right thing to do. - add a couple of "to be freed" dentry pointers to the 'nd' and let terminate_walk() do the dput(). This leaves the unlazy_walk() semantics alone. The second one is hacky, but it doesn't change the semantics of unlzay_walk(). I think the current semantics (drop out of RCU mode on success, but not on failure) are nasty, but I wonder if there is some really subtle reason for it. So the hacky second solution has got that whole "no change to odd/subtle semantics" thing going for it.. Comments? Linus