Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1416327 > unrolled thread
| Started by | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| First post | 2016-06-07 17:40 +0200 |
| Last post | 2016-06-09 14:30 +0200 |
| Articles | 20 on this page of 45 — 4 participants |
Back to article view | Back to linux.kernel
Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-07 17:40 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Jeff Layton <jlayton@poochiereds.net> - 2016-06-07 19:20 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-07 19:40 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Jeff Layton <jlayton@poochiereds.net> - 2016-06-07 22:10 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 01:40 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Jeff Layton <jlayton@poochiereds.net> - 2016-06-08 02:10 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 02:50 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 04:30 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 06:00 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Jeff Layton <jlayton@poochiereds.net> - 2016-06-08 13:00 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 16:50 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 18:20 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Jeff Layton <jlayton@poochiereds.net> - 2016-06-08 19:30 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 19:40 +0200
[PATCH] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-09 05:00 +0200
Re: [PATCH] nfsd: Always lock state exclusively. Jeff Layton <jlayton@poochiereds.net> - 2016-06-09 12:20 +0200
[PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Oleg Drokin <green@linuxhacker.ru> - 2016-06-09 23:10 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Oleg Drokin <green@linuxhacker.ru> - 2016-06-10 06:20 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Jeff Layton <jlayton@poochiereds.net> - 2016-06-10 13:00 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-10 23:00 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Oleg Drokin <green@linuxhacker.ru> - 2016-06-11 17:50 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Jeff Layton <jlayton@poochiereds.net> - 2016-06-12 03:40 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Oleg Drokin <green@linuxhacker.ru> - 2016-06-12 04:10 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Jeff Layton <jlayton@poochiereds.net> - 2016-06-12 05:00 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Oleg Drokin <green@linuxhacker.ru> - 2016-06-12 05:20 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Jeff Layton <jlayton@poochiereds.net> - 2016-06-12 15:20 +0200
[PATCH v2] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-13 03:30 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-14 17:40 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-14 18:00 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-14 21:00 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. Jeff Layton <jlayton@poochiereds.net> - 2016-06-15 01:00 +0200
[PATCH 3/3] nfsd: Make init_open_stateid() a bit more whole Oleg Drokin <green@linuxhacker.ru> - 2016-06-15 05:30 +0200
[PATCH 0/3] nfsd state handling fixes Oleg Drokin <green@linuxhacker.ru> - 2016-06-15 05:30 +0200
[PATCH 2/3] nfsd: Extend the mutex holding region around in nfsd4_process_open2() Oleg Drokin <green@linuxhacker.ru> - 2016-06-15 05:40 +0200
[PATCH 1/3] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-15 05:40 +0200
Re: [PATCH 0/3] nfsd state handling fixes Oleg Drokin <green@linuxhacker.ru> - 2016-06-16 04:00 +0200
Re: [PATCH 0/3] nfsd state handling fixes "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-16 04:10 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-15 01:00 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. Jeff Layton <jlayton@poochiereds.net> - 2016-06-15 01:00 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-14 17:50 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-14 18:00 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-14 20:50 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-15 04:30 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-15 15:40 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Andrew W Elble <aweits@rit.edu> - 2016-06-09 14:30 +0200
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| Date | 2016-06-11 17:50 +0200 |
| Subject | Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file |
| Message-ID | <rITkC-4S0-11@gated-at.bofh.it> |
| In reply to | #1419723 |
On Jun 10, 2016, at 4:55 PM, J . Bruce Fields wrote:
> On Fri, Jun 10, 2016 at 06:50:33AM -0400, Jeff Layton wrote:
>> On Fri, 2016-06-10 at 00:18 -0400, Oleg Drokin wrote:
>>> On Jun 9, 2016, at 5:01 PM, Oleg Drokin wrote:
>>>
>>>> Currently there's an unprotected access mode check in
>>>> nfs4_upgrade_open
>>>> that then calls nfs4_get_vfs_file which in turn assumes whatever
>>>> access mode was present in the state is still valid which is racy.
>>>> Two nfs4_get_vfs_file van enter the same path as result and get two
>>>> references to nfs4_file, but later drop would only happens once
>>>> because
>>>> access mode is only denoted by bits, so no refcounting.
>>>>
>>>> The locking around access mode testing is introduced to avoid this
>>>> race.
>>>>
>>>> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
>>>> ---
>>>>
>>>> This patch performs equally well to the st_rwsem -> mutex
>>>> conversion,
>>>> but is a bit ligher-weight I imagine.
>>>> For one it seems to allow truncates in parallel if we ever want it.
>>>>
>>>> fs/nfsd/nfs4state.c | 28 +++++++++++++++++++++++++---
>>>> 1 file changed, 25 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index f5f82e1..d4b9eba 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -3958,6 +3958,11 @@ static __be32 nfs4_get_vfs_file(struct
>>>> svc_rqst *rqstp, struct nfs4_file *fp,
>>>>
>>>> spin_lock(&fp->fi_lock);
>>>>
>>>> + if (test_access(open->op_share_access, stp)) {
>>>> + spin_unlock(&fp->fi_lock);
>>>> + return nfserr_eagain;
>>>> + }
>>>> +
>>>> /*
>>>> * Are we trying to set a deny mode that would conflict with
>>>> * current access?
>>>> @@ -4017,11 +4022,21 @@ nfs4_upgrade_open(struct svc_rqst *rqstp,
>>>> struct nfs4_file *fp, struct svc_fh *c
>>>> __be32 status;
>>>> unsigned char old_deny_bmap = stp->st_deny_bmap;
>>>>
>>>> - if (!test_access(open->op_share_access, stp))
>>>> - return nfs4_get_vfs_file(rqstp, fp, cur_fh, stp,
>>>> open);
>>>> +again:
>>>> + spin_lock(&fp->fi_lock);
>>>> + if (!test_access(open->op_share_access, stp)) {
>>>> + spin_unlock(&fp->fi_lock);
>>>> + status = nfs4_get_vfs_file(rqstp, fp, cur_fh, stp,
>>>> open);
>>>> + /*
>>>> + * Somebody won the race for access while we did
>>>> not hold
>>>> + * the lock here
>>>> + */
>>>> + if (status == nfserr_eagain)
>>>> + goto again;
>>>> + return status;
>>>> + }
>>>>
>>>> /* test and set deny mode */
>>>> - spin_lock(&fp->fi_lock);
>>>> status = nfs4_file_check_deny(fp, open->op_share_deny);
>>>> if (status == nfs_ok) {
>>>> set_deny(open->op_share_deny, stp);
>>>> @@ -4361,6 +4376,13 @@ nfsd4_process_open2(struct svc_rqst *rqstp,
>>>> struct svc_fh *current_fh, struct nf
>>>> status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp,
>>>> open);
>>>> if (status) {
>>>> up_read(&stp->st_rwsem);
>>>> + /*
>>>> + * EAGAIN is returned when there's a
>>>> racing access,
>>>> + * this should never happen as we are the
>>>> only user
>>>> + * of this new state, and since it's not
>>>> yet hashed,
>>>> + * nobody can find it
>>>> + */
>>>> + WARN_ON(status == nfserr_eagain);
>>>
>>> Ok, some more testing shows that this CAN happen.
>>> So this patch is inferior to the mutex one after all.
>>>
>>
>> Yeah, that can happen for all sorts of reasons. As Andrew pointed out,
>> you can get this when there is a lease break in progress, and that may
>> be occurring for a completely different stateid (or because of samba,
>> etc...)
>>
>> It may be possible to do something like this, but we'd need to audit
>> all of the handling of st_access_bmap (and the deny bmap) to ensure
>> that we get it right.
>>
>> For now, I think just turning that rwsem into a mutex is the best
>> solution. That is a per-stateid mutex so any contention is going to be
>> due to the client sending racing OPEN calls for the same inode anyway.
>> Allowing those to run in parallel again could be useful in some cases,
>> but most use-cases won't be harmed by that serialization.
>
> OK, so for now my plan is to take "nfsd: Always lock state exclusively"
> for 4.7. Thanks to both of you for your work on this….
FYI, I just hit this again with the "Always lock state exclusively" patch too.
I hate when that happens. But it is much harder to hit now.
the trace is also in the nfs4_get_vfs_file() that's called directly from
nfsd4_process_open2().
Otherwise the symptoms are pretty same - first I get the warning in set_access
that the flag is already set and then the nfsd4_free_file_rcu() one and then
unmount of underlying fs fails.
What's strange is I am not sure what else can set the flag.
Basically set_access is called from nfs4_get_vfs_file() - under the mutex via
nfsd4_process_open2() directly or via nfs4_upgrade_open()...,
or from get_lock_access() - without the mutex, but my workload does not do any
file locking, so it should not really be hitting, right?
Ah! I think I see it.
This patch has the same problem as the spinlock moving one:
When we call nfs4_get_vfs_file() directly from nfsd4_process_open2(), there's no
check for anything, so supposed we take this diret cllign path, there's a
mutex_lock() just before the call, but what's to stop another thread from finding
this stateid meanwhile and being first with the mutex and then also settign the
access mode that our first thread no longer sets?
This makes it so that in fact we can never skip the access mode testing unless
testing and setting is atomically done under the same lock which it is not now.
The patch that extended the coverage of the fi_lock got that right, I did not hit
any leaks there, just the "unhandled EAGAIN" warn on, which is wrong in it's own right.
The surprising part is that the state that's not yet been through find_or_hash_clnt_odstate could still be found by somebody else? Is it really
supposed to work like that?
So if we are to check the access mode at all times in nfs4_get_vfs_file(),
and return eagain, should we just check for that (all under the same
lock if we go with the mutex patch) and call into nfs4_upgrade_open in that
case again?
Or I guess it's even better if we resurrect the fi_lock coverage extension patch and
do it there, as that would mean at least the check and set are atomic wrt
locking?
[toc] | [prev] | [next] | [standalone]
| From | Jeff Layton <jlayton@poochiereds.net> |
|---|---|
| Date | 2016-06-12 03:40 +0200 |
| Subject | Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file |
| Message-ID | <rJ2xz-29L-7@gated-at.bofh.it> |
| In reply to | #1419992 |
On Sat, 2016-06-11 at 11:41 -0400, Oleg Drokin wrote:
> On Jun 10, 2016, at 4:55 PM, J . Bruce Fields wrote:
>
> > On Fri, Jun 10, 2016 at 06:50:33AM -0400, Jeff Layton wrote:
> > > On Fri, 2016-06-10 at 00:18 -0400, Oleg Drokin wrote:
> > > > On Jun 9, 2016, at 5:01 PM, Oleg Drokin wrote:
> > > >
> > > > > Currently there's an unprotected access mode check in
> > > > > nfs4_upgrade_open
> > > > > that then calls nfs4_get_vfs_file which in turn assumes whatever
> > > > > access mode was present in the state is still valid which is racy.
> > > > > Two nfs4_get_vfs_file van enter the same path as result and get two
> > > > > references to nfs4_file, but later drop would only happens once
> > > > > because
> > > > > access mode is only denoted by bits, so no refcounting.
> > > > >
> > > > > The locking around access mode testing is introduced to avoid this
> > > > > race.
> > > > >
> > > > > Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
> > > > > ---
> > > > >
> > > > > This patch performs equally well to the st_rwsem -> mutex
> > > > > conversion,
> > > > > but is a bit ligher-weight I imagine.
> > > > > For one it seems to allow truncates in parallel if we ever want it.
> > > > >
> > > > > fs/nfsd/nfs4state.c | 28 +++++++++++++++++++++++++---
> > > > > 1 file changed, 25 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > > index f5f82e1..d4b9eba 100644
> > > > > --- a/fs/nfsd/nfs4state.c
> > > > > +++ b/fs/nfsd/nfs4state.c
> > > > > @@ -3958,6 +3958,11 @@ static __be32 nfs4_get_vfs_file(struct
> > > > > svc_rqst *rqstp, struct nfs4_file *fp,
> > > > >
> > > > > spin_lock(&fp->fi_lock);
> > > > >
> > > > > + if (test_access(open->op_share_access, stp)) {
> > > > > + spin_unlock(&fp->fi_lock);
> > > > > + return nfserr_eagain;
> > > > > + }
> > > > > +
> > > > > /*
> > > > > * Are we trying to set a deny mode that would conflict with
> > > > > * current access?
> > > > > @@ -4017,11 +4022,21 @@ nfs4_upgrade_open(struct svc_rqst *rqstp,
> > > > > struct nfs4_file *fp, struct svc_fh *c
> > > > > __be32 status;
> > > > > unsigned char old_deny_bmap = stp->st_deny_bmap;
> > > > >
> > > > > - if (!test_access(open->op_share_access, stp))
> > > > > - return nfs4_get_vfs_file(rqstp, fp, cur_fh, stp,
> > > > > open);
> > > > > +again:
> > > > > + spin_lock(&fp->fi_lock);
> > > > > + if (!test_access(open->op_share_access, stp)) {
> > > > > + spin_unlock(&fp->fi_lock);
> > > > > + status = nfs4_get_vfs_file(rqstp, fp, cur_fh, stp,
> > > > > open);
> > > > > + /*
> > > > > + * Somebody won the race for access while we did
> > > > > not hold
> > > > > + * the lock here
> > > > > + */
> > > > > + if (status == nfserr_eagain)
> > > > > + goto again;
> > > > > + return status;
> > > > > + }
> > > > >
> > > > > /* test and set deny mode */
> > > > > - spin_lock(&fp->fi_lock);
> > > > > status = nfs4_file_check_deny(fp, open->op_share_deny);
> > > > > if (status == nfs_ok) {
> > > > > set_deny(open->op_share_deny, stp);
> > > > > @@ -4361,6 +4376,13 @@ nfsd4_process_open2(struct svc_rqst *rqstp,
> > > > > struct svc_fh *current_fh, struct nf
> > > > > status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp,
> > > > > open);
> > > > > if (status) {
> > > > > up_read(&stp->st_rwsem);
> > > > > + /*
> > > > > + * EAGAIN is returned when there's a
> > > > > racing access,
> > > > > + * this should never happen as we are the
> > > > > only user
> > > > > + * of this new state, and since it's not
> > > > > yet hashed,
> > > > > + * nobody can find it
> > > > > + */
> > > > > + WARN_ON(status == nfserr_eagain);
> > > >
> > > > Ok, some more testing shows that this CAN happen.
> > > > So this patch is inferior to the mutex one after all.
> > > >
> > >
> > > Yeah, that can happen for all sorts of reasons. As Andrew pointed out,
> > > you can get this when there is a lease break in progress, and that may
> > > be occurring for a completely different stateid (or because of samba,
> > > etc...)
> > >
> > > It may be possible to do something like this, but we'd need to audit
> > > all of the handling of st_access_bmap (and the deny bmap) to ensure
> > > that we get it right.
> > >
> > > For now, I think just turning that rwsem into a mutex is the best
> > > solution. That is a per-stateid mutex so any contention is going to be
> > > due to the client sending racing OPEN calls for the same inode anyway.
> > > Allowing those to run in parallel again could be useful in some cases,
> > > but most use-cases won't be harmed by that serialization.
> >
> > OK, so for now my plan is to take "nfsd: Always lock state exclusively"
> > for 4.7. Thanks to both of you for your work on this….
>
>
> FYI, I just hit this again with the "Always lock state exclusively" patch too.
> I hate when that happens. But it is much harder to hit now.
>
> the trace is also in the nfs4_get_vfs_file() that's called directly from
> nfsd4_process_open2().
>
> Otherwise the symptoms are pretty same - first I get the warning in set_access
> that the flag is already set and then the nfsd4_free_file_rcu() one and then
> unmount of underlying fs fails.
>
> What's strange is I am not sure what else can set the flag.
> Basically set_access is called from nfs4_get_vfs_file() - under the mutex via
> nfsd4_process_open2() directly or via nfs4_upgrade_open()...,
> or from get_lock_access() - without the mutex, but my workload does not do any
> file locking, so it should not really be hitting, right?
>
>
> Ah! I think I see it.
> This patch has the same problem as the spinlock moving one:
> When we call nfs4_get_vfs_file() directly from nfsd4_process_open2(), there's no
> check for anything, so supposed we take this diret cllign path, there's a
> mutex_lock() just before the call, but what's to stop another thread from finding
> this stateid meanwhile and being first with the mutex and then also settign the
> access mode that our first thread no longer sets?
> This makes it so that in fact we can never skip the access mode testing unless
> testing and setting is atomically done under the same lock which it is not now.
> The patch that extended the coverage of the fi_lock got that right, I did not hit
> any leaks there, just the "unhandled EAGAIN" warn on, which is wrong in it's own right.
> The surprising part is that the state that's not yet been through find_or_hash_clnt_odstate could still be found by somebody else? Is it really
> supposed to work like that?
>
> So if we are to check the access mode at all times in nfs4_get_vfs_file(),
> and return eagain, should we just check for that (all under the same
> lock if we go with the mutex patch) and call into nfs4_upgrade_open in that
> case again?
> Or I guess it's even better if we resurrect the fi_lock coverage extension patch and
> do it there, as that would mean at least the check and set are atomic wrt
> locking?
Good catch. Could we fix this by locking the mutex before hashing the
new stateid (and having init_open_stateid return with it locked if it
finds an existing one?).
--
Jeff Layton <jlayton@poochiereds.net>
[toc] | [prev] | [next] | [standalone]
| From | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| Date | 2016-06-12 04:10 +0200 |
| Subject | Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file |
| Message-ID | <rJ30B-2zr-9@gated-at.bofh.it> |
| In reply to | #1420118 |
On Jun 11, 2016, at 9:33 PM, Jeff Layton wrote:
> On Sat, 2016-06-11 at 11:41 -0400, Oleg Drokin wrote:
>> On Jun 10, 2016, at 4:55 PM, J . Bruce Fields wrote:
>>
>>> On Fri, Jun 10, 2016 at 06:50:33AM -0400, Jeff Layton wrote:
>>>> On Fri, 2016-06-10 at 00:18 -0400, Oleg Drokin wrote:
>>>>> On Jun 9, 2016, at 5:01 PM, Oleg Drokin wrote:
>>>>>
>>>>>> Currently there's an unprotected access mode check in
>>>>>> nfs4_upgrade_open
>>>>>> that then calls nfs4_get_vfs_file which in turn assumes whatever
>>>>>> access mode was present in the state is still valid which is racy.
>>>>>> Two nfs4_get_vfs_file van enter the same path as result and get two
>>>>>> references to nfs4_file, but later drop would only happens once
>>>>>> because
>>>>>> access mode is only denoted by bits, so no refcounting.
>>>>>>
>>>>>> The locking around access mode testing is introduced to avoid this
>>>>>> race.
>>>>>>
>>>>>> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
>>>>>> ---
>>>>>>
>>>>>> This patch performs equally well to the st_rwsem -> mutex
>>>>>> conversion,
>>>>>> but is a bit ligher-weight I imagine.
>>>>>> For one it seems to allow truncates in parallel if we ever want it.
>>>>>>
>>>>>> fs/nfsd/nfs4state.c | 28 +++++++++++++++++++++++++---
>>>>>> 1 file changed, 25 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>>> index f5f82e1..d4b9eba 100644
>>>>>> --- a/fs/nfsd/nfs4state.c
>>>>>> +++ b/fs/nfsd/nfs4state.c
>>>>>> @@ -3958,6 +3958,11 @@ static __be32 nfs4_get_vfs_file(struct
>>>>>> svc_rqst *rqstp, struct nfs4_file *fp,
>>>>>>
>>>>>> spin_lock(&fp->fi_lock);
>>>>>>
>>>>>> + if (test_access(open->op_share_access, stp)) {
>>>>>> + spin_unlock(&fp->fi_lock);
>>>>>> + return nfserr_eagain;
>>>>>> + }
>>>>>> +
>>>>>> /*
>>>>>> * Are we trying to set a deny mode that would conflict with
>>>>>> * current access?
>>>>>> @@ -4017,11 +4022,21 @@ nfs4_upgrade_open(struct svc_rqst *rqstp,
>>>>>> struct nfs4_file *fp, struct svc_fh *c
>>>>>> __be32 status;
>>>>>> unsigned char old_deny_bmap = stp->st_deny_bmap;
>>>>>>
>>>>>> - if (!test_access(open->op_share_access, stp))
>>>>>> - return nfs4_get_vfs_file(rqstp, fp, cur_fh, stp,
>>>>>> open);
>>>>>> +again:
>>>>>> + spin_lock(&fp->fi_lock);
>>>>>> + if (!test_access(open->op_share_access, stp)) {
>>>>>> + spin_unlock(&fp->fi_lock);
>>>>>> + status = nfs4_get_vfs_file(rqstp, fp, cur_fh, stp,
>>>>>> open);
>>>>>> + /*
>>>>>> + * Somebody won the race for access while we did
>>>>>> not hold
>>>>>> + * the lock here
>>>>>> + */
>>>>>> + if (status == nfserr_eagain)
>>>>>> + goto again;
>>>>>> + return status;
>>>>>> + }
>>>>>>
>>>>>> /* test and set deny mode */
>>>>>> - spin_lock(&fp->fi_lock);
>>>>>> status = nfs4_file_check_deny(fp, open->op_share_deny);
>>>>>> if (status == nfs_ok) {
>>>>>> set_deny(open->op_share_deny, stp);
>>>>>> @@ -4361,6 +4376,13 @@ nfsd4_process_open2(struct svc_rqst *rqstp,
>>>>>> struct svc_fh *current_fh, struct nf
>>>>>> status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp,
>>>>>> open);
>>>>>> if (status) {
>>>>>> up_read(&stp->st_rwsem);
>>>>>> + /*
>>>>>> + * EAGAIN is returned when there's a
>>>>>> racing access,
>>>>>> + * this should never happen as we are the
>>>>>> only user
>>>>>> + * of this new state, and since it's not
>>>>>> yet hashed,
>>>>>> + * nobody can find it
>>>>>> + */
>>>>>> + WARN_ON(status == nfserr_eagain);
>>>>>
>>>>> Ok, some more testing shows that this CAN happen.
>>>>> So this patch is inferior to the mutex one after all.
>>>>>
>>>>
>>>> Yeah, that can happen for all sorts of reasons. As Andrew pointed out,
>>>> you can get this when there is a lease break in progress, and that may
>>>> be occurring for a completely different stateid (or because of samba,
>>>> etc...)
>>>>
>>>> It may be possible to do something like this, but we'd need to audit
>>>> all of the handling of st_access_bmap (and the deny bmap) to ensure
>>>> that we get it right.
>>>>
>>>> For now, I think just turning that rwsem into a mutex is the best
>>>> solution. That is a per-stateid mutex so any contention is going to be
>>>> due to the client sending racing OPEN calls for the same inode anyway.
>>>> Allowing those to run in parallel again could be useful in some cases,
>>>> but most use-cases won't be harmed by that serialization.
>>>
>>> OK, so for now my plan is to take "nfsd: Always lock state exclusively"
>>> for 4.7. Thanks to both of you for your work on this….
>>
>>
>> FYI, I just hit this again with the "Always lock state exclusively" patch too.
>> I hate when that happens. But it is much harder to hit now.
>>
>> the trace is also in the nfs4_get_vfs_file() that's called directly from
>> nfsd4_process_open2().
>>
>> Otherwise the symptoms are pretty same - first I get the warning in set_access
>> that the flag is already set and then the nfsd4_free_file_rcu() one and then
>> unmount of underlying fs fails.
>>
>> What's strange is I am not sure what else can set the flag.
>> Basically set_access is called from nfs4_get_vfs_file() - under the mutex via
>> nfsd4_process_open2() directly or via nfs4_upgrade_open()...,
>> or from get_lock_access() - without the mutex, but my workload does not do any
>> file locking, so it should not really be hitting, right?
>>
>>
>> Ah! I think I see it.
>> This patch has the same problem as the spinlock moving one:
>> When we call nfs4_get_vfs_file() directly from nfsd4_process_open2(), there's no
>> check for anything, so supposed we take this diret cllign path, there's a
>> mutex_lock() just before the call, but what's to stop another thread from finding
>> this stateid meanwhile and being first with the mutex and then also settign the
>> access mode that our first thread no longer sets?
>> This makes it so that in fact we can never skip the access mode testing unless
>> testing and setting is atomically done under the same lock which it is not now.
>> The patch that extended the coverage of the fi_lock got that right, I did not hit
>> any leaks there, just the "unhandled EAGAIN" warn on, which is wrong in it's own right.
>> The surprising part is that the state that's not yet been through find_or_hash_clnt_odstate could still be found by somebody else? Is it really
>> supposed to work like that?
>>
>> So if we are to check the access mode at all times in nfs4_get_vfs_file(),
>> and return eagain, should we just check for that (all under the same
>> lock if we go with the mutex patch) and call into nfs4_upgrade_open in that
>> case again?
>> Or I guess it's even better if we resurrect the fi_lock coverage extension patch and
>> do it there, as that would mean at least the check and set are atomic wrt
>> locking?
>
> Good catch. Could we fix this by locking the mutex before hashing the
> new stateid (and having init_open_stateid return with it locked if it
> finds an existing one?).
Hm. I am trying to lock the newly initialized one and that seems to be holding up
well (but I want 24 hours just to be extra sure).
Hn, I just noticed a bug in this, so that'll reset the clock back.
But I think we cannot return with locked one if we found existing one due to lock
inversion?
I see that normally first we lock the state rwsem (now mutex) and then
lock the fi_lock.
Now if we make init_open_stateid() to lock the new state mutex while the fi_lock
is locked - that's probably ok, because we can do it before adding it to the list,
so nobody can find it.
Now the existing state that we find, we cannot really lock while holding that fi_lock,
because what if there's a parallel thread that already holds the mutex and now
wants the fi_lock?
And so it's probably best to return with existing state unlocked and let caller lock it?
Or do you think it's best to separately lock the found stp outside of spinlock
just for consistency?
[toc] | [prev] | [next] | [standalone]
| From | Jeff Layton <jlayton@poochiereds.net> |
|---|---|
| Date | 2016-06-12 05:00 +0200 |
| Subject | Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file |
| Message-ID | <rJ3MZ-2Xu-5@gated-at.bofh.it> |
| In reply to | #1420124 |
On Sat, 2016-06-11 at 22:06 -0400, Oleg Drokin wrote:
> On Jun 11, 2016, at 9:33 PM, Jeff Layton wrote:
>
> > On Sat, 2016-06-11 at 11:41 -0400, Oleg Drokin wrote:
> > > On Jun 10, 2016, at 4:55 PM, J . Bruce Fields wrote:
> > >
> > > > On Fri, Jun 10, 2016 at 06:50:33AM -0400, Jeff Layton wrote:
> > > > > On Fri, 2016-06-10 at 00:18 -0400, Oleg Drokin wrote:
> > > > > > On Jun 9, 2016, at 5:01 PM, Oleg Drokin wrote:
> > > > > >
> > > > > > > Currently there's an unprotected access mode check in
> > > > > > > nfs4_upgrade_open
> > > > > > > that then calls nfs4_get_vfs_file which in turn assumes whatever
> > > > > > > access mode was present in the state is still valid which is racy.
> > > > > > > Two nfs4_get_vfs_file van enter the same path as result and get two
> > > > > > > references to nfs4_file, but later drop would only happens once
> > > > > > > because
> > > > > > > access mode is only denoted by bits, so no refcounting.
> > > > > > >
> > > > > > > The locking around access mode testing is introduced to avoid this
> > > > > > > race.
> > > > > > >
> > > > > > > Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
> > > > > > > ---
> > > > > > >
> > > > > > > This patch performs equally well to the st_rwsem -> mutex
> > > > > > > conversion,
> > > > > > > but is a bit ligher-weight I imagine.
> > > > > > > For one it seems to allow truncates in parallel if we ever want it.
> > > > > > >
> > > > > > > fs/nfsd/nfs4state.c | 28 +++++++++++++++++++++++++---
> > > > > > > 1 file changed, 25 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > > > > index f5f82e1..d4b9eba 100644
> > > > > > > --- a/fs/nfsd/nfs4state.c
> > > > > > > +++ b/fs/nfsd/nfs4state.c
> > > > > > > @@ -3958,6 +3958,11 @@ static __be32 nfs4_get_vfs_file(struct
> > > > > > > svc_rqst *rqstp, struct nfs4_file *fp,
> > > > > > >
> > > > > > > spin_lock(&fp->fi_lock);
> > > > > > >
> > > > > > > + if (test_access(open->op_share_access, stp)) {
> > > > > > > + spin_unlock(&fp->fi_lock);
> > > > > > > + return nfserr_eagain;
> > > > > > > + }
> > > > > > > +
> > > > > > > /*
> > > > > > > * Are we trying to set a deny mode that would conflict with
> > > > > > > * current access?
> > > > > > > @@ -4017,11 +4022,21 @@ nfs4_upgrade_open(struct svc_rqst *rqstp,
> > > > > > > struct nfs4_file *fp, struct svc_fh *c
> > > > > > > __be32 status;
> > > > > > > unsigned char old_deny_bmap = stp->st_deny_bmap;
> > > > > > >
> > > > > > > - if (!test_access(open->op_share_access, stp))
> > > > > > > - return nfs4_get_vfs_file(rqstp, fp, cur_fh, stp,
> > > > > > > open);
> > > > > > > +again:
> > > > > > > + spin_lock(&fp->fi_lock);
> > > > > > > + if (!test_access(open->op_share_access, stp)) {
> > > > > > > + spin_unlock(&fp->fi_lock);
> > > > > > > + status = nfs4_get_vfs_file(rqstp, fp, cur_fh, stp,
> > > > > > > open);
> > > > > > > + /*
> > > > > > > + * Somebody won the race for access while we did
> > > > > > > not hold
> > > > > > > + * the lock here
> > > > > > > + */
> > > > > > > + if (status == nfserr_eagain)
> > > > > > > + goto again;
> > > > > > > + return status;
> > > > > > > + }
> > > > > > >
> > > > > > > /* test and set deny mode */
> > > > > > > - spin_lock(&fp->fi_lock);
> > > > > > > status = nfs4_file_check_deny(fp, open->op_share_deny);
> > > > > > > if (status == nfs_ok) {
> > > > > > > set_deny(open->op_share_deny, stp);
> > > > > > > @@ -4361,6 +4376,13 @@ nfsd4_process_open2(struct svc_rqst *rqstp,
> > > > > > > struct svc_fh *current_fh, struct nf
> > > > > > > status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp,
> > > > > > > open);
> > > > > > > if (status) {
> > > > > > > up_read(&stp->st_rwsem);
> > > > > > > + /*
> > > > > > > + * EAGAIN is returned when there's a
> > > > > > > racing access,
> > > > > > > + * this should never happen as we are the
> > > > > > > only user
> > > > > > > + * of this new state, and since it's not
> > > > > > > yet hashed,
> > > > > > > + * nobody can find it
> > > > > > > + */
> > > > > > > + WARN_ON(status == nfserr_eagain);
> > > > > >
> > > > > > Ok, some more testing shows that this CAN happen.
> > > > > > So this patch is inferior to the mutex one after all.
> > > > > >
> > > > >
> > > > > Yeah, that can happen for all sorts of reasons. As Andrew pointed out,
> > > > > you can get this when there is a lease break in progress, and that may
> > > > > be occurring for a completely different stateid (or because of samba,
> > > > > etc...)
> > > > >
> > > > > It may be possible to do something like this, but we'd need to audit
> > > > > all of the handling of st_access_bmap (and the deny bmap) to ensure
> > > > > that we get it right.
> > > > >
> > > > > For now, I think just turning that rwsem into a mutex is the best
> > > > > solution. That is a per-stateid mutex so any contention is going to be
> > > > > due to the client sending racing OPEN calls for the same inode anyway.
> > > > > Allowing those to run in parallel again could be useful in some cases,
> > > > > but most use-cases won't be harmed by that serialization.
> > > >
> > > > OK, so for now my plan is to take "nfsd: Always lock state exclusively"
> > > > for 4.7. Thanks to both of you for your work on this….
> > >
> > >
> > > FYI, I just hit this again with the "Always lock state exclusively" patch too.
> > > I hate when that happens. But it is much harder to hit now.
> > >
> > > the trace is also in the nfs4_get_vfs_file() that's called directly from
> > > nfsd4_process_open2().
> > >
> > > Otherwise the symptoms are pretty same - first I get the warning in set_access
> > > that the flag is already set and then the nfsd4_free_file_rcu() one and then
> > > unmount of underlying fs fails.
> > >
> > > What's strange is I am not sure what else can set the flag.
> > > Basically set_access is called from nfs4_get_vfs_file() - under the mutex via
> > > nfsd4_process_open2() directly or via nfs4_upgrade_open()...,
> > > or from get_lock_access() - without the mutex, but my workload does not do any
> > > file locking, so it should not really be hitting, right?
> > >
> > >
> > > Ah! I think I see it.
> > > This patch has the same problem as the spinlock moving one:
> > > When we call nfs4_get_vfs_file() directly from nfsd4_process_open2(), there's no
> > > check for anything, so supposed we take this diret cllign path, there's a
> > > mutex_lock() just before the call, but what's to stop another thread from finding
> > > this stateid meanwhile and being first with the mutex and then also settign the
> > > access mode that our first thread no longer sets?
> > > This makes it so that in fact we can never skip the access mode testing unless
> > > testing and setting is atomically done under the same lock which it is not now.
> > > The patch that extended the coverage of the fi_lock got that right, I did not hit
> > > any leaks there, just the "unhandled EAGAIN" warn on, which is wrong in it's own right.
> > > The surprising part is that the state that's not yet been through find_or_hash_clnt_odstate could still be found by somebody else? Is it really
> > > supposed to work like that?
> > >
> > > So if we are to check the access mode at all times in nfs4_get_vfs_file(),
> > > and return eagain, should we just check for that (all under the same
> > > lock if we go with the mutex patch) and call into nfs4_upgrade_open in that
> > > case again?
> > > Or I guess it's even better if we resurrect the fi_lock coverage extension patch and
> > > do it there, as that would mean at least the check and set are atomic wrt
> > > locking?
> >
> > Good catch. Could we fix this by locking the mutex before hashing the
> > new stateid (and having init_open_stateid return with it locked if it
> > finds an existing one?).
>
> Hm. I am trying to lock the newly initialized one and that seems to be holding up
> well (but I want 24 hours just to be extra sure).
> Hn, I just noticed a bug in this, so that'll reset the clock back.
>
> But I think we cannot return with locked one if we found existing one due to lock
> inversion?
> I see that normally first we lock the state rwsem (now mutex) and then
> lock the fi_lock.
> Now if we make init_open_stateid() to lock the new state mutex while the fi_lock
> is locked - that's probably ok, because we can do it before adding it to the list,
> so nobody can find it.
> Now the existing state that we find, we cannot really lock while holding that fi_lock,
> because what if there's a parallel thread that already holds the mutex and now
> wants the fi_lock?
> And so it's probably best to return with existing state unlocked and let caller lock it?
> Or do you think it's best to separately lock the found stp outside of spinlock
> just for consistency?
I think we just have to ensure that if the new stateid is hashed that
its mutex is locked prior to being inserted into the hashtable. That
should prevent the race you mentioned.
If we find an existing one in the hashtable in init_open_stateid, then
we _can_ take the mutex after dropping the spinlocks, since we won't
call release_open_stateid in that case anyway.
We'll also need to consider what happens if nfs4_get_vfs_file fails
after we hashed the stateid, but then another task finds it while
processing another open. So we might have to have release_open_stateid
unlock the mutex after unhashing the stateid, but before putting the
reference, and then have init_open_stateid check to see if the thing is
still hashed after it gets the mutex.
--
Jeff Layton <jlayton@poochiereds.net>
[toc] | [prev] | [next] | [standalone]
| From | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| Date | 2016-06-12 05:20 +0200 |
| Subject | Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file |
| Message-ID | <rJ46l-3jR-23@gated-at.bofh.it> |
| In reply to | #1420135 |
On Jun 11, 2016, at 10:50 PM, Jeff Layton wrote:
> On Sat, 2016-06-11 at 22:06 -0400, Oleg Drokin wrote:
>>
>> Hm. I am trying to lock the newly initialized one and that seems to be holding up
>> well (but I want 24 hours just to be extra sure).
>> Hn, I just noticed a bug in this, so that'll reset the clock back.
>>
>> But I think we cannot return with locked one if we found existing one due to lock
>> inversion?
>> I see that normally first we lock the state rwsem (now mutex) and then
>> lock the fi_lock.
>> Now if we make init_open_stateid() to lock the new state mutex while the fi_lock
>> is locked - that's probably ok, because we can do it before adding it to the list,
>> so nobody can find it.
>> Now the existing state that we find, we cannot really lock while holding that fi_lock,
>> because what if there's a parallel thread that already holds the mutex and now
>> wants the fi_lock?
>> And so it's probably best to return with existing state unlocked and let caller lock it?
>> Or do you think it's best to separately lock the found stp outside of spinlock
>> just for consistency?
>
> I think we just have to ensure that if the new stateid is hashed that
> its mutex is locked prior to being inserted into the hashtable. That
> should prevent the race you mentioned.
>
> If we find an existing one in the hashtable in init_open_stateid, then
> we _can_ take the mutex after dropping the spinlocks, since we won't
> call release_open_stateid in that case anyway.
Yes.
> We'll also need to consider what happens if nfs4_get_vfs_file fails
> after we hashed the stateid, but then another task finds it while
> processing another open. So we might have to have release_open_stateid
> unlock the mutex after unhashing the stateid, but before putting the
> reference, and then have init_open_stateid check to see if the thing is
> still hashed after it gets the mutex.
Hm.
So what's going to go wrong if another user reuses the unhashed stateid?
As long as they drop it once they are done it'll be freed and all is fine, no?
Are there other implications?
Hm, it looks like free_ol_stateid_reaplist() just frees the thing without any looking
into mutexes and stuff?
Ok, so we get the mutex, check that the stateid is hashed, it's not anymore
(actually unhashing could be done without mutex too, right? so just mutex held
is not going to protect us), then we need to drop the mutex and restart the search
from scratch (including all relocking), I assume?
I guess I'll have it as a separate follow on patch.
We'll probably also need some fault-injection here to trigger this case, as triggering
it "naturally" will be a tough problem even on my mega-racy setup.
Something like:
if (swapstp) {
…
}
if (FAULTINJECTION) {
msleep(some_random_time);
status = nfserr_eio;
} else
status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open);
should increase the chance.
Ideally there'd be a way to trigger this case more deterministically,
how do I have two OPEN requests in parallel in NFS for the same file,
just have two threads do it and that would 100% result in two requests,
no merging anywhere along the way that I need to be aware of?
[toc] | [prev] | [next] | [standalone]
| From | Jeff Layton <jlayton@poochiereds.net> |
|---|---|
| Date | 2016-06-12 15:20 +0200 |
| Subject | Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file |
| Message-ID | <rJdt0-GD-13@gated-at.bofh.it> |
| In reply to | #1420151 |
On Sat, 2016-06-11 at 23:15 -0400, Oleg Drokin wrote:
> On Jun 11, 2016, at 10:50 PM, Jeff Layton wrote:
>
> >
> > On Sat, 2016-06-11 at 22:06 -0400, Oleg Drokin wrote:
> > >
> > >
> > > Hm. I am trying to lock the newly initialized one and that seems to be holding up
> > > well (but I want 24 hours just to be extra sure).
> > > Hn, I just noticed a bug in this, so that'll reset the clock back.
> > >
> > > But I think we cannot return with locked one if we found existing one due to lock
> > > inversion?
> > > I see that normally first we lock the state rwsem (now mutex) and then
> > > lock the fi_lock.
> > > Now if we make init_open_stateid() to lock the new state mutex while the fi_lock
> > > is locked - that's probably ok, because we can do it before adding it to the list,
> > > so nobody can find it.
> > > Now the existing state that we find, we cannot really lock while holding that fi_lock,
> > > because what if there's a parallel thread that already holds the mutex and now
> > > wants the fi_lock?
> > > And so it's probably best to return with existing state unlocked and let caller lock it?
> > > Or do you think it's best to separately lock the found stp outside of spinlock
> > > just for consistency?
> > I think we just have to ensure that if the new stateid is hashed that
> > its mutex is locked prior to being inserted into the hashtable. That
> > should prevent the race you mentioned.
> >
> > If we find an existing one in the hashtable in init_open_stateid, then
> > we _can_ take the mutex after dropping the spinlocks, since we won't
> > call release_open_stateid in that case anyway.
> Yes.
>
> >
> > We'll also need to consider what happens if nfs4_get_vfs_file fails
> > after we hashed the stateid, but then another task finds it while
> > processing another open. So we might have to have release_open_stateid
> > unlock the mutex after unhashing the stateid, but before putting the
> > reference, and then have init_open_stateid check to see if the thing is
> > still hashed after it gets the mutex.
> Hm.
> So what's going to go wrong if another user reuses the unhashed stateid?
> As long as they drop it once they are done it'll be freed and all is fine, no?
> Are there other implications?
> Hm, it looks like free_ol_stateid_reaplist() just frees the thing without any looking
> into mutexes and stuff?
>
The problem there is that you're sending a very soon to be defunct
stateid to the client as valid, which means the client will end up in
state recovery (at best).
The initial open is somewhat of a special case...we are hashing a new
stateid. If the operation that hashes it fails, then we want to make
like it never happened at all. The client will never see it, so we
don't want to leave it hanging around on the server. We need to unhash
the stateid in that case.
So, yes, the mutex doesn't really prevent the thing from being
unhashed, but if you take the mutex and the thing is still hashed
afterward, then you know that the above situation didn't happen. The
initial incarnation of the stateid will have been sent to the client.
> Ok, so we get the mutex, check that the stateid is hashed, it's not anymore
> (actually unhashing could be done without mutex too, right? so just mutex held
> is not going to protect us), then we need to drop the mutex and restart the search
> from scratch (including all relocking), I assume?
Yeah, that's what I was thinking. It should be a fairly rare race so
taking an extra hit on the locking in that case shouldn't be too bad. I
think there's quite a bit of opportunity to simplify the open path by
reworking this code as well.
What we probably need to do is turn nfsd4_find_existing_open into
something that intializes and hashes the stateid if it doesn't find one
, and returns it with the mutex locked. It could also do the check to
see if an existing stateid was still hashed after taking the mutex and
could redrive the search if it isn't. That would also make the open
code more resilient in the face of OPEN vs. CLOSE races, which would
also be nice...
> I guess I'll have it as a separate follow on patch.
> We'll probably also need some fault-injection here to trigger this case, as triggering
> it "naturally" will be a tough problem even on my mega-racy setup.
>
> Something like:
> if (swapstp) {
> …
> }
> if (FAULTINJECTION) {
> msleep(some_random_time);
> status = nfserr_eio;
> } else
> status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open);
>
> should increase the chance.
Sure. Look for CONFIG_NFSD_FAULT_INJECTION. You might be able to use
that framework (though it is a bit manky).
> Ideally there'd be a way to trigger this case more deterministically,
> how do I have two OPEN requests in parallel in NFS for the same file,
> just have two threads do it and that would 100% result in two requests,
> no merging anywhere along the way that I need to be aware of?
Yeah, that's basically it. You need two racing OPEN calls for the same
stateid. Might be easiest to do with something like pynfs...
--
Jeff Layton <jlayton@poochiereds.net>
[toc] | [prev] | [next] | [standalone]
| From | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| Date | 2016-06-13 03:30 +0200 |
| Subject | [PATCH v2] nfsd: Always lock state exclusively. |
| Message-ID | <rJoRr-7Qf-1@gated-at.bofh.it> |
| In reply to | #1420151 |
It used to be the case that state had an rwlock that was locked for write
by downgrades, but for read for upgrades (opens). Well, the problem is
if there are two competing opens for the same state, they step on
each other toes potentially leading to leaking file descriptors
from the state structure, since access mode is a bitmap only set once.
Extend the holding region around in nfsd4_process_open2() to avoid
racing entry into nfs4_get_vfs_file().
Make init_open_stateid() return with locked stateid to be unlocked
by the caller.
Now this version held up pretty well in my testing for 24 hours.
It still does not address the situation if during one of the racing
nfs4_get_vfs_file() calls we are getting an error from one (first?)
of them. This is to be addressed in a separate patch after having a
solid reproducer (potentially using some fault injection).
Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
fs/nfsd/nfs4state.c | 47 +++++++++++++++++++++++++++--------------------
fs/nfsd/state.h | 2 +-
2 files changed, 28 insertions(+), 21 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f5f82e1..fa5fb5a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3487,6 +3487,10 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
struct nfs4_openowner *oo = open->op_openowner;
struct nfs4_ol_stateid *retstp = NULL;
+ /* We are moving these outside of the spinlocks to avoid the warnings */
+ mutex_init(&stp->st_mutex);
+ mutex_lock(&stp->st_mutex);
+
spin_lock(&oo->oo_owner.so_client->cl_lock);
spin_lock(&fp->fi_lock);
@@ -3502,13 +3506,14 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
stp->st_access_bmap = 0;
stp->st_deny_bmap = 0;
stp->st_openstp = NULL;
- init_rwsem(&stp->st_rwsem);
list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids);
list_add(&stp->st_perfile, &fp->fi_stateids);
out_unlock:
spin_unlock(&fp->fi_lock);
spin_unlock(&oo->oo_owner.so_client->cl_lock);
+ if (retstp)
+ mutex_lock(&retstp->st_mutex);
return retstp;
}
@@ -4335,32 +4340,34 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
*/
if (stp) {
/* Stateid was found, this is an OPEN upgrade */
- down_read(&stp->st_rwsem);
+ mutex_lock(&stp->st_mutex);
status = nfs4_upgrade_open(rqstp, fp, current_fh, stp, open);
if (status) {
- up_read(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
goto out;
}
} else {
stp = open->op_stp;
open->op_stp = NULL;
+ /*
+ * init_open_stateid() either returns a locked stateid
+ * it found, or initializes and locks the new one we passed in
+ */
swapstp = init_open_stateid(stp, fp, open);
if (swapstp) {
nfs4_put_stid(&stp->st_stid);
stp = swapstp;
- down_read(&stp->st_rwsem);
status = nfs4_upgrade_open(rqstp, fp, current_fh,
stp, open);
if (status) {
- up_read(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
goto out;
}
goto upgrade_out;
}
- down_read(&stp->st_rwsem);
status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open);
if (status) {
- up_read(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
release_open_stateid(stp);
goto out;
}
@@ -4372,7 +4379,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
}
upgrade_out:
nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
- up_read(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
if (nfsd4_has_session(&resp->cstate)) {
if (open->op_deleg_want & NFS4_SHARE_WANT_NO_DELEG) {
@@ -4977,12 +4984,12 @@ static __be32 nfs4_seqid_op_checks(struct nfsd4_compound_state *cstate, stateid_
* revoked delegations are kept only for free_stateid.
*/
return nfserr_bad_stateid;
- down_write(&stp->st_rwsem);
+ mutex_lock(&stp->st_mutex);
status = check_stateid_generation(stateid, &stp->st_stid.sc_stateid, nfsd4_has_session(cstate));
if (status == nfs_ok)
status = nfs4_check_fh(current_fh, &stp->st_stid);
if (status != nfs_ok)
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
return status;
}
@@ -5030,7 +5037,7 @@ static __be32 nfs4_preprocess_confirmed_seqid_op(struct nfsd4_compound_state *cs
return status;
oo = openowner(stp->st_stateowner);
if (!(oo->oo_flags & NFS4_OO_CONFIRMED)) {
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
nfs4_put_stid(&stp->st_stid);
return nfserr_bad_stateid;
}
@@ -5062,12 +5069,12 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
oo = openowner(stp->st_stateowner);
status = nfserr_bad_stateid;
if (oo->oo_flags & NFS4_OO_CONFIRMED) {
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
goto put_stateid;
}
oo->oo_flags |= NFS4_OO_CONFIRMED;
nfs4_inc_and_copy_stateid(&oc->oc_resp_stateid, &stp->st_stid);
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
dprintk("NFSD: %s: success, seqid=%d stateid=" STATEID_FMT "\n",
__func__, oc->oc_seqid, STATEID_VAL(&stp->st_stid.sc_stateid));
@@ -5143,7 +5150,7 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
nfs4_inc_and_copy_stateid(&od->od_stateid, &stp->st_stid);
status = nfs_ok;
put_stateid:
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
nfs4_put_stid(&stp->st_stid);
out:
nfsd4_bump_seqid(cstate, status);
@@ -5196,7 +5203,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (status)
goto out;
nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
nfsd4_close_open_stateid(stp);
@@ -5422,7 +5429,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
stp->st_access_bmap = 0;
stp->st_deny_bmap = open_stp->st_deny_bmap;
stp->st_openstp = open_stp;
- init_rwsem(&stp->st_rwsem);
+ mutex_init(&stp->st_mutex);
list_add(&stp->st_locks, &open_stp->st_locks);
list_add(&stp->st_perstateowner, &lo->lo_owner.so_stateids);
spin_lock(&fp->fi_lock);
@@ -5591,7 +5598,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
&open_stp, nn);
if (status)
goto out;
- up_write(&open_stp->st_rwsem);
+ mutex_unlock(&open_stp->st_mutex);
open_sop = openowner(open_stp->st_stateowner);
status = nfserr_bad_stateid;
if (!same_clid(&open_sop->oo_owner.so_client->cl_clientid,
@@ -5600,7 +5607,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = lookup_or_create_lock_state(cstate, open_stp, lock,
&lock_stp, &new);
if (status == nfs_ok)
- down_write(&lock_stp->st_rwsem);
+ mutex_lock(&lock_stp->st_mutex);
} else {
status = nfs4_preprocess_seqid_op(cstate,
lock->lk_old_lock_seqid,
@@ -5704,7 +5711,7 @@ out:
seqid_mutating_err(ntohl(status)))
lock_sop->lo_owner.so_seqid++;
- up_write(&lock_stp->st_rwsem);
+ mutex_unlock(&lock_stp->st_mutex);
/*
* If this is a new, never-before-used stateid, and we are
@@ -5874,7 +5881,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
fput:
fput(filp);
put_stateid:
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
nfs4_put_stid(&stp->st_stid);
out:
nfsd4_bump_seqid(cstate, status);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 986e51e..64053ea 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -535,7 +535,7 @@ struct nfs4_ol_stateid {
unsigned char st_access_bmap;
unsigned char st_deny_bmap;
struct nfs4_ol_stateid *st_openstp;
- struct rw_semaphore st_rwsem;
+ struct mutex st_mutex;
};
static inline struct nfs4_ol_stateid *openlockstateid(struct nfs4_stid *s)
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | "J . Bruce Fields" <bfields@fieldses.org> |
|---|---|
| Date | 2016-06-14 17:40 +0200 |
| Subject | Re: [PATCH v2] nfsd: Always lock state exclusively. |
| Message-ID | <rJYBz-6HN-9@gated-at.bofh.it> |
| In reply to | #1420402 |
On Sun, Jun 12, 2016 at 09:26:27PM -0400, Oleg Drokin wrote:
> It used to be the case that state had an rwlock that was locked for write
> by downgrades, but for read for upgrades (opens). Well, the problem is
> if there are two competing opens for the same state, they step on
> each other toes potentially leading to leaking file descriptors
> from the state structure, since access mode is a bitmap only set once.
>
> Extend the holding region around in nfsd4_process_open2() to avoid
> racing entry into nfs4_get_vfs_file().
> Make init_open_stateid() return with locked stateid to be unlocked
> by the caller.
>
> Now this version held up pretty well in my testing for 24 hours.
> It still does not address the situation if during one of the racing
> nfs4_get_vfs_file() calls we are getting an error from one (first?)
> of them. This is to be addressed in a separate patch after having a
> solid reproducer (potentially using some fault injection).
>
> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
> ---
> fs/nfsd/nfs4state.c | 47 +++++++++++++++++++++++++++--------------------
> fs/nfsd/state.h | 2 +-
> 2 files changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index f5f82e1..fa5fb5a 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3487,6 +3487,10 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
> struct nfs4_openowner *oo = open->op_openowner;
> struct nfs4_ol_stateid *retstp = NULL;
>
> + /* We are moving these outside of the spinlocks to avoid the warnings */
> + mutex_init(&stp->st_mutex);
> + mutex_lock(&stp->st_mutex);
> +
> spin_lock(&oo->oo_owner.so_client->cl_lock);
> spin_lock(&fp->fi_lock);
>
> @@ -3502,13 +3506,14 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
> stp->st_access_bmap = 0;
> stp->st_deny_bmap = 0;
> stp->st_openstp = NULL;
> - init_rwsem(&stp->st_rwsem);
> list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids);
> list_add(&stp->st_perfile, &fp->fi_stateids);
>
> out_unlock:
> spin_unlock(&fp->fi_lock);
> spin_unlock(&oo->oo_owner.so_client->cl_lock);
> + if (retstp)
> + mutex_lock(&retstp->st_mutex);
> return retstp;
You're returning with both stp->st_mutex and retstp->st_mutex locked.
Did you mean to drop that first lock in the (retstp) case, or am I
missing something?
--b.
> }
>
> @@ -4335,32 +4340,34 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> */
> if (stp) {
> /* Stateid was found, this is an OPEN upgrade */
> - down_read(&stp->st_rwsem);
> + mutex_lock(&stp->st_mutex);
> status = nfs4_upgrade_open(rqstp, fp, current_fh, stp, open);
> if (status) {
> - up_read(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> goto out;
> }
> } else {
> stp = open->op_stp;
> open->op_stp = NULL;
> + /*
> + * init_open_stateid() either returns a locked stateid
> + * it found, or initializes and locks the new one we passed in
> + */
> swapstp = init_open_stateid(stp, fp, open);
> if (swapstp) {
> nfs4_put_stid(&stp->st_stid);
> stp = swapstp;
> - down_read(&stp->st_rwsem);
> status = nfs4_upgrade_open(rqstp, fp, current_fh,
> stp, open);
> if (status) {
> - up_read(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> goto out;
> }
> goto upgrade_out;
> }
> - down_read(&stp->st_rwsem);
> status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open);
> if (status) {
> - up_read(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> release_open_stateid(stp);
> goto out;
> }
> @@ -4372,7 +4379,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> }
> upgrade_out:
> nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
> - up_read(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
>
> if (nfsd4_has_session(&resp->cstate)) {
> if (open->op_deleg_want & NFS4_SHARE_WANT_NO_DELEG) {
> @@ -4977,12 +4984,12 @@ static __be32 nfs4_seqid_op_checks(struct nfsd4_compound_state *cstate, stateid_
> * revoked delegations are kept only for free_stateid.
> */
> return nfserr_bad_stateid;
> - down_write(&stp->st_rwsem);
> + mutex_lock(&stp->st_mutex);
> status = check_stateid_generation(stateid, &stp->st_stid.sc_stateid, nfsd4_has_session(cstate));
> if (status == nfs_ok)
> status = nfs4_check_fh(current_fh, &stp->st_stid);
> if (status != nfs_ok)
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> return status;
> }
>
> @@ -5030,7 +5037,7 @@ static __be32 nfs4_preprocess_confirmed_seqid_op(struct nfsd4_compound_state *cs
> return status;
> oo = openowner(stp->st_stateowner);
> if (!(oo->oo_flags & NFS4_OO_CONFIRMED)) {
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> nfs4_put_stid(&stp->st_stid);
> return nfserr_bad_stateid;
> }
> @@ -5062,12 +5069,12 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> oo = openowner(stp->st_stateowner);
> status = nfserr_bad_stateid;
> if (oo->oo_flags & NFS4_OO_CONFIRMED) {
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> goto put_stateid;
> }
> oo->oo_flags |= NFS4_OO_CONFIRMED;
> nfs4_inc_and_copy_stateid(&oc->oc_resp_stateid, &stp->st_stid);
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> dprintk("NFSD: %s: success, seqid=%d stateid=" STATEID_FMT "\n",
> __func__, oc->oc_seqid, STATEID_VAL(&stp->st_stid.sc_stateid));
>
> @@ -5143,7 +5150,7 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
> nfs4_inc_and_copy_stateid(&od->od_stateid, &stp->st_stid);
> status = nfs_ok;
> put_stateid:
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> nfs4_put_stid(&stp->st_stid);
> out:
> nfsd4_bump_seqid(cstate, status);
> @@ -5196,7 +5203,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> if (status)
> goto out;
> nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
>
> nfsd4_close_open_stateid(stp);
>
> @@ -5422,7 +5429,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
> stp->st_access_bmap = 0;
> stp->st_deny_bmap = open_stp->st_deny_bmap;
> stp->st_openstp = open_stp;
> - init_rwsem(&stp->st_rwsem);
> + mutex_init(&stp->st_mutex);
> list_add(&stp->st_locks, &open_stp->st_locks);
> list_add(&stp->st_perstateowner, &lo->lo_owner.so_stateids);
> spin_lock(&fp->fi_lock);
> @@ -5591,7 +5598,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> &open_stp, nn);
> if (status)
> goto out;
> - up_write(&open_stp->st_rwsem);
> + mutex_unlock(&open_stp->st_mutex);
> open_sop = openowner(open_stp->st_stateowner);
> status = nfserr_bad_stateid;
> if (!same_clid(&open_sop->oo_owner.so_client->cl_clientid,
> @@ -5600,7 +5607,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> status = lookup_or_create_lock_state(cstate, open_stp, lock,
> &lock_stp, &new);
> if (status == nfs_ok)
> - down_write(&lock_stp->st_rwsem);
> + mutex_lock(&lock_stp->st_mutex);
> } else {
> status = nfs4_preprocess_seqid_op(cstate,
> lock->lk_old_lock_seqid,
> @@ -5704,7 +5711,7 @@ out:
> seqid_mutating_err(ntohl(status)))
> lock_sop->lo_owner.so_seqid++;
>
> - up_write(&lock_stp->st_rwsem);
> + mutex_unlock(&lock_stp->st_mutex);
>
> /*
> * If this is a new, never-before-used stateid, and we are
> @@ -5874,7 +5881,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> fput:
> fput(filp);
> put_stateid:
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> nfs4_put_stid(&stp->st_stid);
> out:
> nfsd4_bump_seqid(cstate, status);
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 986e51e..64053ea 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -535,7 +535,7 @@ struct nfs4_ol_stateid {
> unsigned char st_access_bmap;
> unsigned char st_deny_bmap;
> struct nfs4_ol_stateid *st_openstp;
> - struct rw_semaphore st_rwsem;
> + struct mutex st_mutex;
> };
>
> static inline struct nfs4_ol_stateid *openlockstateid(struct nfs4_stid *s)
> --
> 2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| Date | 2016-06-14 18:00 +0200 |
| Subject | Re: [PATCH v2] nfsd: Always lock state exclusively. |
| Message-ID | <rJYUW-6QY-39@gated-at.bofh.it> |
| In reply to | #1422021 |
On Jun 14, 2016, at 11:38 AM, J . Bruce Fields wrote:
> On Sun, Jun 12, 2016 at 09:26:27PM -0400, Oleg Drokin wrote:
>> It used to be the case that state had an rwlock that was locked for write
>> by downgrades, but for read for upgrades (opens). Well, the problem is
>> if there are two competing opens for the same state, they step on
>> each other toes potentially leading to leaking file descriptors
>> from the state structure, since access mode is a bitmap only set once.
>>
>> Extend the holding region around in nfsd4_process_open2() to avoid
>> racing entry into nfs4_get_vfs_file().
>> Make init_open_stateid() return with locked stateid to be unlocked
>> by the caller.
>>
>> Now this version held up pretty well in my testing for 24 hours.
>> It still does not address the situation if during one of the racing
>> nfs4_get_vfs_file() calls we are getting an error from one (first?)
>> of them. This is to be addressed in a separate patch after having a
>> solid reproducer (potentially using some fault injection).
>>
>> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
>> ---
>> fs/nfsd/nfs4state.c | 47 +++++++++++++++++++++++++++--------------------
>> fs/nfsd/state.h | 2 +-
>> 2 files changed, 28 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index f5f82e1..fa5fb5a 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -3487,6 +3487,10 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
>> struct nfs4_openowner *oo = open->op_openowner;
>> struct nfs4_ol_stateid *retstp = NULL;
>>
>> + /* We are moving these outside of the spinlocks to avoid the warnings */
>> + mutex_init(&stp->st_mutex);
>> + mutex_lock(&stp->st_mutex);
>> +
>> spin_lock(&oo->oo_owner.so_client->cl_lock);
>> spin_lock(&fp->fi_lock);
>>
>> @@ -3502,13 +3506,14 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
>> stp->st_access_bmap = 0;
>> stp->st_deny_bmap = 0;
>> stp->st_openstp = NULL;
>> - init_rwsem(&stp->st_rwsem);
>> list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids);
>> list_add(&stp->st_perfile, &fp->fi_stateids);
>>
>> out_unlock:
>> spin_unlock(&fp->fi_lock);
>> spin_unlock(&oo->oo_owner.so_client->cl_lock);
>> + if (retstp)
>> + mutex_lock(&retstp->st_mutex);
>> return retstp;
>
> You're returning with both stp->st_mutex and retstp->st_mutex locked.
> Did you mean to drop that first lock in the (retstp) case, or am I
> missing something?
Well, I think it's ok (perhaps worthy of a comment) it's that if we matched a different
retstp state, then stp is not used and either released right away or even
if reused, it would be reinitialized in another call to init_open_stateid(),
so it's fine?
>
> --b.
>
>> }
>>
>> @@ -4335,32 +4340,34 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
>> */
>> if (stp) {
>> /* Stateid was found, this is an OPEN upgrade */
>> - down_read(&stp->st_rwsem);
>> + mutex_lock(&stp->st_mutex);
>> status = nfs4_upgrade_open(rqstp, fp, current_fh, stp, open);
>> if (status) {
>> - up_read(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> goto out;
>> }
>> } else {
>> stp = open->op_stp;
>> open->op_stp = NULL;
>> + /*
>> + * init_open_stateid() either returns a locked stateid
>> + * it found, or initializes and locks the new one we passed in
>> + */
>> swapstp = init_open_stateid(stp, fp, open);
>> if (swapstp) {
>> nfs4_put_stid(&stp->st_stid);
>> stp = swapstp;
>> - down_read(&stp->st_rwsem);
>> status = nfs4_upgrade_open(rqstp, fp, current_fh,
>> stp, open);
>> if (status) {
>> - up_read(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> goto out;
>> }
>> goto upgrade_out;
>> }
>> - down_read(&stp->st_rwsem);
>> status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open);
>> if (status) {
>> - up_read(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> release_open_stateid(stp);
>> goto out;
>> }
>> @@ -4372,7 +4379,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
>> }
>> upgrade_out:
>> nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
>> - up_read(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>>
>> if (nfsd4_has_session(&resp->cstate)) {
>> if (open->op_deleg_want & NFS4_SHARE_WANT_NO_DELEG) {
>> @@ -4977,12 +4984,12 @@ static __be32 nfs4_seqid_op_checks(struct nfsd4_compound_state *cstate, stateid_
>> * revoked delegations are kept only for free_stateid.
>> */
>> return nfserr_bad_stateid;
>> - down_write(&stp->st_rwsem);
>> + mutex_lock(&stp->st_mutex);
>> status = check_stateid_generation(stateid, &stp->st_stid.sc_stateid, nfsd4_has_session(cstate));
>> if (status == nfs_ok)
>> status = nfs4_check_fh(current_fh, &stp->st_stid);
>> if (status != nfs_ok)
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> return status;
>> }
>>
>> @@ -5030,7 +5037,7 @@ static __be32 nfs4_preprocess_confirmed_seqid_op(struct nfsd4_compound_state *cs
>> return status;
>> oo = openowner(stp->st_stateowner);
>> if (!(oo->oo_flags & NFS4_OO_CONFIRMED)) {
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> nfs4_put_stid(&stp->st_stid);
>> return nfserr_bad_stateid;
>> }
>> @@ -5062,12 +5069,12 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> oo = openowner(stp->st_stateowner);
>> status = nfserr_bad_stateid;
>> if (oo->oo_flags & NFS4_OO_CONFIRMED) {
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> goto put_stateid;
>> }
>> oo->oo_flags |= NFS4_OO_CONFIRMED;
>> nfs4_inc_and_copy_stateid(&oc->oc_resp_stateid, &stp->st_stid);
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> dprintk("NFSD: %s: success, seqid=%d stateid=" STATEID_FMT "\n",
>> __func__, oc->oc_seqid, STATEID_VAL(&stp->st_stid.sc_stateid));
>>
>> @@ -5143,7 +5150,7 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
>> nfs4_inc_and_copy_stateid(&od->od_stateid, &stp->st_stid);
>> status = nfs_ok;
>> put_stateid:
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> nfs4_put_stid(&stp->st_stid);
>> out:
>> nfsd4_bump_seqid(cstate, status);
>> @@ -5196,7 +5203,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> if (status)
>> goto out;
>> nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>>
>> nfsd4_close_open_stateid(stp);
>>
>> @@ -5422,7 +5429,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
>> stp->st_access_bmap = 0;
>> stp->st_deny_bmap = open_stp->st_deny_bmap;
>> stp->st_openstp = open_stp;
>> - init_rwsem(&stp->st_rwsem);
>> + mutex_init(&stp->st_mutex);
>> list_add(&stp->st_locks, &open_stp->st_locks);
>> list_add(&stp->st_perstateowner, &lo->lo_owner.so_stateids);
>> spin_lock(&fp->fi_lock);
>> @@ -5591,7 +5598,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> &open_stp, nn);
>> if (status)
>> goto out;
>> - up_write(&open_stp->st_rwsem);
>> + mutex_unlock(&open_stp->st_mutex);
>> open_sop = openowner(open_stp->st_stateowner);
>> status = nfserr_bad_stateid;
>> if (!same_clid(&open_sop->oo_owner.so_client->cl_clientid,
>> @@ -5600,7 +5607,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> status = lookup_or_create_lock_state(cstate, open_stp, lock,
>> &lock_stp, &new);
>> if (status == nfs_ok)
>> - down_write(&lock_stp->st_rwsem);
>> + mutex_lock(&lock_stp->st_mutex);
>> } else {
>> status = nfs4_preprocess_seqid_op(cstate,
>> lock->lk_old_lock_seqid,
>> @@ -5704,7 +5711,7 @@ out:
>> seqid_mutating_err(ntohl(status)))
>> lock_sop->lo_owner.so_seqid++;
>>
>> - up_write(&lock_stp->st_rwsem);
>> + mutex_unlock(&lock_stp->st_mutex);
>>
>> /*
>> * If this is a new, never-before-used stateid, and we are
>> @@ -5874,7 +5881,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> fput:
>> fput(filp);
>> put_stateid:
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> nfs4_put_stid(&stp->st_stid);
>> out:
>> nfsd4_bump_seqid(cstate, status);
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index 986e51e..64053ea 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -535,7 +535,7 @@ struct nfs4_ol_stateid {
>> unsigned char st_access_bmap;
>> unsigned char st_deny_bmap;
>> struct nfs4_ol_stateid *st_openstp;
>> - struct rw_semaphore st_rwsem;
>> + struct mutex st_mutex;
>> };
>>
>> static inline struct nfs4_ol_stateid *openlockstateid(struct nfs4_stid *s)
>> --
>> 2.7.4
[toc] | [prev] | [next] | [standalone]
| From | "J . Bruce Fields" <bfields@fieldses.org> |
|---|---|
| Date | 2016-06-14 21:00 +0200 |
| Subject | Re: [PATCH v2] nfsd: Always lock state exclusively. |
| Message-ID | <rK1J8-fP-39@gated-at.bofh.it> |
| In reply to | #1422046 |
On Tue, Jun 14, 2016 at 11:53:27AM -0400, Oleg Drokin wrote: > > On Jun 14, 2016, at 11:38 AM, J . Bruce Fields wrote: > > > On Sun, Jun 12, 2016 at 09:26:27PM -0400, Oleg Drokin wrote: > >> It used to be the case that state had an rwlock that was locked for write > >> by downgrades, but for read for upgrades (opens). Well, the problem is > >> if there are two competing opens for the same state, they step on > >> each other toes potentially leading to leaking file descriptors > >> from the state structure, since access mode is a bitmap only set once. > >> > >> Extend the holding region around in nfsd4_process_open2() to avoid > >> racing entry into nfs4_get_vfs_file(). > >> Make init_open_stateid() return with locked stateid to be unlocked > >> by the caller. > >> > >> Now this version held up pretty well in my testing for 24 hours. > >> It still does not address the situation if during one of the racing > >> nfs4_get_vfs_file() calls we are getting an error from one (first?) > >> of them. This is to be addressed in a separate patch after having a > >> solid reproducer (potentially using some fault injection). > >> > >> Signed-off-by: Oleg Drokin <green@linuxhacker.ru> > >> --- > >> fs/nfsd/nfs4state.c | 47 +++++++++++++++++++++++++++-------------------- > >> fs/nfsd/state.h | 2 +- > >> 2 files changed, 28 insertions(+), 21 deletions(-) > >> > >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > >> index f5f82e1..fa5fb5a 100644 > >> --- a/fs/nfsd/nfs4state.c > >> +++ b/fs/nfsd/nfs4state.c > >> @@ -3487,6 +3487,10 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp, > >> struct nfs4_openowner *oo = open->op_openowner; > >> struct nfs4_ol_stateid *retstp = NULL; > >> > >> + /* We are moving these outside of the spinlocks to avoid the warnings */ > >> + mutex_init(&stp->st_mutex); > >> + mutex_lock(&stp->st_mutex); > >> + > >> spin_lock(&oo->oo_owner.so_client->cl_lock); > >> spin_lock(&fp->fi_lock); > >> > >> @@ -3502,13 +3506,14 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp, > >> stp->st_access_bmap = 0; > >> stp->st_deny_bmap = 0; > >> stp->st_openstp = NULL; > >> - init_rwsem(&stp->st_rwsem); > >> list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids); > >> list_add(&stp->st_perfile, &fp->fi_stateids); > >> > >> out_unlock: > >> spin_unlock(&fp->fi_lock); > >> spin_unlock(&oo->oo_owner.so_client->cl_lock); > >> + if (retstp) > >> + mutex_lock(&retstp->st_mutex); > >> return retstp; > > > > You're returning with both stp->st_mutex and retstp->st_mutex locked. > > Did you mean to drop that first lock in the (retstp) case, or am I > > missing something? > > Well, I think it's ok (perhaps worthy of a comment) it's that if we matched a different > retstp state, then stp is not used and either released right away or even > if reused, it would be reinitialized in another call to init_open_stateid(), > so it's fine? Oh, I see, you're right. Though I wouldn't have been surprised if that triggered some kind of warning--I guess it's OK here, but typically if I saw a structure freed that had a locked lock in it I'd be a little suspicious that somebody made a mistake. --b.
[toc] | [prev] | [next] | [standalone]
| From | Jeff Layton <jlayton@poochiereds.net> |
|---|---|
| Date | 2016-06-15 01:00 +0200 |
| Subject | Re: [PATCH v2] nfsd: Always lock state exclusively. |
| Message-ID | <rK5tn-2E2-3@gated-at.bofh.it> |
| In reply to | #1422235 |
On Tue, 2016-06-14 at 18:54 -0400, Oleg Drokin wrote: > On Jun 14, 2016, at 6:52 PM, Jeff Layton wrote: > > > > I think I'd still prefer to have it unlock the mutex in the event that > > it's not going to use it after all. While that kind of thing is ok for > > now, it's stuff like that that can turn into a subtle source of bugs > > later. > > > > Also, I think I'd be more comfortable with this being split into (at > > least) two patches. Do one patch as a straight conversion from rwsem to > > mutex, and then another that changes the code to take the mutex before > > hashing the new stateid. > Ok, I guess that could be arranged too. > > And then there's this Bruce's patch to pull more stuff into the init_open_stateid Yeah, that seems like a good idea. We _really_ need an effort to simplify this code. OPEN handling is always messy, but the current code is really much messier than it should be. -- Jeff Layton <jlayton@poochiereds.net>
[toc] | [prev] | [next] | [standalone]
| From | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| Date | 2016-06-15 05:30 +0200 |
| Subject | [PATCH 3/3] nfsd: Make init_open_stateid() a bit more whole |
| Message-ID | <rK9GG-5v1-27@gated-at.bofh.it> |
| In reply to | #1422434 |
Move the state selection logic inside from the caller,
always making it return correct stp to use.
Signed-off-by: J . Bruce Fields <bfields@fieldses.org>
Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
fs/nfsd/nfs4state.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 94854a0..70d0b9b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3480,13 +3480,14 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
}
static struct nfs4_ol_stateid *
-init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
- struct nfsd4_open *open)
+init_open_stateid(struct nfs4_file *fp, struct nfsd4_open *open)
{
struct nfs4_openowner *oo = open->op_openowner;
struct nfs4_ol_stateid *retstp = NULL;
+ struct nfs4_ol_stateid *stp;
+ stp = open->op_stp;
/* We are moving these outside of the spinlocks to avoid the warnings */
mutex_init(&stp->st_mutex);
mutex_lock(&stp->st_mutex);
@@ -3497,6 +3498,8 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
retstp = nfsd4_find_existing_open(fp, open);
if (retstp)
goto out_unlock;
+
+ open->op_stp = NULL;
atomic_inc(&stp->st_stid.sc_count);
stp->st_stid.sc_type = NFS4_OPEN_STID;
INIT_LIST_HEAD(&stp->st_locks);
@@ -3514,10 +3517,11 @@ out_unlock:
spin_unlock(&oo->oo_owner.so_client->cl_lock);
if (retstp) {
mutex_lock(&retstp->st_mutex);
- /* Not that we need to, just for neatness */
+ /* To keep mutex tracking happy */
mutex_unlock(&stp->st_mutex);
+ stp = retstp;
}
- return retstp;
+ return stp;
}
/*
@@ -4313,7 +4317,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
struct nfs4_client *cl = open->op_openowner->oo_owner.so_client;
struct nfs4_file *fp = NULL;
struct nfs4_ol_stateid *stp = NULL;
- struct nfs4_ol_stateid *swapstp = NULL;
struct nfs4_delegation *dp = NULL;
__be32 status;
@@ -4350,16 +4353,10 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
goto out;
}
} else {
- stp = open->op_stp;
- open->op_stp = NULL;
- /*
- * init_open_stateid() either returns a locked stateid
- * it found, or initializes and locks the new one we passed in
- */
- swapstp = init_open_stateid(stp, fp, open);
- if (swapstp) {
- nfs4_put_stid(&stp->st_stid);
- stp = swapstp;
+ /* stp is returned locked. */
+ stp = init_open_stateid(fp, open);
+ /* See if we lost the race to some other thread */
+ if (stp->st_access_bmap != 0) {
status = nfs4_upgrade_open(rqstp, fp, current_fh,
stp, open);
if (status) {
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| Date | 2016-06-15 05:30 +0200 |
| Subject | [PATCH 0/3] nfsd state handling fixes |
| Message-ID | <rK9GG-5v1-29@gated-at.bofh.it> |
| In reply to | #1422434 |
These three patches do the much discussed job of making nfsd state handling more robust in face of races where several opens arrive for the same file at the same time from the same client. This does not yet handle a case when one of those opens gets an error and others don't. Also this is undergoing testing ATM, so please only use it as a discussion/review piece for now. Oleg Drokin (3): nfsd: Always lock state exclusively. nfsd: Extend the mutex holding region around in nfsd4_process_open2() nfsd: Make init_open_stateid() a bit more whole fs/nfsd/nfs4state.c | 67 +++++++++++++++++++++++++++++------------------------ fs/nfsd/state.h | 2 +- 2 files changed, 38 insertions(+), 31 deletions(-) -- 2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| Date | 2016-06-15 05:40 +0200 |
| Subject | [PATCH 2/3] nfsd: Extend the mutex holding region around in nfsd4_process_open2() |
| Message-ID | <rK9Ql-5yd-9@gated-at.bofh.it> |
| In reply to | #1422546 |
To avoid racing entry into nfs4_get_vfs_file().
Make init_open_stateid() return with locked stateid to be unlocked
by the caller.
Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
fs/nfsd/nfs4state.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c927d36..94854a0 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3487,6 +3487,10 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
struct nfs4_openowner *oo = open->op_openowner;
struct nfs4_ol_stateid *retstp = NULL;
+ /* We are moving these outside of the spinlocks to avoid the warnings */
+ mutex_init(&stp->st_mutex);
+ mutex_lock(&stp->st_mutex);
+
spin_lock(&oo->oo_owner.so_client->cl_lock);
spin_lock(&fp->fi_lock);
@@ -3502,13 +3506,17 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
stp->st_access_bmap = 0;
stp->st_deny_bmap = 0;
stp->st_openstp = NULL;
- mutex_init(&stp->st_mutex);
list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids);
list_add(&stp->st_perfile, &fp->fi_stateids);
out_unlock:
spin_unlock(&fp->fi_lock);
spin_unlock(&oo->oo_owner.so_client->cl_lock);
+ if (retstp) {
+ mutex_lock(&retstp->st_mutex);
+ /* Not that we need to, just for neatness */
+ mutex_unlock(&stp->st_mutex);
+ }
return retstp;
}
@@ -4344,11 +4352,14 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
} else {
stp = open->op_stp;
open->op_stp = NULL;
+ /*
+ * init_open_stateid() either returns a locked stateid
+ * it found, or initializes and locks the new one we passed in
+ */
swapstp = init_open_stateid(stp, fp, open);
if (swapstp) {
nfs4_put_stid(&stp->st_stid);
stp = swapstp;
- mutex_lock(&stp->st_mutex);
status = nfs4_upgrade_open(rqstp, fp, current_fh,
stp, open);
if (status) {
@@ -4357,7 +4368,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
}
goto upgrade_out;
}
- mutex_lock(&stp->st_mutex);
status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open);
if (status) {
mutex_unlock(&stp->st_mutex);
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| Date | 2016-06-15 05:40 +0200 |
| Subject | [PATCH 1/3] nfsd: Always lock state exclusively. |
| Message-ID | <rK9Qm-5yd-17@gated-at.bofh.it> |
| In reply to | #1422546 |
It used to be the case that state had an rwlock that was locked for write
by downgrades, but for read for upgrades (opens). Well, the problem is
if there are two competing opens for the same state, they step on
each other toes potentially leading to leaking file descriptors
from the state structure, since access mode is a bitmap only set once.
Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
fs/nfsd/nfs4state.c | 40 ++++++++++++++++++++--------------------
fs/nfsd/state.h | 2 +-
2 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f5f82e1..c927d36 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3502,7 +3502,7 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
stp->st_access_bmap = 0;
stp->st_deny_bmap = 0;
stp->st_openstp = NULL;
- init_rwsem(&stp->st_rwsem);
+ mutex_init(&stp->st_mutex);
list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids);
list_add(&stp->st_perfile, &fp->fi_stateids);
@@ -4335,10 +4335,10 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
*/
if (stp) {
/* Stateid was found, this is an OPEN upgrade */
- down_read(&stp->st_rwsem);
+ mutex_lock(&stp->st_mutex);
status = nfs4_upgrade_open(rqstp, fp, current_fh, stp, open);
if (status) {
- up_read(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
goto out;
}
} else {
@@ -4348,19 +4348,19 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
if (swapstp) {
nfs4_put_stid(&stp->st_stid);
stp = swapstp;
- down_read(&stp->st_rwsem);
+ mutex_lock(&stp->st_mutex);
status = nfs4_upgrade_open(rqstp, fp, current_fh,
stp, open);
if (status) {
- up_read(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
goto out;
}
goto upgrade_out;
}
- down_read(&stp->st_rwsem);
+ mutex_lock(&stp->st_mutex);
status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open);
if (status) {
- up_read(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
release_open_stateid(stp);
goto out;
}
@@ -4372,7 +4372,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
}
upgrade_out:
nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
- up_read(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
if (nfsd4_has_session(&resp->cstate)) {
if (open->op_deleg_want & NFS4_SHARE_WANT_NO_DELEG) {
@@ -4977,12 +4977,12 @@ static __be32 nfs4_seqid_op_checks(struct nfsd4_compound_state *cstate, stateid_
* revoked delegations are kept only for free_stateid.
*/
return nfserr_bad_stateid;
- down_write(&stp->st_rwsem);
+ mutex_lock(&stp->st_mutex);
status = check_stateid_generation(stateid, &stp->st_stid.sc_stateid, nfsd4_has_session(cstate));
if (status == nfs_ok)
status = nfs4_check_fh(current_fh, &stp->st_stid);
if (status != nfs_ok)
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
return status;
}
@@ -5030,7 +5030,7 @@ static __be32 nfs4_preprocess_confirmed_seqid_op(struct nfsd4_compound_state *cs
return status;
oo = openowner(stp->st_stateowner);
if (!(oo->oo_flags & NFS4_OO_CONFIRMED)) {
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
nfs4_put_stid(&stp->st_stid);
return nfserr_bad_stateid;
}
@@ -5062,12 +5062,12 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
oo = openowner(stp->st_stateowner);
status = nfserr_bad_stateid;
if (oo->oo_flags & NFS4_OO_CONFIRMED) {
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
goto put_stateid;
}
oo->oo_flags |= NFS4_OO_CONFIRMED;
nfs4_inc_and_copy_stateid(&oc->oc_resp_stateid, &stp->st_stid);
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
dprintk("NFSD: %s: success, seqid=%d stateid=" STATEID_FMT "\n",
__func__, oc->oc_seqid, STATEID_VAL(&stp->st_stid.sc_stateid));
@@ -5143,7 +5143,7 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
nfs4_inc_and_copy_stateid(&od->od_stateid, &stp->st_stid);
status = nfs_ok;
put_stateid:
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
nfs4_put_stid(&stp->st_stid);
out:
nfsd4_bump_seqid(cstate, status);
@@ -5196,7 +5196,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (status)
goto out;
nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
nfsd4_close_open_stateid(stp);
@@ -5422,7 +5422,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
stp->st_access_bmap = 0;
stp->st_deny_bmap = open_stp->st_deny_bmap;
stp->st_openstp = open_stp;
- init_rwsem(&stp->st_rwsem);
+ mutex_init(&stp->st_mutex);
list_add(&stp->st_locks, &open_stp->st_locks);
list_add(&stp->st_perstateowner, &lo->lo_owner.so_stateids);
spin_lock(&fp->fi_lock);
@@ -5591,7 +5591,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
&open_stp, nn);
if (status)
goto out;
- up_write(&open_stp->st_rwsem);
+ mutex_unlock(&open_stp->st_mutex);
open_sop = openowner(open_stp->st_stateowner);
status = nfserr_bad_stateid;
if (!same_clid(&open_sop->oo_owner.so_client->cl_clientid,
@@ -5600,7 +5600,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = lookup_or_create_lock_state(cstate, open_stp, lock,
&lock_stp, &new);
if (status == nfs_ok)
- down_write(&lock_stp->st_rwsem);
+ mutex_lock(&lock_stp->st_mutex);
} else {
status = nfs4_preprocess_seqid_op(cstate,
lock->lk_old_lock_seqid,
@@ -5704,7 +5704,7 @@ out:
seqid_mutating_err(ntohl(status)))
lock_sop->lo_owner.so_seqid++;
- up_write(&lock_stp->st_rwsem);
+ mutex_unlock(&lock_stp->st_mutex);
/*
* If this is a new, never-before-used stateid, and we are
@@ -5874,7 +5874,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
fput:
fput(filp);
put_stateid:
- up_write(&stp->st_rwsem);
+ mutex_unlock(&stp->st_mutex);
nfs4_put_stid(&stp->st_stid);
out:
nfsd4_bump_seqid(cstate, status);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 986e51e..64053ea 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -535,7 +535,7 @@ struct nfs4_ol_stateid {
unsigned char st_access_bmap;
unsigned char st_deny_bmap;
struct nfs4_ol_stateid *st_openstp;
- struct rw_semaphore st_rwsem;
+ struct mutex st_mutex;
};
static inline struct nfs4_ol_stateid *openlockstateid(struct nfs4_stid *s)
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| Date | 2016-06-16 04:00 +0200 |
| Subject | Re: [PATCH 0/3] nfsd state handling fixes |
| Message-ID | <rKuL7-1Ve-13@gated-at.bofh.it> |
| In reply to | #1422546 |
On Jun 14, 2016, at 11:28 PM, Oleg Drokin wrote: > These three patches do the much discussed job of making nfsd state handling > more robust in face of races where several opens arrive for the same file > at the same time from the same client. > > This does not yet handle a case when one of those opens gets an error > and others don't. > > Also this is undergoing testing ATM, so please only use it as a > discussion/review piece for now. With 24 hours in testing an no problems encountered, I guess it's safe to declare this patchset as good to go if nobody has any objections against it. > > Oleg Drokin (3): > nfsd: Always lock state exclusively. > nfsd: Extend the mutex holding region around in nfsd4_process_open2() > nfsd: Make init_open_stateid() a bit more whole > > fs/nfsd/nfs4state.c | 67 +++++++++++++++++++++++++++++------------------------ > fs/nfsd/state.h | 2 +- > 2 files changed, 38 insertions(+), 31 deletions(-) > > -- > 2.7.4
[toc] | [prev] | [next] | [standalone]
| From | "J . Bruce Fields" <bfields@fieldses.org> |
|---|---|
| Date | 2016-06-16 04:10 +0200 |
| Subject | Re: [PATCH 0/3] nfsd state handling fixes |
| Message-ID | <rKuUN-2dr-3@gated-at.bofh.it> |
| In reply to | #1423636 |
On Wed, Jun 15, 2016 at 09:54:28PM -0400, Oleg Drokin wrote: > > On Jun 14, 2016, at 11:28 PM, Oleg Drokin wrote: > > > These three patches do the much discussed job of making nfsd state handling > > more robust in face of races where several opens arrive for the same file > > at the same time from the same client. > > > > This does not yet handle a case when one of those opens gets an error > > and others don't. > > > > Also this is undergoing testing ATM, so please only use it as a > > discussion/review piece for now. > > With 24 hours in testing an no problems encountered, I guess > it's safe to declare this patchset as good to go if nobody has any > objections against it. Great, thanks for the testing, and all the work tracking this down; committing for 4.7 (and the first two for stable). --b. > > > > > Oleg Drokin (3): > > nfsd: Always lock state exclusively. > > nfsd: Extend the mutex holding region around in nfsd4_process_open2() > > nfsd: Make init_open_stateid() a bit more whole > > > > fs/nfsd/nfs4state.c | 67 +++++++++++++++++++++++++++++------------------------ > > fs/nfsd/state.h | 2 +- > > 2 files changed, 38 insertions(+), 31 deletions(-) > > > > -- > > 2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| Date | 2016-06-15 01:00 +0200 |
| Subject | Re: [PATCH v2] nfsd: Always lock state exclusively. |
| Message-ID | <rK5tn-2E2-7@gated-at.bofh.it> |
| In reply to | #1422235 |
On Jun 14, 2016, at 6:52 PM, Jeff Layton wrote: > I think I'd still prefer to have it unlock the mutex in the event that > it's not going to use it after all. While that kind of thing is ok for > now, it's stuff like that that can turn into a subtle source of bugs > later. > > Also, I think I'd be more comfortable with this being split into (at > least) two patches. Do one patch as a straight conversion from rwsem to > mutex, and then another that changes the code to take the mutex before > hashing the new stateid. Ok, I guess that could be arranged too. And then there's this Bruce's patch to pull more stuff into the init_open_stateid
[toc] | [prev] | [next] | [standalone]
| From | Jeff Layton <jlayton@poochiereds.net> |
|---|---|
| Date | 2016-06-15 01:00 +0200 |
| Subject | Re: [PATCH v2] nfsd: Always lock state exclusively. |
| Message-ID | <rK5tn-2E2-5@gated-at.bofh.it> |
| In reply to | #1422235 |
On Tue, 2016-06-14 at 14:50 -0400, J . Bruce Fields wrote: > On Tue, Jun 14, 2016 at 11:53:27AM -0400, Oleg Drokin wrote: > > > > > > On Jun 14, 2016, at 11:38 AM, J . Bruce Fields wrote: > > > > > > > > On Sun, Jun 12, 2016 at 09:26:27PM -0400, Oleg Drokin wrote: > > > > > > > > It used to be the case that state had an rwlock that was locked for write > > > > by downgrades, but for read for upgrades (opens). Well, the problem is > > > > if there are two competing opens for the same state, they step on > > > > each other toes potentially leading to leaking file descriptors > > > > from the state structure, since access mode is a bitmap only set once. > > > > > > > > Extend the holding region around in nfsd4_process_open2() to avoid > > > > racing entry into nfs4_get_vfs_file(). > > > > Make init_open_stateid() return with locked stateid to be unlocked > > > > by the caller. > > > > > > > > Now this version held up pretty well in my testing for 24 hours. > > > > It still does not address the situation if during one of the racing > > > > nfs4_get_vfs_file() calls we are getting an error from one (first?) > > > > of them. This is to be addressed in a separate patch after having a > > > > solid reproducer (potentially using some fault injection). > > > > > > > > Signed-off-by: Oleg Drokin <green@linuxhacker.ru> > > > > --- > > > > fs/nfsd/nfs4state.c | 47 +++++++++++++++++++++++++++-------------------- > > > > fs/nfsd/state.h | 2 +- > > > > 2 files changed, 28 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > > > index f5f82e1..fa5fb5a 100644 > > > > --- a/fs/nfsd/nfs4state.c > > > > +++ b/fs/nfsd/nfs4state.c > > > > @@ -3487,6 +3487,10 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp, > > > > struct nfs4_openowner *oo = open->op_openowner; > > > > struct nfs4_ol_stateid *retstp = NULL; > > > > > > > > + /* We are moving these outside of the spinlocks to avoid the warnings */ > > > > + mutex_init(&stp->st_mutex); > > > > + mutex_lock(&stp->st_mutex); > > > > + > > > > spin_lock(&oo->oo_owner.so_client->cl_lock); > > > > spin_lock(&fp->fi_lock); > > > > > > > > @@ -3502,13 +3506,14 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp, > > > > stp->st_access_bmap = 0; > > > > stp->st_deny_bmap = 0; > > > > stp->st_openstp = NULL; > > > > - init_rwsem(&stp->st_rwsem); > > > > list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids); > > > > list_add(&stp->st_perfile, &fp->fi_stateids); > > > > > > > > out_unlock: > > > > spin_unlock(&fp->fi_lock); > > > > spin_unlock(&oo->oo_owner.so_client->cl_lock); > > > > + if (retstp) > > > > + mutex_lock(&retstp->st_mutex); > > > > return retstp; > > > You're returning with both stp->st_mutex and retstp->st_mutex locked. > > > Did you mean to drop that first lock in the (retstp) case, or am I > > > missing something? > > Well, I think it's ok (perhaps worthy of a comment) it's that if we matched a different > > retstp state, then stp is not used and either released right away or even > > if reused, it would be reinitialized in another call to init_open_stateid(), > > so it's fine? > Oh, I see, you're right. > > Though I wouldn't have been surprised if that triggered some kind of > warning--I guess it's OK here, but typically if I saw a structure freed > that had a locked lock in it I'd be a little suspicious that somebody > made a mistake. > > --b. I think I'd still prefer to have it unlock the mutex in the event that it's not going to use it after all. While that kind of thing is ok for now, it's stuff like that that can turn into a subtle source of bugs later. Also, I think I'd be more comfortable with this being split into (at least) two patches. Do one patch as a straight conversion from rwsem to mutex, and then another that changes the code to take the mutex before hashing the new stateid. -- Jeff Layton <jlayton@poochiereds.net>
[toc] | [prev] | [next] | [standalone]
| From | "J . Bruce Fields" <bfields@fieldses.org> |
|---|---|
| Date | 2016-06-14 17:50 +0200 |
| Subject | Re: [PATCH v2] nfsd: Always lock state exclusively. |
| Message-ID | <rJYLg-6MM-7@gated-at.bofh.it> |
| In reply to | #1420402 |
On Sun, Jun 12, 2016 at 09:26:27PM -0400, Oleg Drokin wrote:
> It used to be the case that state had an rwlock that was locked for write
> by downgrades, but for read for upgrades (opens). Well, the problem is
> if there are two competing opens for the same state, they step on
> each other toes potentially leading to leaking file descriptors
> from the state structure, since access mode is a bitmap only set once.
>
> Extend the holding region around in nfsd4_process_open2() to avoid
> racing entry into nfs4_get_vfs_file().
> Make init_open_stateid() return with locked stateid to be unlocked
> by the caller.
>
> Now this version held up pretty well in my testing for 24 hours.
> It still does not address the situation if during one of the racing
> nfs4_get_vfs_file() calls we are getting an error from one (first?)
> of them. This is to be addressed in a separate patch after having a
> solid reproducer (potentially using some fault injection).
>
> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
> ---
> fs/nfsd/nfs4state.c | 47 +++++++++++++++++++++++++++--------------------
> fs/nfsd/state.h | 2 +-
> 2 files changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index f5f82e1..fa5fb5a 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3487,6 +3487,10 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
> struct nfs4_openowner *oo = open->op_openowner;
> struct nfs4_ol_stateid *retstp = NULL;
>
> + /* We are moving these outside of the spinlocks to avoid the warnings */
> + mutex_init(&stp->st_mutex);
> + mutex_lock(&stp->st_mutex);
A mutex_init_locked() primitive might also be convenient here.
You could also take the two previous lines from the caller into this
function instead of passing in stp, that might simplify the code.
(Haven't checked.)
--b.
> +
> spin_lock(&oo->oo_owner.so_client->cl_lock);
> spin_lock(&fp->fi_lock);
>
> @@ -3502,13 +3506,14 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
> stp->st_access_bmap = 0;
> stp->st_deny_bmap = 0;
> stp->st_openstp = NULL;
> - init_rwsem(&stp->st_rwsem);
> list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids);
> list_add(&stp->st_perfile, &fp->fi_stateids);
>
> out_unlock:
> spin_unlock(&fp->fi_lock);
> spin_unlock(&oo->oo_owner.so_client->cl_lock);
> + if (retstp)
> + mutex_lock(&retstp->st_mutex);
> return retstp;
> }
>
> @@ -4335,32 +4340,34 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> */
> if (stp) {
> /* Stateid was found, this is an OPEN upgrade */
> - down_read(&stp->st_rwsem);
> + mutex_lock(&stp->st_mutex);
> status = nfs4_upgrade_open(rqstp, fp, current_fh, stp, open);
> if (status) {
> - up_read(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> goto out;
> }
> } else {
> stp = open->op_stp;
> open->op_stp = NULL;
> + /*
> + * init_open_stateid() either returns a locked stateid
> + * it found, or initializes and locks the new one we passed in
> + */
> swapstp = init_open_stateid(stp, fp, open);
> if (swapstp) {
> nfs4_put_stid(&stp->st_stid);
> stp = swapstp;
> - down_read(&stp->st_rwsem);
> status = nfs4_upgrade_open(rqstp, fp, current_fh,
> stp, open);
> if (status) {
> - up_read(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> goto out;
> }
> goto upgrade_out;
> }
> - down_read(&stp->st_rwsem);
> status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open);
> if (status) {
> - up_read(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> release_open_stateid(stp);
> goto out;
> }
> @@ -4372,7 +4379,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> }
> upgrade_out:
> nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
> - up_read(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
>
> if (nfsd4_has_session(&resp->cstate)) {
> if (open->op_deleg_want & NFS4_SHARE_WANT_NO_DELEG) {
> @@ -4977,12 +4984,12 @@ static __be32 nfs4_seqid_op_checks(struct nfsd4_compound_state *cstate, stateid_
> * revoked delegations are kept only for free_stateid.
> */
> return nfserr_bad_stateid;
> - down_write(&stp->st_rwsem);
> + mutex_lock(&stp->st_mutex);
> status = check_stateid_generation(stateid, &stp->st_stid.sc_stateid, nfsd4_has_session(cstate));
> if (status == nfs_ok)
> status = nfs4_check_fh(current_fh, &stp->st_stid);
> if (status != nfs_ok)
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> return status;
> }
>
> @@ -5030,7 +5037,7 @@ static __be32 nfs4_preprocess_confirmed_seqid_op(struct nfsd4_compound_state *cs
> return status;
> oo = openowner(stp->st_stateowner);
> if (!(oo->oo_flags & NFS4_OO_CONFIRMED)) {
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> nfs4_put_stid(&stp->st_stid);
> return nfserr_bad_stateid;
> }
> @@ -5062,12 +5069,12 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> oo = openowner(stp->st_stateowner);
> status = nfserr_bad_stateid;
> if (oo->oo_flags & NFS4_OO_CONFIRMED) {
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> goto put_stateid;
> }
> oo->oo_flags |= NFS4_OO_CONFIRMED;
> nfs4_inc_and_copy_stateid(&oc->oc_resp_stateid, &stp->st_stid);
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> dprintk("NFSD: %s: success, seqid=%d stateid=" STATEID_FMT "\n",
> __func__, oc->oc_seqid, STATEID_VAL(&stp->st_stid.sc_stateid));
>
> @@ -5143,7 +5150,7 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
> nfs4_inc_and_copy_stateid(&od->od_stateid, &stp->st_stid);
> status = nfs_ok;
> put_stateid:
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> nfs4_put_stid(&stp->st_stid);
> out:
> nfsd4_bump_seqid(cstate, status);
> @@ -5196,7 +5203,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> if (status)
> goto out;
> nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
>
> nfsd4_close_open_stateid(stp);
>
> @@ -5422,7 +5429,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
> stp->st_access_bmap = 0;
> stp->st_deny_bmap = open_stp->st_deny_bmap;
> stp->st_openstp = open_stp;
> - init_rwsem(&stp->st_rwsem);
> + mutex_init(&stp->st_mutex);
> list_add(&stp->st_locks, &open_stp->st_locks);
> list_add(&stp->st_perstateowner, &lo->lo_owner.so_stateids);
> spin_lock(&fp->fi_lock);
> @@ -5591,7 +5598,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> &open_stp, nn);
> if (status)
> goto out;
> - up_write(&open_stp->st_rwsem);
> + mutex_unlock(&open_stp->st_mutex);
> open_sop = openowner(open_stp->st_stateowner);
> status = nfserr_bad_stateid;
> if (!same_clid(&open_sop->oo_owner.so_client->cl_clientid,
> @@ -5600,7 +5607,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> status = lookup_or_create_lock_state(cstate, open_stp, lock,
> &lock_stp, &new);
> if (status == nfs_ok)
> - down_write(&lock_stp->st_rwsem);
> + mutex_lock(&lock_stp->st_mutex);
> } else {
> status = nfs4_preprocess_seqid_op(cstate,
> lock->lk_old_lock_seqid,
> @@ -5704,7 +5711,7 @@ out:
> seqid_mutating_err(ntohl(status)))
> lock_sop->lo_owner.so_seqid++;
>
> - up_write(&lock_stp->st_rwsem);
> + mutex_unlock(&lock_stp->st_mutex);
>
> /*
> * If this is a new, never-before-used stateid, and we are
> @@ -5874,7 +5881,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> fput:
> fput(filp);
> put_stateid:
> - up_write(&stp->st_rwsem);
> + mutex_unlock(&stp->st_mutex);
> nfs4_put_stid(&stp->st_stid);
> out:
> nfsd4_bump_seqid(cstate, status);
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 986e51e..64053ea 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -535,7 +535,7 @@ struct nfs4_ol_stateid {
> unsigned char st_access_bmap;
> unsigned char st_deny_bmap;
> struct nfs4_ol_stateid *st_openstp;
> - struct rw_semaphore st_rwsem;
> + struct mutex st_mutex;
> };
>
> static inline struct nfs4_ol_stateid *openlockstateid(struct nfs4_stid *s)
> --
> 2.7.4
[toc] | [prev] | [next] | [standalone]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web