[nfs-discuss] webrev for CR 6768607

[Jeff A. Smith]

DOH!  What the heck was I thinking?  I
initially reviewed this really late and
completely missed the point.  No more
late night code reviews for me.  Sorry
about that!

Jeff

On Mar 13, 2009, at 3:20 PM, Dai Ngo wrote:
> The intention of the change is to handle condition when the  
> set_clientid_confirm
> somehow never comes. In this case the server (rfs4_do_cb_null())  
> never sends out
> any cb_null call and returns immediately causing rfs4_cbinfo_hold()  
> to loop forever.

[nfs-discuss] webrev for CR 6768607

[Jeff A. Smith]

On Mar 12, 2009, at 12:28 PM, Dai Ngo wrote:
> Hi Jim,
>
> james wahlig wrote:
>>
>> I suppose I could run through the server code and see if there is a  
>> precedent already set, but thought maybe someone on the alias might  
>> know.  This is something that Spencer probably would have known off  
>> the top of his head, but I don't.
>>
>> Maybe there is a retry variable defined somewhere that we could  
>> reuse instead of creating a new one.
> I consulted with Jeff, and we did not find any existing configurable  
> variable to use so
> I created a new one.

Hi Jim,

I also thought that we'd have a "num retries" variable
defined somewhere.  I found nfs4_max_recov_error_retry which
is used by client recovery and set to 3.  The client has another
retry global for retrying a mount (set to 2).  My personal
favorite is recov_state.rs_num_retry_despite_err (client).  :-)

I didn't think it made sense to limit max retries for this bug
using one of the other retry-related vars I found.  So I thought
it would be okay for Dai to define a new one for this case.
I did mention that 10 felt "too big" to me (probably because I'm
a little polluted with client defaults of 2-3), but I didn't
insist on making it smaller because the fix is about not
retrying forever, and for that, 10 is as good as 3.

The more I think about it, the more I like no retries.  Maybe
I'm missing something, but why would the client drop/ignore
the first cbnull but process a subsequent cbnull?  I'm thinking
that if it doesn't reply to the first, then it will probably not
reply to subsequent cb_null calls.  It would be interesting to
know how many times our client fails to reply to cb_null.  I
suspect that we'd see server either issue no retries or the
max number of retries and nothing in between.

Jeff

[nfs-discuss] webrev for CR 6768607

[Dai Ngo]

>
> The more I think about it, the more I like no retries.  Maybe
> I'm missing something, but why would the client drop/ignore
> the first cbnull but process a subsequent cbnull?  I'm thinking
> that if it doesn't reply to the first, then it will probably not
> reply to subsequent cb_null calls.  It would be interesting to
> know how many times our client fails to reply to cb_null.  I
> suspect that we'd see server either issue no retries or the
> max number of retries and nothing in between.
The intention of the change is to handle condition when the 
set_clientid_confirm
somehow never comes. In this case the server (rfs4_do_cb_null()) never 
sends out
any cb_null call and returns immediately causing rfs4_cbinfo_hold() to 
loop forever.
If the set_clientid_confirm does arrive, rfs4_do_cb_null() will send the 
cb_null call
and rfs4_cbinfo_hold()  will break out of the loop immediately, 
regardless of the
result of the call, since cb_new is no longer TRUE.

So the retries is to allow some time (5 secs) for the 
set_clientid_confirm to arrive,
and not for retrying the cb_null call.

-Dai

[nfs-discuss] webrev for CR 6768607

[Frank Batschulat (Home)]

On Thu, 12 Mar 2009 22:38:28 +0100, Dai Ngo  wrote:

>
> Frank, I added comments as suggested.
> Webrev is updated here : http://cr.opensolaris.org/~dain/6768607/

thanks! that's what I had in mind.

cheers
frankB

>
> Thanks,
> -Dai
>>
>>> what I'd definitively request to see is extending the comments
>>> preceeding rfs4_cbinfo_hold() to give hints about why our while loop
>>> goes:
>>>
>>>  355 while (cbp->cb_newer.cb_new == TRUE &&
>>> cbp->cb_nullcaller == FALSE) {
>>>
>>> and the new bailout happens the other way around with:
>>>
>>>  365 if (cbp->cb_newer.cb_new == FALSE ||
>>> cbp->cb_nullcaller == TRUE)
>>>
>>> I'm pretty sure, after this has been putbacked, and 3 or 4 months
>>> went over,
>>> nobody can explain that code immediately anymore... ;-)
>>>
>> I will add some comment here, and will update the webrev.

