2009-06-18 01:55:29

by Labiaga, Ricardo

Subject: [PATCH 1/1] nfsd41: renew_client() needs to be called with the client_mutex held

renew_client() manipulates the client queue for lease renewal. Need to
obtain the client_mutex before manipulating it.

Signed-off-by: Ricardo Labiaga <[email protected]>
---
fs/nfsd/nfs4state.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4cb5d1d..18258d7 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1537,7 +1537,9 @@ replay_cache:
* Hold a session reference until done processing the compound:
* nfsd4_put_session called only if the cstate slot is set.
*/
+ nfs4_lock_state();
renew_client(session->se_client);
+ nfs4_unlock_state();
nfsd4_get_session(session);
out:
spin_unlock(&sessionid_lock);
--
1.5.4.3

2009-06-18 17:31:17

by J. Bruce Fields

Subject: Re: [PATCH 1/1] nfsd41: renew_client() needs to be called with the client_mutex held

On Wed, Jun 17, 2009 at 06:50:45PM -0700, Ricardo Labiaga wrote:
> renew_client() manipulates the client queue for lease renewal. Need to
> obtain the client_mutex before manipulating it.
>
> Signed-off-by: Ricardo Labiaga <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 4cb5d1d..18258d7 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1537,7 +1537,9 @@ replay_cache:
> * Hold a session reference until done processing the compound:
> * nfsd4_put_session called only if the cstate slot is set.
> */
> + nfs4_lock_state();
> renew_client(session->se_client);
> + nfs4_unlock_state();
> nfsd4_get_session(session);
> out:
> spin_unlock(&sessionid_lock);

We can't take a mutex while handling a spinlock. (And you should have
gotten warnings about this--not sure what kernel hacking config options
you may need turned on for that.)

Possible hack: add a spinlock that for now will just cover the client
lru?

--b.

2009-06-18 17:39:10

by Labiaga, Ricardo

Subject: Re: [PATCH 1/1] nfsd41: renew_client() needs to be called with the client_mutex held

On 6/18/09 10:31 AM, "J. Bruce Fields" <[email protected]> wrote:

> On Wed, Jun 17, 2009 at 06:50:45PM -0700, Ricardo Labiaga wrote:
>> renew_client() manipulates the client queue for lease renewal. Need to
>> obtain the client_mutex before manipulating it.
>>
>> Signed-off-by: Ricardo Labiaga <[email protected]>
>> ---
>> fs/nfsd/nfs4state.c | 2 ++
>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 4cb5d1d..18258d7 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1537,7 +1537,9 @@ replay_cache:
>> * Hold a session reference until done processing the compound:
>> * nfsd4_put_session called only if the cstate slot is set.
>> */
>> + nfs4_lock_state();
>> renew_client(session->se_client);
>> + nfs4_unlock_state();
>> nfsd4_get_session(session);
>> out:
>> spin_unlock(&sessionid_lock);
>
> We can't take a mutex while handling a spinlock. (And you should have
> gotten warnings about this--not sure what kernel hacking config options
> you may need turned on for that.)
>

Ah, OK. I didn't know that.

> Possible hack: add a spinlock that for now will just cover the client
> lru?

Let me study this some more and see if I can simply drop the sessionid
spinlock before renewing the state.

- ricardo

>
> --b.

2009-06-18 17:48:46

by J. Bruce Fields

Subject: Re: [PATCH 1/1] nfsd41: renew_client() needs to be called with the client_mutex held

On Thu, Jun 18, 2009 at 10:39:10AM -0700, Labiaga, Ricardo wrote:
> On 6/18/09 10:31 AM, "J. Bruce Fields" <[email protected]> wrote:
>
> > On Wed, Jun 17, 2009 at 06:50:45PM -0700, Ricardo Labiaga wrote:
> >> renew_client() manipulates the client queue for lease renewal. Need to
> >> obtain the client_mutex before manipulating it.
> >>
> >> Signed-off-by: Ricardo Labiaga <[email protected]>
> >> ---
> >> fs/nfsd/nfs4state.c | 2 ++
> >> 1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index 4cb5d1d..18258d7 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -1537,7 +1537,9 @@ replay_cache:
> >> * Hold a session reference until done processing the compound:
> >> * nfsd4_put_session called only if the cstate slot is set.
> >> */
> >> + nfs4_lock_state();
> >> renew_client(session->se_client);
> >> + nfs4_unlock_state();
> >> nfsd4_get_session(session);
> >> out:
> >> spin_unlock(&sessionid_lock);
> >
> > We can't take a mutex while handling a spinlock. (And you should have
> > gotten warnings about this--not sure what kernel hacking config options
> > you may need turned on for that.)
> >
>
> Ah, OK. I didn't know that.

The deadlock:

Task 1 takes a spinlock.
Task 1 sleeps on the mutex.
Task 2 is scheduled in.
Task 2 requests the same spinlock, spins.

Now Task 2 is spinning waiting for the spinlock, keeping task 1 from
getting the cpu back and releaseing the spinlock. So, no sleeping under
spinlocks.

> > Possible hack: add a spinlock that for now will just cover the client
> > lru?
>
> Let me study this some more and see if I can simply drop the sessionid
> spinlock before renewing the state.

OK.

--b.

2009-06-18 17:51:23

by Labiaga, Ricardo

Subject: Re: [PATCH 1/1] nfsd41: renew_client() needs to be called with the client_mutex held

On 6/18/09 10:48 AM, "J. Bruce Fields" <[email protected]> wrote:

> On Thu, Jun 18, 2009 at 10:39:10AM -0700, Labiaga, Ricardo wrote:
>> On 6/18/09 10:31 AM, "J. Bruce Fields" <[email protected]> wrote:
>>
>>> On Wed, Jun 17, 2009 at 06:50:45PM -0700, Ricardo Labiaga wrote:
>>>> renew_client() manipulates the client queue for lease renewal. Need to
>>>> obtain the client_mutex before manipulating it.
>>>>
>>>> Signed-off-by: Ricardo Labiaga <[email protected]>
>>>> ---
>>>> fs/nfsd/nfs4state.c | 2 ++
>>>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index 4cb5d1d..18258d7 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -1537,7 +1537,9 @@ replay_cache:
>>>> * Hold a session reference until done processing the compound:
>>>> * nfsd4_put_session called only if the cstate slot is set.
>>>> */
>>>> + nfs4_lock_state();
>>>> renew_client(session->se_client);
>>>> + nfs4_unlock_state();
>>>> nfsd4_get_session(session);
>>>> out:
>>>> spin_unlock(&sessionid_lock);
>>>
>>> We can't take a mutex while handling a spinlock. (And you should have
>>> gotten warnings about this--not sure what kernel hacking config options
>>> you may need turned on for that.)
>>>
>>
>> Ah, OK. I didn't know that.
>
> The deadlock:
>
> Task 1 takes a spinlock.
> Task 1 sleeps on the mutex.
> Task 2 is scheduled in.
> Task 2 requests the same spinlock, spins.
>
> Now Task 2 is spinning waiting for the spinlock, keeping task 1 from
> getting the cpu back and releaseing the spinlock. So, no sleeping under
> spinlocks.

Right, thanks.

- ricardo

>
>>> Possible hack: add a spinlock that for now will just cover the client
>>> lru?
>>
>> Let me study this some more and see if I can simply drop the sessionid
>> spinlock before renewing the state.
>
> OK.
>
> --b.