Re: Revalidate failure leads to unmount

[Oleg Drokin <green@linuxhacker.ru>]

On Dec 6, 2016, at 1:17 AM, Al Viro wrote:

> On Tue, Dec 06, 2016 at 12:45:11AM -0500, Oleg Drokin wrote:
> 
>> Well, certainly if d_splice_alias was working like that so that even non-directory
>> dentry would find an alias (not necessarily unhashed even) for that same inode and use that instead, that would make ll_splice_alias/ll_find_alias unnecessary.
>> 
>> We still retain the weird d_compare() that rejects otherwise perfectly valid aliases
>> if the lock guarding them is gone, triggering relookup (and necessiating the
>> above logic to pick up just rejected alias again now that we have the lock again).
> 
> Why not have ->d_revalidate() kick them, instead?  _IF_ we have a way to
> do unhash-and-trigger-lookup that way, do you really need those games with
> ->d_compare()?

Ok, so this does appear to work in mainline, but not in the well known vendor kernels,
where they still live in the past with d_invalidate returning non zero and
lookup_dcache() dutifuly ignoring the error code from revalidate.

Anyway, I can submit the patch doing away with ll_dcompare, but I still
need the kludge to always return 1 when revalidating mountpoints,
otherwise they would be constantly unmounted, I guess.

Is this something you'd like to carry along with the rest of (yet to be) patches
that only unmount stuff on lookup failure/different result as we discussed before,
or should I shoot this to Greg right away?

The patch pretty much amounts to this now:
diff --git a/drivers/staging/lustre/lustre/llite/dcache.c b/drivers/staging/lustre/lustre/llite/dcache.c
index 0e45d8f..f532167 100644
--- a/drivers/staging/lustre/lustre/llite/dcache.c
+++ b/drivers/staging/lustre/lustre/llite/dcache.c
@@ -69,38 +69,6 @@ static void ll_release(struct dentry *de)
 	call_rcu(&lld->lld_rcu_head, free_dentry_data);
 }
 
-/* Compare if two dentries are the same.  Don't match if the existing dentry
- * is marked invalid.  Returns 1 if different, 0 if the same.
- *
- * This avoids a race where ll_lookup_it() instantiates a dentry, but we get
- * an AST before calling d_revalidate_it().  The dentry still exists (marked
- * INVALID) so d_lookup() matches it, but we have no lock on it (so
- * lock_match() fails) and we spin around real_lookup().
- */
-static int ll_dcompare(const struct dentry *dentry,
-		       unsigned int len, const char *str,
-		       const struct qstr *name)
-{
-	if (len != name->len)
-		return 1;
-
-	if (memcmp(str, name->name, len))
-		return 1;
-
-	CDEBUG(D_DENTRY, "found name %.*s(%p) flags %#x refc %d\n",
-	       name->len, name->name, dentry, dentry->d_flags,
-	       d_count(dentry));
-
-	/* mountpoint is always valid */
-	if (d_mountpoint((struct dentry *)dentry))
-		return 0;
-
-	if (d_lustre_invalid(dentry))
-		return 1;
-
-	return 0;
-}
-
 /**
  * Called when last reference to a dentry is dropped and dcache wants to know
  * whether or not it should cache it:
@@ -255,6 +223,15 @@ static int ll_revalidate_dentry(struct dentry *dentry,
 {
 	struct inode *dir = d_inode(dentry->d_parent);
 
+	/* mountpoint is always valid */
+	if (d_mountpoint((struct dentry *)dentry))
+		return 1;
+
+	/* No lock? Bail out */
+	if (d_lustre_invalid(dentry))
+		return 0;
+
+
 	/* If this is intermediate component path lookup and we were able to get
 	 * to this dentry, then its lock has not been revoked and the
 	 * path component is valid.
@@ -303,5 +280,4 @@ const struct dentry_operations ll_d_ops = {
 	.d_revalidate = ll_revalidate_nd,
 	.d_release = ll_release,
 	.d_delete  = ll_ddelete,
-	.d_compare = ll_dcompare,
 };