Re: Q. hlist_bl_add_head_rcu() in d_alloc_parallel()

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

On Sat, Jun 18, 2016 at 05:50:30AM +0900, J. R. Okajima wrote:
> 	hlist_bl_lock(b);
> 	rcu_read_unlock();
> 	hlist_bl_for_each_entry(dentry, node, b, d_u.d_in_lookup_hash) {
> 		if (!matched_dentry_found)
> 			continue;
> 		dget(dentry);
> 		hlist_bl_unlock(b);
> 		return dentry;
> 	}
> 	hlist_bl_add_head_rcu(&new->d_u.d_in_lookup_hash, b);
> 	hlist_bl_unlock(b);
> 	return new;
> }

> When two processes try opening a single existing file and enters
> d_alloc_parallel() at the same time, only one process wins and should
> succeeds hlist_bl_add_head_rcu(). The other process should find the
> dentry in d_u.d_in_lookup_hash and return 'dentry' (instead of
> 'new'). Am I right?
> 
> My question is when will 'new' be added into d_u.d_in_lookup_hash?

You've quoted it yourself:
> 	hlist_bl_add_head_rcu(&new->d_u.d_in_lookup_hash, b);

> It should be between these two lines, I guess.
> 	rcu_read_unlock();
> 	hlist_bl_for_each_entry(dentry, node, b, d_u.d_in_lookup_hash) {
> But can it surely happen?
> If 'new' is not added here because someone else is in rcu_read_lock
> region or other reason, then both processes will add the same named but
> different dentry?

Huh?  If you have two processes reaching that insertion into the in-lookup
hash, whoever gets the hlist_bl_lock() first wins; the loser will find
the instance added by the winner and bugger off with it.  RCU is completely
unrelated to that.  It's about the search in *primary* hash.
 
> Is it better to change the lock/unlock-order like this?
> 
> 	rcu_read_unlock();
> 	rcu_barrier();
> 	hlist_bl_lock(b);
> 	hlist_bl_for_each_entry(dentry, node, b, d_u.d_in_lookup_hash) {

No.  We need to verify that nothing had been transferred from in-lookup to
primary hash after we'd found no match in the primary.  That's what the
->i_dir_seq check is about, and it has to be done after we'd locked the
in-lookup hash chain.  We could drop rcu_read_lock before locking the
chain, but this way we make sure we won't get preempted between fetching
the ->i_dir_seq and checking if it had been changed.  The last thing we
want is rcu_barrier() - WTF for?  To get an extra chance of something
being moved from in-lookup to primary, forcing us to repeat the primary
hash lookup?

We are *NOT* modifying the primary hash in d_alloc_parallel().  At all.
With respect to the primary hash it's a pure reader.  And in-lookup hash
is only accessed under hlist_bl_lock() on its chains - no RCU accesses to
that one.