[nfs-discuss] Code reviewers wanted for 6751438 mirror mounted mountpoints panic when umounted

[Frank Batschulat (Home)]

On Thu, 02 Oct 2008 16:21:04 +0200, Frank Batschulat (Home)  wrote:

> Outch ! really ? man, we got a problem already ;-)
>
for the outside observer this should have been of course:

http://src.opensolaris.org/source/search?q=&defs=&refs=VERIFY&path=&hist=&project=%2Fonnv

---
frankB

[nfs-discuss] Code reviewers wanted for 6751438 mirror mounted mountpoints panic when umounted

[Frank Batschulat (Home)]

On Thu, 02 Oct 2008 16:11:01 +0200, Tom Haynes  wrote:

>>   292 nfs4_ephemeral_tree_incr(nfs4_ephemeral_tree_t *net)
>>   293 {
>>   294 ASSERT(mutex_owned(&net->net_cnt_lock));
>>   295 net->net_refcnt++;
>>   296 ASSERT(net->net_refcnt != 0);
>>
>>   312 nfs4_ephemeral_tree_decr(nfs4_ephemeral_tree_t *net)
>>   313 {
>>   314 ASSERT(mutex_owned(&net->net_cnt_lock));
>>   315 ASSERT(net->net_refcnt != 0);
>>   316 net->net_refcnt--;
>>
>> to me, it looks lile we never ever just want to continue if
>> we encounter the "net_refcnt != 0" case, debug or not, thus
>> a VERIFY() seems to be more appropriate here.
>
> I was taught not to use VERIFYs as it  caused customers problems.

Outch ! really ? man, we got a problem already ;-)

http://grok.czech.sun.com:8080/source/search?q=&defs=&refs=VERIFY&path=&hist=&project=%2Fonnv-clone

nah, honestly I'd be interested to know about that, really...

>> PS: fwiw, I usually consider code that drops locks that have
>> been acquired somewhere else 'net_tree_lock' bad practice and
>> not "safely" ;-) even more if the "go/nogo" decision is being
>> parsed thru multiple layers here with 'pmust_unlock'
>
> The alternative is to pull code from the v4 unmounting down into this code.
>
> As that code is dependent on whether it is a forced unmount or not, it makes
> it harder to abstract. But that could be done.
>
> But I didn't want to pull non-mirror mount code down into the mirror mount
> code. I.e., I didn't want to inadvertently hide it because it was put where
> not expected.

quite, wasn't a request for this fix as this was not new code that had been
added but already existing one...

cheers
frankB

[nfs-discuss] Code reviewers wanted for 6751438 mirror mounted mountpoints panic when umounted

[Frank Batschulat (Home)]

On Thu, 02 Oct 2008 02:56:46 +0200, Tom Haynes  wrote:

> The webrev can be found here:
> http://cr.opensolaris.org/~tdh/6751438/

