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?
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?
--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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
