Re: [PATCH/RFC] VFS: Improve fairness when locking the per-superblock s_anon list

[J. Bruce Fields]

On Tue, Feb 02, 2016 at 03:10:43PM +1100, NeilBrown wrote:
> On Tue, Feb 02 2016, J. Bruce Fields wrote:
> 
> > On Fri, Jan 29, 2016 at 11:17:43AM +1100, NeilBrown wrote:
> >> bit-spin-locks, as used for dcache hash chains, are not fair.
> >> This is not a problem for the dcache hash table as different CPUs are
> >> likely to access different entries in the hash table so high contention
> >> is not expected.
> >> However anonymous dentryies (created by NFSD) all live on a single hash
> >> chain "s_anon" and the bitlock on this can be highly contended, resulting
> >> in soft-lockup warnings.
> >
> > Just out of curiosity, because I can't recall seeing complaints about
> > warnings, when do you see it happen?  Server reboot, maybe?
> 
> Soft-lockup warnings.  Possibly some client might notice delays longer
> than they should be, but the only actual complaints have been about the warnings.

Yeah, I was curious about the cause, not the effect.  Server reboot
seems like one of those cases where you might suddenly have to do a lot
of uncached lookups-by-filehandle.

> >
> > It should be hitting that __d_obtain_alias() case only when a filehandle
> > lookup finds a file without a cached dentry, which is an important case
> > to handle, but not normally what I'd expect to be the common case.  Am I
> > forgetting something?
> 
> I don't think you are missing anything significant.  I too was somewhat
> surprised that there would be enough contention to cause problems, but
> the evidence was fairly conclusive (at two separate sites), and the
> proposed fix made the symptoms disappear.
> 
> Maybe there are a great many different files being accessed and a lot of
> memory pressure on the server keeps pushing them out of cache.  I find
> that customers often have loads that have quite different from what I
> might expect...

Sure.  Thanks for looking at this.  (Feel free to add a "Reviewed-by",
FWIW, though in my case that's only "agree that it fixes a real
problem".  I've no informed opinion on the correct solution....).

--b.

> 
> Thanks,
> NeilBrown
> 
> 
> >
> > --b.
> >
> >> 
> >> So introduce a global (fair) spinlock and take it before grabing the
> >> bitlock on s_anon.  This provides fairness and makes the warnings go away.
> >> 
> >> We could alternately use s_inode_list_lock, or add another spinlock
> >> to struct super_block.  Suggestions?
> >> 
> >> Signed-off-by: NeilBrown <neilb@xxxxxxxx>
> >> ---
> >> 
> >> Dave: I'd guess you would be against using the new s_inode_list_lock
> >> for this because it is already highly contended - yes?
> >> Is it worth adding another spinlock to 'struct super_block' ?
> >> 
> >> Thanks,
> >> NeilBrown
> >> 
> >> 
> >>  fs/dcache.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >> 
> >> diff --git a/fs/dcache.c b/fs/dcache.c
> >> index 92d5140de851..e83f1ac1540c 100644
> >> --- a/fs/dcache.c
> >> +++ b/fs/dcache.c
> >> @@ -104,6 +104,8 @@ static unsigned int d_hash_shift __read_mostly;
> >>  
> >>  static struct hlist_bl_head *dentry_hashtable __read_mostly;
> >>  
> >> +static DEFINE_SPINLOCK(s_anon_lock);
> >> +
> >>  static inline struct hlist_bl_head *d_hash(const struct dentry *parent,
> >>  					unsigned int hash)
> >>  {
> >> @@ -490,10 +492,14 @@ void __d_drop(struct dentry *dentry)
> >>  		else
> >>  			b = d_hash(dentry->d_parent, dentry->d_name.hash);
> >>  
> >> +		if (b == &dentry->d_sb->s_anon)
> >> +			spin_lock(&s_anon_lock);
> >>  		hlist_bl_lock(b);
> >>  		__hlist_bl_del(&dentry->d_hash);
> >>  		dentry->d_hash.pprev = NULL;
> >>  		hlist_bl_unlock(b);
> >> +		if (b == &dentry->d_sb->s_anon)
> >> +			spin_unlock(&s_anon_lock);
> >>  		dentry_rcuwalk_invalidate(dentry);
> >>  	}
> >>  }
> >> @@ -1978,9 +1984,11 @@ static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected)
> >>  	spin_lock(&tmp->d_lock);
> >>  	__d_set_inode_and_type(tmp, inode, add_flags);
> >>  	hlist_add_head(&tmp->d_u.d_alias, &inode->i_dentry);
> >> +	spin_lock(&s_anon_lock);
> >>  	hlist_bl_lock(&tmp->d_sb->s_anon);
> >>  	hlist_bl_add_head(&tmp->d_hash, &tmp->d_sb->s_anon);
> >>  	hlist_bl_unlock(&tmp->d_sb->s_anon);
> >> +	spin_unlock(&s_anon_lock);
> >>  	spin_unlock(&tmp->d_lock);
> >>  	spin_unlock(&inode->i_lock);
> >>  	security_d_instantiate(tmp, inode);
> >> -- 
> >> 2.7.0
> >>