Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

[Oleg Drokin]

Hello!

On Jun 17, 2016, at 12:29 AM, Al Viro wrote:

> On Fri, Jun 17, 2016 at 12:09:19AM -0400, Oleg Drokin wrote:
> 
>>    So they both do d_drop(), the dentry is now unhashed, and they both
>>    dive into nfs_lookup().
>>    There eventually they both call
>> 
>> 	res = d_splice_alias(inode, dentry);
>> 
>>   And so the first lucky one continues on it's merry way with a hashed dentry,
>>   but the other less lucky one ends up calling into d_splice_alias() with
>>   dentry that's already hashed and hits the very familiar assertion.
>> 
>>   I took a brief look into ceph and it looks like a very similar thing
>>   might happen there with handle_reply() for two parallel replies calling into
>>   ceph_fill_trace() and then splice_alias()->d_splice_alias(), since the
>>   unhashed check it does is not under any locks, it's unsafe, so the problem
>>   might be more generic than just NFS too.
>> 
>>   So I wonder how to best fix this? Holding some sort of dentry lock across a call
>>   into atomic_open in VFS? We cannot just make d_splice_alias() callers call with
>>   inode->i_lock held because dentry might be negative.
> 
> Oh, lovely...  So basically the problem is that we violate the "no lookups on
> the same name in parallel" rule on those fallbacks from foo_atomic_open() to
> foo_lookup().  The thing is, a lot of ->atomic_open() instances have such
> fallbacks and I wonder if that's a sign that we need to lift some of that
> to fs/namei.c...
> 
> Hell knows; alternative is to have that d_drop() followed by d_alloc_parallel()
> and feeding that dentry to lookup.  I'll play with that a bit and see what's
> better; hopefully I'll have something by tomorrow.

Sorry to nag you about this, but did any of those pan out?

d_alloc_parallel() sounds like a bit too heavy there, esp. considering we came in with
a dentry already (though a potentially shared one, I understand).
Would not it be better to try and establish some dentry locking rule for calling into
d_splice_alias() instead? At least then the callers can make sure the dentry does
not change under them?
Though I guess if there's dentry locking like that, we might as well do all the
checking in d_splice_alias(), but that means the unhashed dentries would no
longer be disallowed which is a change of semantic from now.