On Tue, 09 Jun 2009 17:13:36 +0200, pavel filipensky <Pavel.Filipensky at sun.com> wrote:
>> http://cr.opensolaris.org/~pavelf/6500269/usr/src/uts/common/fs/nfs/nfs4_vfsops.c.udiff.html >> >> 3) find_nfs4_server_all() >> >> 3809 find_nfs4_server_all(mntinfo4_t *mi, int all) >> 3810 { >> 3811 nfs4_server_t *np = mi->mi_srv; >> 3812 >> 3813 ASSERT(nfs_rw_lock_held(&mi->mi_recovlock, RW_READER) || >> 3814 nfs_rw_lock_held(&mi->mi_recovlock, RW_WRITER)); >> 3815 >> 3816 if (np && (np->s_thread_exit != NFS4_THREAD_EXIT || all)) { >> 3817 mutex_enter(&np->s_lock); >> 3818 np->s_refcnt++; >> 3819 return (np); >> 3820 } >> 3821 return (NULL); >> 3822 } >> >> now, that looks suspicious, you use the nfs4_server_t 'np' prior having a >> lock on it, that is >> different from the old find_nfs4_server_all() impl. as well. since we're also >> no longer holding the nfs4_server_lst_lock I have doubts we're properly >> protected here. > > Yes, this part is tricky and definitely should get enough attention. The > code related to find_nfs4_server() > code is fragile: > - the value based lookup gives uncertain results > - the results are used for actions with - no tolerance for uncertainty > (removing element from linked list) > > and unclear: > - the caller of find_nfs4_server_all() / find_nfs4_server() should be > aware if it the resulting nfs4_server_t > should be only the one to which is the mntinfo4_t currently linked > > I claim that all the callers of find_nfs4_server_all() / > find_nfs4_server() wants only the nfs4_server_t > to which is the mntinfo4_t currently linked. I was trying to describe > that here: > > + * find_nfs4_server() and find_nfs4_server_all() are used to find > + * a nfs4_server_t to which the mntinfo4_t was already linked. > + * This is in contrast to searching a new nfs4_server_t, by walking > + * the nfs4_server_lst list, needed e.g. when doing a failover. > > nfs4_server_lst_lock is not needed there; the changes of nfs4_server_lst > are irrelevant as: > 1) we are never interested in a newly added server > 2) we are protected from 'our' to be removed from nfs4_server_lst (by > holding mi_recovlock ) thanks, sounds reasonable to me. since we're holding the mi_recovlock which does protect mi_srv as well, we ought to be safe from someone de-linking the mi_srv from the mi we're currently looking at. fine. --- frankB
