Re: [PATCH 03/12] staging: lustre: llite: fix various issues with ll_splice_alias.

[Oleg Drokin <oleg.drokin@intel.com>]

On Jul 19, 2017, at 12:33 AM, NeilBrown wrote:

> On Tue, Jul 18 2017, Oleg Drokin wrote:
> 
>> Unfortunately this patch causes insta-crash on first stat call after mount.
>> Sorry, I cannot dig into this deeper right this moment, but I will a bit later.
> 
> V.strange.  The crash suggests that the lock, and hence the inode, is
> not initialized.  I cannot see how that might happen.
> though...
> 
>>> +		spin_lock(&lli->lli_lock);
>>> +		new = ll_find_invalid_alias(inode, de);
>>> +		if (!new)
>>> +			d_add(de, inode);
>>> +		spin_lock(&lli->lli_lock);
> 
> Had it not crashed, it would have deadlocked.  That second spin_lock()
> should be spin_unlock() :-( I don't *think* that would have caused this crash…

No, that's not it.
-	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);
+	de = d_splice_alias(inode, de);
+	if (!IS_ERR(de))
+		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;

This is likely it.
d_splice_alias would return NULL if there's no alias, after d_add of the de.
But ll_splice_alias callers expect either an ERR_PTR on error or dentry to use
otherwise.
So in ll_lookup_it_finish() we have:
        alias = ll_splice_alias(inode, *de);
        if (IS_ERR(alias)) {
                rc = PTR_ERR(alias);
                goto out;
        }
        *de = alias;
…
                if (md_revalidate_lock(ll_i2mdexp(parent), &parent_it, &fid,
                                       NULL)) {
===>>> whoops!          d_lustre_revalidate(*de);
                        ll_intent_release(&parent_it);
                }

>> I am adding Al that we discussed this code at some length and he found no problems
>> here, so I am a bit surprised by your findings.
> I'd be very happy to read Al's thoughts.

Some of the discussions were in lkml under subject of "More parallel
atomic_open/d_splice_alias fun with NFS and possibly more FSes"
in July 2016, in between nfs stuff.
There was more but I cannot readily find it.

>> Also the reason we reinvent the d_splice_alias is because we need to
>> splice not just directories, but also regular files.
> 
> I see that.  A key simplification I bring is that directories and
> non-directories can be handled separately.  d_splice_alias() does
> all we need for directories, and nothing useful for non-dirs.

I see.
I still need to  think some more about this whole thing.

Please see commit 99f1c013194e64d4b67d5d318148303b0e1585e1 for double d_add
of the same dentry fix.6