[nfs-discuss] webrev for CR 6768607

[Frank Batschulat (Home)]

On Thu, 12 Mar 2009 15:29:02 +0100, Calum Mackay  
wrote:

> Dai Ngo wrote:
>> The problem was caused by an infinite loop in rfs4_cbinfo_hold().
>> This thread put a hold on the DBE of a rfs4_deleg_state causing the reaper
>> thread to be delayed (forever). Since the deleg_state table was not
>> cleaned,
>> this caused the reaper threads of the file and client table to also be
>> delayed, due to the hold on their DBEs from the deleg_state entries.
>
> Interesting; I wonder if this, or something similar, might be related to
> the issue being seen in SC HA-NFSv4 where the distributed stable storage
> files are occasionally not being removed from the SC RG state paths?
> That mechanism is also dependent on the client DBE reap.
>
> However, if it's the case that the /var/nfs state file does get removed
> when this occurs, that would be something else...

fwiw, Calum is talking about:

http://bugs.opensolaris.org/view_bug.do?bug_id=6802893


> cheers,
> calum.
>
>> I added
>> detailed
>> analysis in the evaluation section of the CR.
>>
>> The fix is to limit the number of retries to 5 (5 secs).
>>
>> webrev: http://cr.opensolaris.org/~dain/6768607/
>> 
>> CR: http://monaco.sfbay/detail.jsf?cr=6768607
>>
>> Thanks,
>> -Dai
>> ___
>> nfs-discuss mailing list
>> nfs-discuss at opensolaris.org
>



-- 
frankB

It is always possible to agglutinate multiple separate problems
into a single complex interdependent solution.
In most cases this is a bad idea.

[nfs-discuss] webrev for CR 6768607

[Frank Batschulat (Home)]

On Thu, 12 Mar 2009 07:50:14 +0100, Dai Ngo  wrote:

> I'd like to have a code review for the change to fix CR 6768607.
>
> The problem was caused by an infinite loop in rfs4_cbinfo_hold().
> This thread put a hold on the DBE of a rfs4_deleg_state causing the reaper
> thread to be delayed (forever). Since the deleg_state table was not cleaned,
> this caused the reaper threads of the file and client table to also be
> delayed,
> due to the hold on their DBEs from the deleg_state entries. I added detailed
> analysis in the evaluation section of the CR.
>
> The fix is to limit the number of retries to 5 (5 secs).
>
> webrev:  http://cr.opensolaris.org/~dain/6768607/
> CR: http://monaco.sfbay/detail.jsf?cr=6768607

Dai, currently rfs4_cbinfo_hold() only returns NULL to its
caller in the case of the callback state != CB_OK, the changes
will introduce a new 2nd error return path for returning NULL without
knowing about the callback state itself from the first blush,
are we sure that it is impossible to return here with a cb_state of CB_OK ?

ie. somehow leave a dangling rfs4_cbinfo_t with CB_OK around although we
failed here for the caller. from the first blush it appears that there shouldn't
be any harm as we'll immediately fail with RPC_FAILED in rfs4_do_callback().

fix looks technically correct to me, although the dance around with the cb_lock
looks somewhat ... hmm...blinding.

what I'd definitively request to see is extending the comments 
preceeding rfs4_cbinfo_hold() to give hints about why our while loop
goes:

 355 while (cbp->cb_newer.cb_new == TRUE && cbp->cb_nullcaller == 
FALSE) {

and the new bailout happens the other way around with:

 365 if (cbp->cb_newer.cb_new == FALSE || cbp->cb_nullcaller == 
TRUE)

I'm pretty sure, after this has been putbacked, and 3 or 4 months went over,
nobody can explain that code immediately anymore... ;-)

thanks
frankB

[nfs-discuss] webrev for CR 6768607

[Dai Ngo]

Frank, I added comments as suggested.
Webrev is updated here : http://cr.opensolaris.org/~dain/6768607/ 


Thanks,
-Dai
>
>> what I'd definitively request to see is extending the comments 
>> preceeding rfs4_cbinfo_hold() to give hints about why our while loop
>> goes:
>>
>>  355 while (cbp->cb_newer.cb_new == TRUE && 
>> cbp->cb_nullcaller == FALSE) {
>>
>> and the new bailout happens the other way around with:
>>
>>  365 if (cbp->cb_newer.cb_new == FALSE || 
>> cbp->cb_nullcaller == TRUE)
>>
>> I'm pretty sure, after this has been putbacked, and 3 or 4 months 
>> went over,
>> nobody can explain that code immediately anymore... ;-)
>>   
> I will add some comment here, and will update the webrev.

