Re: races in ll_splice_alias()

[Al Viro <viro@ZenIV.linux.org.uk>]

On Tue, Mar 08, 2016 at 11:18:09PM +0000, Drokin, Oleg wrote:
> Rename on server cannot get us to see the same directory in two places, or what's
> the scenario?
> In Lustre:
> thread1: lookup a directory on the server.
>          get a lock back
>          instantiate a dentry (guarded by the lock)
>          make a lock revocable (ll_lookup_finish_locks() in ll_lookup_it())
> thread2: rename the directory moving it somewhere else
>          server attempts to revoke the lock from thread1
>          node that runs thread1 drops the lock and marks all dentries for that
>               inode invalid
>          server completes rename and releases the lock it holds
> thread3: lookup the directory under new name on the server
>          this can only return from server when the rename has completed and the
>          dentry from thread1 marked invalid.

thread2 might run on another client; might or might not make any difference,
but in any case server going nuts shouldn't corrupt data structures on client...

> Ok, let me try my hand at that. Hopefully whatever complications are there would
> show themselves right away too.
> 
> > would always either inserted inode reference into a new dentry or dropped it.
> > I'm still trying to trace what does iput() in case of error in your current
> > code; I understand the one in do_statahead_enter(), but what does it in
> > ll_lookup_it_finish()?
> 
> You mean when ll_d_init() fails in ll_splice_alias()?
> Hm… It appears that we are indeed leaking the inode in that case, thanks.
> I'll try to address that too.
> Hm, in fact this was almost noticed, but I guess nobody retested it after
> fixing the initial crash we had with 7486bc06ab2c46d6957f0211d09bc549aaf9cc87

If that's the case, I'd try this (on top of the patch from upthread):

diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
index da5f443..bcc9841 100644
--- a/drivers/staging/lustre/lustre/llite/namei.c
+++ b/drivers/staging/lustre/lustre/llite/namei.c
@@ -320,81 +320,37 @@ void ll_i2gids(__u32 *suppgids, struct inode *i1, struct inode *i2)
 }
 
 /*
- * try to reuse three types of dentry:
- * 1. unhashed alias, this one is unhashed by d_invalidate (but it may be valid
- *    by concurrent .revalidate).
- * 2. INVALID alias (common case for no valid ldlm lock held, but this flag may
- *    be cleared by others calling d_lustre_revalidate).
- * 3. DISCONNECTED alias.
- */
-static struct dentry *ll_find_alias(struct inode *inode, struct dentry *dentry)
-{
-	struct dentry *alias, *discon_alias, *invalid_alias;
-
-	if (hlist_empty(&inode->i_dentry))
-		return NULL;
-
-	discon_alias = invalid_alias = NULL;
-
-	ll_lock_dcache(inode);
-	hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
-		LASSERT(alias != dentry);
-
-		spin_lock(&alias->d_lock);
-		if (alias->d_flags & DCACHE_DISCONNECTED)
-			/* LASSERT(last_discon == NULL); LU-405, bz 20055 */
-			discon_alias = alias;
-		else if (alias->d_parent == dentry->d_parent	     &&
-			 alias->d_name.hash == dentry->d_name.hash       &&
-			 alias->d_name.len == dentry->d_name.len	 &&
-			 memcmp(alias->d_name.name, dentry->d_name.name,
-				dentry->d_name.len) == 0)
-			invalid_alias = alias;
-		spin_unlock(&alias->d_lock);
-
-		if (invalid_alias)
-			break;
-	}
-	alias = invalid_alias ?: discon_alias ?: NULL;
-	if (alias) {
-		spin_lock(&alias->d_lock);
-		dget_dlock(alias);
-		spin_unlock(&alias->d_lock);
-	}
-	ll_unlock_dcache(inode);
-
-	return alias;
-}
-
-/*
  * Similar to d_splice_alias(), but lustre treats invalid alias
  * similar to DCACHE_DISCONNECTED, and tries to use it anyway.
  */
 struct dentry *ll_splice_alias(struct inode *inode, struct dentry *de)
 {
-	struct dentry *new;
+	struct dentry *alias = NULL;
 	int rc;
 
 	if (inode) {
-		new = ll_find_alias(inode, de);
-		if (new) {
-			rc = ll_d_init(new);
-			if (rc < 0) {
-				dput(new);
-				return ERR_PTR(rc);
-			}
-			d_move(new, de);
+		alias = d_exact_alias(de, inode);
+		if (alias)
+			iput(inode);
+	}
+
+	if (!alias) {
+		rc = ll_d_init(de);
+		if (rc < 0) {
 			iput(inode);
-			CDEBUG(D_DENTRY,
-			       "Reuse dentry %p inode %p refc %d flags %#x\n",
-			      new, d_inode(new), d_count(new), new->d_flags);
-			return new;
+			return ERR_PTR(rc);
 		}
+		alias = d_splice_alias(inode, de);
+		if (IS_ERR(alias))
+			return alias;
+	}
+
+	if (alias) {
+		CDEBUG(D_DENTRY,
+		       "Reuse dentry %p inode %p refc %d flags %#x\n",
+		      alias, d_inode(alias), d_count(alias), alias->d_flags);
+		return alias;
 	}
-	rc = ll_d_init(de);
-	if (rc < 0)
-		return ERR_PTR(rc);
-	d_add(de, inode);
 	CDEBUG(D_DENTRY, "Add dentry %p inode %p refc %d flags %#x\n",
 	       de, d_inode(de), d_count(de), de->d_flags);
 	return de;
diff --git a/drivers/staging/lustre/lustre/llite/statahead.c b/drivers/staging/lustre/lustre/llite/statahead.c
index 88ffd8e..6c64de0 100644
--- a/drivers/staging/lustre/lustre/llite/statahead.c
+++ b/drivers/staging/lustre/lustre/llite/statahead.c
@@ -1576,6 +1576,7 @@ int do_statahead_enter(struct inode *dir, struct dentry **dentryp,
 					alias = ll_splice_alias(inode,
 								   *dentryp);
 					if (IS_ERR(alias)) {
+						entry->se_inode = NULL;
 						ll_sai_unplug(sai, entry);
 						return PTR_ERR(alias);
 					}