looks sufficiently sane to me.
comments though...

  292 nfs4_ephemeral_tree_incr(nfs4_ephemeral_tree_t *net)
  293 {
  294 ASSERT(mutex_owned(&net->net_cnt_lock));
  295 net->net_refcnt++;
  296 ASSERT(net->net_refcnt != 0);

  312 nfs4_ephemeral_tree_decr(nfs4_ephemeral_tree_t *net)
  313 {
  314 ASSERT(mutex_owned(&net->net_cnt_lock));
  315 ASSERT(net->net_refcnt != 0);
  316 net->net_refcnt--;

to me, it looks lile we never ever just want to continue if
we encounter the "net_refcnt != 0" case, debug or not, thus
a VERIFY() seems to be more appropriate here.

PS: fwiw, I usually consider code that drops locks that have
been acquired somewhere else 'net_tree_lock' bad practice and
not "safely" ;-) even more if the "go/nogo" decision is being
parsed thru multiple layers here with 'pmust_unlock'

1706  * Common code to safely release net_cnt_lock and net_tree_lock
1707  */
1708 void
1709 nfs4_ephemeral_umount_unlock(bool_t *pmust_unlock,
1710 bool_t *pmust_rele, nfs4_ephemeral_tree_t **pnet)
1711 {
1712 nfs4_ephemeral_tree_t   *net = *pnet;
1713
1714 if (*pmust_unlock) {
1715 mutex_enter(&net->net_cnt_lock);
1716 net->net_status &= ~NFS4_EPHEMERAL_TREE_UMOUNTING;
1717 if (*pmust_rele)
1718 nfs4_ephemeral_tree_decr(net);
1719 mutex_exit(&net->net_cnt_lock);
1720
1721 mutex_exit(&net->net_tree_lock);
1722
1723 *pmust_unlock = FALSE;
1724 }
1725 }

---
frankB

[nfs-discuss] Code reviewers wanted for 6751438 mirror mounted mountpoints panic when umounted

[Tom Haynes]

Frank Batschulat (Home) wrote:
> On Thu, 02 Oct 2008 02:56:46 +0200, Tom Haynes  
> wrote:
>
>   
>> The webrev can be found here:
>> http://cr.opensolaris.org/~tdh/6751438/
>> 
>
> looks sufficiently sane to me.
> comments though...
>
>   292 nfs4_ephemeral_tree_incr(nfs4_ephemeral_tree_t *net)
>   293 {
>   294 ASSERT(mutex_owned(&net->net_cnt_lock));
>   295 net->net_refcnt++;
>   296 ASSERT(net->net_refcnt != 0);
>
>   312 nfs4_ephemeral_tree_decr(nfs4_ephemeral_tree_t *net)
>   313 {
>   314 ASSERT(mutex_owned(&net->net_cnt_lock));
>   315 ASSERT(net->net_refcnt != 0);
>   316 net->net_refcnt--;
>
> to me, it looks lile we never ever just want to continue if
> we encounter the "net_refcnt != 0" case, debug or not, thus
> a VERIFY() seems to be more appropriate here.
>
>   


I was taught not to use VERIFYs as it  caused customers problems.
Yes, I know they still have problems and we are maiking it harder
to observe.

And while this follows common practice in using refcnts in the
NFS code, perhaps to aid in observability we could add some
dtrace probes?

Or is this something we could easily do in a dtrace script?
I could envision a script that would capture it.




> PS: fwiw, I usually consider code that drops locks that have
> been acquired somewhere else 'net_tree_lock' bad practice and
> not "safely" ;-) even more if the "go/nogo" decision is being
> parsed thru multiple layers here with 'pmust_unlock'
>
>   

The alternative is to pull code from the v4 unmounting down into this code.

As that code is dependent on whether it is a forced unmount or not, it makes
it harder to abstract. But that could be done.

But I didn't want to pull non-mirror mount code down into the mirror mount
code. I.e., I didn't want to inadvertently hide it because it was put where
not expected.



> 1706  * Common code to safely release net_cnt_lock and net_tree_lock
> 1707  */
> 1708 void
> 1709 nfs4_ephemeral_umount_unlock(bool_t *pmust_unlock,
> 1710 bool_t *pmust_rele, nfs4_ephemeral_tree_t **pnet)
> 1711 {
> 1712 nfs4_ephemeral_tree_t   *net = *pnet;
> 1713
> 1714 if (*pmust_unlock) {
> 1715 mutex_enter(&net->net_cnt_lock);
> 1716 net->net_status &= ~NFS4_EPHEMERAL_TREE_UMOUNTING;
> 1717 if (*pmust_rele)
> 1718 nfs4_ephemeral_tree_decr(net);
> 1719 mutex_exit(&net->net_cnt_lock);
> 1720
> 1721 mutex_exit(&net->net_tree_lock);
> 1722
> 1723 *pmust_unlock = FALSE;
> 1724 }
> 1725 }
>
> ---
> frankB
>   

[nfs-discuss] Code reviewers wanted for 6751438 mirror mounted mountpoints panic when umounted

[Tom Haynes]

See http://blogs.sun.com/tdh/entry/debugging_a_refcnt and 
http://blogs.sun.com/tdh/entry/more_on_that_refcnt_debugging
for more than you want to know about this bug.

Basically, the last couple of paragraphs of the second article explain 
it all:

> Okay, I took my first advice to add more comments and I have a new 
> piece of code to question in nfs4_ephemeral_umount 
> :
>
>1763   int is_recursed = FALSE;
>1764   int was_locked = FALSE;
> ...
>1770   *pmust_unlock = FALSE;
> ...
>1817   if (!is_recursed) {
> ...
>1845   was_locked = TRUE;
>1846   } else {
>1847   net->net_refcnt++;
>1848   ASSERT(net->net_refcnt != 0);
> ...
>1859   if (was_locked == FALSE) {
> ...
>1866   if (mutex_tryenter(&net->net_tree_lock)) {
>1867   *pmust_unlock = TRUE;
>1868   } else {
>   
>
> So, if *!is_recursed*, then we can end up on line 1847 to bump the 
> refcnt. At this point, **pmust_unlock* is FALSE and *was_locked* is 
> also FALSE. If we grab the *net->net_tree_lock* right away on 1866, 
> then crud, it is okay.
>
> But the other case is not. Assume that *is_recursed* is TRUE. That 
> means we do not bump the refcnt and at 1859, the following values 
> hold: *was_locked* is FALSE and **pmust_unlock* is FALSE. Now if we 
> grab the mutex right away, we end up with **pmust_unlock* as TRUE. In 
> my examination of the code, that then implies we *must* drop the 
> refcnt at some exit point. But we just saw that we never grabbed it in 
> this scenario.
>
> This would account for us trying to rele a refcnt we never held. We 
> return TRUE to nfs4_free_mount 
>  and it 
> sends this into nfs4_ephemeral_umount_activate 
>  
> and thus into nfs4_ephemeral_umount_unlock 
> .
>
> I'm going to introduce more state in a *pmust_rele to keep track of 
> whether I need to rele or not.
>
> 

Testing has shown that we were dropping the refcnt when we had never 
held it in the first place.

I solve the problem by passing in a boolean to keep track of whether or 
not we held the
lock and thus we need to rele it.

As shown in the blog entries, I did some other changes as well:

>1. Introduced a incr function to make sure that all bumps are correct:
>  291  static void
>  292  nfs4_ephemeral_tree_incr(nfs4_ephemeral_tree_t *net)
>  293  {
>  294  ASSERT(mutex_owned(&net->net_cnt_lock));
>  295  net->net_refcnt++;
>  296  ASSERT(net->net_refcnt != 0);
>  297  }
>  298
>  299  static void
>  300  nfs4_ephemeral_tree_hold(nfs4_ephemeral_tree_t *net)
>  301  {
>  302  mutex_enter(&net->net_cnt_lock);
>  303  nfs4_ephemeral_tree_incr(net);
>  304  mutex_exit(&net->net_cnt_lock);
>  305  }
>   ...
> 1859  } else {
> 1860  nfs4_ephemeral_tree_incr(net);
> 1861  }
> 
>2. Fixed up the one locking issue:
>  734  } else {
>  735  net = mi->mi_ephemeral_tree;
>  736  nfs4_ephemeral_tree_hold(net);
>  737
>  738  mutex_exit(&mi->mi_lock);
> 
>3. And I still have the change which I thought would fix the bug:
> 2006  if (was_locked == FALSE)
> 2007  mutex_exit(&net->net_tree_lock);
> 
>

The webrev can be found here:

http://cr.opensolaris.org/~tdh/6751438/