[nfs-discuss] webrev for CR 6768607

[Calum Mackay]

Dai Ngo wrote:
> The problem was caused by an infinite loop in rfs4_cbinfo_hold().
> This thread put a hold on the DBE of a rfs4_deleg_state causing the reaper
> thread to be delayed (forever). Since the deleg_state table was not
> cleaned,
> this caused the reaper threads of the file and client table to also be
> delayed,
> due to the hold on their DBEs from the deleg_state entries.

Interesting; I wonder if this, or something similar, might be related to 
the issue being seen in SC HA-NFSv4 where the distributed stable storage 
files are occasionally not being removed from the SC RG state paths? 
That mechanism is also dependent on the client DBE reap.

However, if it's the case that the /var/nfs state file does get removed 
when this occurs, that would be something else...

cheers,
calum.

> I added
> detailed
> analysis in the evaluation section of the CR.
>
> The fix is to limit the number of retries to 5 (5 secs).
>
> webrev: http://cr.opensolaris.org/~dain/6768607/
> 
> CR: http://monaco.sfbay/detail.jsf?cr=6768607
>
> Thanks,
> -Dai
> ___
> nfs-discuss mailing list
> nfs-discuss at opensolaris.org

-- 
Calum Mackay
Senior Staff Engineer
Systems Group, Quality Office

[nfs-discuss] webrev for CR 6768607

[Dai Ngo]

Hi Frank,

Please see reply below:
Frank Batschulat (Home) wrote:
> On Thu, 12 Mar 2009 07:50:14 +0100, Dai Ngo  wrote:
>
>   
>> I'd like to have a code review for the change to fix CR 6768607.
>>
>> The problem was caused by an infinite loop in rfs4_cbinfo_hold().
>> This thread put a hold on the DBE of a rfs4_deleg_state causing the reaper
>> thread to be delayed (forever). Since the deleg_state table was not cleaned,
>> this caused the reaper threads of the file and client table to also be
>> delayed,
>> due to the hold on their DBEs from the deleg_state entries. I added detailed
>> analysis in the evaluation section of the CR.
>>
>> The fix is to limit the number of retries to 5 (5 secs).
>>
>> webrev:  http://cr.opensolaris.org/~dain/6768607/
>> CR: http://monaco.sfbay/detail.jsf?cr=6768607
>> 
>
> Dai, currently rfs4_cbinfo_hold() only returns NULL to its
> caller in the case of the callback state != CB_OK, the changes
> will introduce a new 2nd error return path for returning NULL without
> knowing about the callback state itself from the first blush,
> are we sure that it is impossible to return here with a cb_state of CB_OK ?
>   
This change takes a conservative approach, assuming that if the 
set_clientid_confirm does
not arrive within 5 secs  (configurable), after the set_clientid, then 
just actively revokes the
delegation and leaves the cbinfo_t as is. The change does not attempt to 
fix the problem with
the callback path that depends on set_clientid and set_clientid_confirm 
requests, but rather
the fix is not to get stuck in this loop causing the problem reported in 
the CR.
> ie. somehow leave a dangling rfs4_cbinfo_t with CB_OK around although we
> failed here for the caller. from the first blush it appears that there 
> shouldn't
> be any harm as we'll immediately fail with RPC_FAILED
>  in rfs4_do_callback().
>   
Yes, the call was returned with RPC_FAILED by rfs4_do_callback() and the 
delegation
is revoked by rfs4_do_cb_recall(). So I think we should be OK.

> fix looks technically correct to me, although the dance around with the 
> cb_lock
> looks somewhat ... hmm...blinding.
>   
Agree, but it makes the code looks consistent; protecting access to 
rfs4_cbinfo_t with cb_lock.
Since this is a fail path, performance should not be effected under 
normal condition.
> what I'd definitively request to see is extending the comments 
> preceeding rfs4_cbinfo_hold() to give hints about why our while loop
> goes:
>
>  355 while (cbp->cb_newer.cb_new == TRUE && cbp->cb_nullcaller == 
> FALSE) {
>
> and the new bailout happens the other way around with:
>
>  365 if (cbp->cb_newer.cb_new == FALSE || cbp->cb_nullcaller 
> == TRUE)
>
> I'm pretty sure, after this has been putbacked, and 3 or 4 months went over,
> nobody can explain that code immediately anymore... ;-)
>   
I will add some comment here, and will update the webrev.

