Re: [PATCH v7 1/4] spinlock: A new lockref structure for lockless update of refcount

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

On Sun, Sep 8, 2013 at 5:03 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Well...  unlazy_walk() is always followed by terminate_walk() very shortly,
> but there's a minor problem - terminate_walk() uses "are we in RCU
> mode?" for two things:
>         a) do we need to do path_put() here?
>         b) do we need to unlock?
> If you introduce the third case ("no need to do unlock and no need to
> do path_put()"), we'd better decide how to check for that case...

Actually, I decided to take advantage of those two cases instead, and
I have a patch that I think does the right thing. Basically, I start
off unlazy_walk() with just doing that lockref_get_not_dead() on the
parent dentry, and if that fails I just return an error in RCU mode
(so terminate_walk() does what it always used to do, and we haven't
done anything else to any refcounts).

Now, if the lockref_get_not_dead() succeeded, that means that we have
a reference on the nd->path.dentry, and we can now just do
"mntget(nd->path.mnt);". Ta-Daa! We now have everything done for the
non-RCU case for terminate_walk().

So after that point, we clear LOOKUP_RCU, and make the rule be that
any return (error or success) has to do unlock_rcu_walk(). And then
all the other refcounts are easy, we can just "dput(dentry);" after
that.

I haven't tested it yet, I was going to reboot into it just now. But
I'm attaching the patch here. Maybe I missed some detail, but it all
seems simpler.

Note that this patch requires the "lockref_get_not_dead()" cleanup at
the top of my current -git.

        Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 4618 bytes --]

 fs/namei.c | 102 +++++++++++++++++++++++++++++--------------------------------
 1 file changed, 49 insertions(+), 53 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index cc4bcfaa8624..56e4f4d537d0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -494,37 +494,6 @@ static inline void unlock_rcu_walk(void)
 	br_read_unlock(&vfsmount_lock);
 }
 
-/*
- * When we move over from the RCU domain to properly refcounted
- * long-lived dentries, we need to check the sequence numbers
- * we got before lookup very carefully.
- *
- * We cannot blindly increment a dentry refcount - even if it
- * is not locked - if it is zero, because it may have gone
- * through the final d_kill() logic already.
- *
- * So for a zero refcount, we need to get the spinlock (which is
- * safe even for a dead dentry because the de-allocation is
- * RCU-delayed), and check the sequence count under the lock.
- *
- * Once we have checked the sequence count, we know it is live,
- * and since we hold the spinlock it cannot die from under us.
- *
- * In contrast, if the reference count wasn't zero, we can just
- * increment the lockref without having to take the spinlock.
- * Even if the sequence number ends up being stale, we haven't
- * gone through the final dput() and killed the dentry yet.
- */
-static inline int d_rcu_to_refcount(struct dentry *dentry, seqcount_t *validate, unsigned seq)
-{
-	if (likely(lockref_get_not_dead(&dentry->d_lockref))) {
-		if (!read_seqcount_retry(validate, seq))
-				return 0;
-		dput(dentry);
-	}
-	return -ECHILD;
-}
-
 /**
  * unlazy_walk - try to switch to ref-walk mode.
  * @nd: nameidata pathwalk data
@@ -539,16 +508,29 @@ static int unlazy_walk(struct nameidata *nd, struct dentry *dentry)
 {
 	struct fs_struct *fs = current->fs;
 	struct dentry *parent = nd->path.dentry;
-	int want_root = 0;
 
 	BUG_ON(!(nd->flags & LOOKUP_RCU));
-	if (nd->root.mnt && !(nd->flags & LOOKUP_ROOT)) {
-		want_root = 1;
-		spin_lock(&fs->lock);
-		if (nd->root.mnt != fs->root.mnt ||
-				nd->root.dentry != fs->root.dentry)
-			goto err_root;
-	}
+
+	/*
+	 * Get a reference to the parent first: we're
+	 * going to make "path_put(nd->path)" valid in
+	 * non-RCU context for "terminate_walk()".
+	 *
+	 * If this doesn't work, return immediately with
+	 * RCU walking still active (and then we will do
+	 * the RCU walk cleanup in terminate_walk()).
+	 */
+	if (!lockref_get_not_dead(&parent->d_lockref))
+		return -ECHILD;
+
+	/*
+	 * After the mntget(), we terminate_walk() will do
+	 * the right thing for non-RCU mode, and all our
+	 * subsequent exit cases should unlock_rcu_walk()
+	 * before returning.
+	 */
+	mntget(nd->path.mnt);
+	nd->flags &= ~LOOKUP_RCU;
 
 	/*
 	 * For a negative lookup, the lookup sequence point is the parents
@@ -562,30 +544,39 @@ static int unlazy_walk(struct nameidata *nd, struct dentry *dentry)
 	 * be valid if the child sequence number is still valid.
 	 */
 	if (!dentry) {
-		if (d_rcu_to_refcount(parent, &parent->d_seq, nd->seq) < 0)
-			goto err_root;
+		if (read_seqcount_retry(&parent->d_seq, nd->seq))
+			goto out;
 		BUG_ON(nd->inode != parent->d_inode);
 	} else {
-		if (d_rcu_to_refcount(dentry, &dentry->d_seq, nd->seq) < 0)
-			goto err_root;
-		if (d_rcu_to_refcount(parent, &dentry->d_seq, nd->seq) < 0)
-			goto err_parent;
+		if (!lockref_get_not_dead(&dentry->d_lockref))
+			goto out;
+		if (read_seqcount_retry(&dentry->d_seq, nd->seq))
+			goto drop_dentry;
 	}
-	if (want_root) {
+
+	/*
+	 * Sequence counts matched. Now make sure that the root is
+	 * still valid and get it if required.
+	 */
+	if (nd->root.mnt && !(nd->flags & LOOKUP_ROOT)) {
+		spin_lock(&fs->lock);
+		if (nd->root.mnt != fs->root.mnt || nd->root.dentry != fs->root.dentry)
+			goto unlock_and_drop_dentry;
 		path_get(&nd->root);
 		spin_unlock(&fs->lock);
 	}
-	mntget(nd->path.mnt);
 
 	unlock_rcu_walk();
-	nd->flags &= ~LOOKUP_RCU;
 	return 0;
 
-err_parent:
+unlock_and_drop_dentry:
+	spin_unlock(&fs->lock);
+drop_dentry:
+	unlock_rcu_walk();
 	dput(dentry);
-err_root:
-	if (want_root)
-		spin_unlock(&fs->lock);
+	return -ECHILD;
+out:
+	unlock_rcu_walk();
 	return -ECHILD;
 }
 
@@ -614,10 +605,15 @@ static int complete_walk(struct nameidata *nd)
 		if (!(nd->flags & LOOKUP_ROOT))
 			nd->root.mnt = NULL;
 
-		if (d_rcu_to_refcount(dentry, &dentry->d_seq, nd->seq) < 0) {
+		if (unlikely(!lockref_get_not_dead(&dentry->d_lockref))) {
 			unlock_rcu_walk();
 			return -ECHILD;
 		}
+		if (read_seqcount_retry(&dentry->d_seq, nd->seq)) {
+			unlock_rcu_walk();
+			dput(dentry);
+			return -ECHILD;
+		}
 		mntget(nd->path.mnt);
 		unlock_rcu_walk();
 	}