Frank Batschulat (Home) wrote:
> On Thu, 02 Oct 2008 02:56:46 +0200, Tom Haynes <Thomas.Haynes at Sun.COM>
> 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
>