Thank you for reviewing the change,

-Dai
> thanks
> frankB
>
>
>
>
>
>   

[nfs-discuss] webrev for CR 6768607

[Dai Ngo]

Hi Jim,

james wahlig wrote:
> The code looks functionally correct, however, I was wondering what 
> criteria you used for the number and duration of retries?
This is based on the assumption that the set_clientid_confirm should be 
sent almost immediately
after the set_clientid. Under normal circumstances this piece of code 
should be rarely executed.
If a race condition occurs, a delegation recall happens before the 
set_clientid_confirm arrives,
the change allows about 5 secs (retries) for the set_clientid_confirm to 
arrive.
>
> I suppose I could run through the server code and see if there is a 
> precedent already set, but thought maybe someone on the alias might 
> know.  This is something that Spencer probably would have known off 
> the top of his head, but I don't.
>
> Maybe there is a retry variable defined somewhere that we could reuse 
> instead of creating a new one.
I consulted with Jeff, and we did not find any existing configurable 
variable to use so
I created a new one.

Thanks for reviewing the change.

-Dai
>
> Dai, the code looks good, and I'm not requiring a change, just looking 
> to see if we are being consistent with retries in the server.
>
> Jim
>
> Dai Ngo wrote:
>
>> Hi,
>>
>> I'd like to have a code review for the change to fix CR 6768607.
>>
>> The problem was caused by an infinite loop in rfs4_cbinfo_hold().
>> This thread put a hold on the DBE of a rfs4_deleg_state causing the 
>> reaper
>> thread to be delayed (forever). Since the deleg_state table was not 
>> cleaned,
>> this caused the reaper threads of the file and client table to also 
>> be delayed,
>> due to the hold on their DBEs from the deleg_state entries. I added 
>> detailed
>> analysis in the evaluation section of the CR.
>>
>> The fix is to limit the number of retries to 5 (5 secs).
>>
>> webrev:  http://cr.opensolaris.org/~dain/6768607/ 
>> 
>> CR: http://monaco.sfbay/detail.jsf?cr=6768607
>>
>> Thanks,
>> -Dai
>> ___
>> nfs-discuss mailing list
>> nfs-discuss at opensolaris.org
>
>

[nfs-discuss] webrev for CR 6768607

[james wahlig]

The code looks functionally correct, however, I was wondering what 
criteria you used for the number and duration of retries?

I suppose I could run through the server code and see if there is a 
precedent already set, but thought maybe someone on the alias might 
know.  This is something that Spencer probably would have known off the 
top of his head, but I don't.

Maybe there is a retry variable defined somewhere that we could reuse 
instead of creating a new one.

Dai, the code looks good, and I'm not requiring a change, just looking 
to see if we are being consistent with retries in the server.

Jim

Dai Ngo wrote:

> Hi,
>
> I'd like to have a code review for the change to fix CR 6768607.
>
> The problem was caused by an infinite loop in rfs4_cbinfo_hold().
> This thread put a hold on the DBE of a rfs4_deleg_state causing the 
> reaper
> thread to be delayed (forever). Since the deleg_state table was not 
> cleaned,
> this caused the reaper threads of the file and client table to also be 
> delayed,
> due to the hold on their DBEs from the deleg_state entries. I added 
> detailed
> analysis in the evaluation section of the CR.
>
> The fix is to limit the number of retries to 5 (5 secs).
>
> webrev:  http://cr.opensolaris.org/~dain/6768607/ 
> 
> CR: http://monaco.sfbay/detail.jsf?cr=6768607
>
> Thanks,
> -Dai
> ___
> nfs-discuss mailing list
> nfs-discuss at opensolaris.org

[nfs-discuss] webrev for CR 6768607

[Dai Ngo]

Hi,

I'd like to have a code review for the change to fix CR 6768607.

The problem was caused by an infinite loop in rfs4_cbinfo_hold().
This thread put a hold on the DBE of a rfs4_deleg_state causing the reaper
thread to be delayed (forever). Since the deleg_state table was not cleaned,
this caused the reaper threads of the file and client table to also be 
delayed,
due to the hold on their DBEs from the deleg_state entries. I added detailed
analysis in the evaluation section of the CR.

The fix is to limit the number of retries to 5 (5 secs).

webrev:  http://cr.opensolaris.org/~dain/6768607/ 

CR: http://monaco.sfbay/detail.jsf?cr=6768607

Thanks,
-Dai