* [PATCH/RFC] VFS: Improve fairness when locking the per-superblock s_anon list
@ 2016-01-29 0:17 NeilBrown
2016-02-01 21:26 ` J. Bruce Fields
0 siblings, 1 reply; 4+ messages in thread
From: NeilBrown @ 2016-01-29 0:17 UTC (permalink / raw)
To: Al Viro, Dave Chinner; +Cc: LKML, linux-nfs
[-- Attachment #1: Type: text/plain, Size: 2360 bytes --]
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.
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@suse.com>
---
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH/RFC] VFS: Improve fairness when locking the per-superblock s_anon list
2016-01-29 0:17 [PATCH/RFC] VFS: Improve fairness when locking the per-superblock s_anon list NeilBrown
@ 2016-02-01 21:26 ` J. Bruce Fields
2016-02-02 4:10 ` NeilBrown
0 siblings, 1 reply; 4+ messages in thread
From: J. Bruce Fields @ 2016-02-01 21:26 UTC (permalink / raw)
To: NeilBrown; +Cc: Al Viro, Dave Chinner, LKML, linux-nfs
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@suse.com>
> ---
>
> 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH/RFC] VFS: Improve fairness when locking the per-superblock s_anon list
2016-02-01 21:26 ` J. Bruce Fields
@ 2016-02-02 4:10 ` NeilBrown
2016-02-02 14:32 ` J. Bruce Fields
0 siblings, 1 reply; 4+ messages in thread
From: NeilBrown @ 2016-02-02 4:10 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Al Viro, Dave Chinner, LKML, linux-nfs
[-- Attachment #1: Type: text/plain, Size: 3768 bytes --]
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.
>
> 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...
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@suse.com>
>> ---
>>
>> 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
>>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH/RFC] VFS: Improve fairness when locking the per-superblock s_anon list
2016-02-02 4:10 ` NeilBrown
@ 2016-02-02 14:32 ` J. Bruce Fields
0 siblings, 0 replies; 4+ messages in thread
From: J. Bruce Fields @ 2016-02-02 14:32 UTC (permalink / raw)
To: NeilBrown; +Cc: Al Viro, Dave Chinner, LKML, linux-nfs
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@suse.com>
> >> ---
> >>
> >> 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
> >>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-02 14:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-29 0:17 [PATCH/RFC] VFS: Improve fairness when locking the per-superblock s_anon list NeilBrown
2016-02-01 21:26 ` J. Bruce Fields
2016-02-02 4:10 ` NeilBrown
2016-02-02 14:32 ` J. Bruce Fields
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).