Frank's comments are incorporated: 1) Only changes in the comments, bo change in the C code:
http://cr.opensolaris.org/~pavelf/6500269 2) - mainly changes in the comments; only a small "int -> uint_t" change in the C code - 1 change in the comments for 6507213 http://cr.opensolaris.org/~pavelf/6707722 3) no changes http://cr.opensolaris.org/~pavelf/6721281 --Pavel On 06/09/09 16:40, Frank Batschulat (Home) wrote: > On Tue, 09 Jun 2009 11:12:22 +0200, pavel filipensky <Pavel.Filipensky at > sun.com> wrote: > > >> I have created separated webrevs for the bugs - to make the review more >> comfortable and allow the reviewers to review only a subset of the 3 CRs: >> > > Hi Pavel, first of all, if you expect outside reviewers you probably should > put the lot of analyse done in the bugs comments/evaluation section > to either the bugs description so it appears on bugs.opensolaris.org, or even > better, store a text file containing them at cr.opensolaris.org along > with the webrevs, for those 3 bugs below, it would have been really helpfull > for outside observers to better understand how you arrived here... > > 6500269 assertion failed: sp->mntinfo4_list == mi, file: > ../../common/fs/nfs/nfs4_vfsops.c, line: 3258 > >> http://cr.opensolaris.org/~pavelf/6500269 <<< I have added some >> > > overall looks good, comments: > > http://cr.opensolaris.org/~pavelf/6500269/usr/src/uts/common/nfs/nfs4_clnt.h.udiff.html > > 1) > * The mntinfo4_t::mi_recovlock protects the following fields: > * mi_srvsettime > * > + * The mntinfo4_t::mi_recovlock protects also mi_srv. > > I'd prefer this to be instead: > > * The mntinfo4_t::mi_recovlock protects the following fields: > ! * mi_srvsettime, mi_srv > > 2) > + * If changing mi_srv from one server to different server, mi_recovlock > + * as RW_WRITER is needed - happens in recov_newserver() and > recov_clientid(). > + * If mi_srv is set for the first time - happens in nfs4_mount() > + * and nfs4_mountroot(), mi_recovlock is held as RW_READER. > + * This means that holding mi_recovlock as RW_READER is enough to protect > + * mi_srv from being changed from one server to another. > > this is a bit confusing...I'd prefer this to be changes like below, the > functions may change > over time...and the latter part is redundant > > * Changing mi_srv from one nfs4_server_t to a different one requires > * holding the mi_recovlock as RW_WRITER. > * Exception: setting mi_srv the first time in mount/mounroot is done > * holding the mi_recovlock as RW_READER. > > 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. > > Btw, not your changes but still worth mentioning it: > > 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)); > > btw, this assert is not sufficient in the context of find_nfs4_server_all() > it only assert that _someone_ is holding the lock: > > 4883 nfs_rw_lock_held(nfs_rwlock_t *l, krw_t rw) > 4884 { > 4885 > 4886 if (rw == RW_READER) > 4887 return (l->count > 0); > 4888 ASSERT(rw == RW_WRITER); > 4889 return (l->count < 0); > 4890 } > > in the context of find_nfs4_server_all()/find_nfs4_server() we > really want to know that the _current caller_ is holding the lock as > called out in the comment: > > 3791 * The caller should be holding mi->mi_recovlock, and it should > continue to > 3792 * hold the lock until done with the returned nfs4_server_t. Once > 3793 * mi->mi_recovlock is released, there is no guarantee that the > returned > 3794 * mi->nfs4_server_t will continue to correspond to mi. > 3795 */ > 3796 nfs4_server_t * > 3797 find_nfs4_server(mntinfo4_t *mi) > > ================================= > > 6707722 nfs4_start_fop() uses gethrestime_sec() with 1 second granularity to > detect server change > >> http://cr.opensolaris.org/~pavelf/6707722 >> > > looks good, but comments: > > http://cr.opensolaris.org/~pavelf/6707722/usr/src/uts/common/fs/nfs/nfs4_recovery.c.udiff.html > > + int droplock_cnt; > > I'd prefer this to be a uint_t. > > http://cr.opensolaris.org/~pavelf/6707722/usr/src/uts/common/nfs/nfs4_clnt.h.udiff.html > + int mi_srvset_cnt; /* increment whenever changing the nfs4_server_t > */ > > I'd prefer mi_srvset_cnt to be a uint_t. > > ========================================= > > 6721281 panic: assertion failed: np != 0L, file: > ../../common/fs/nfs/nfs4_callback.c, line: 1309 > >> http://cr.opensolaris.org/~pavelf/6721281 >> > > this looks good to me. > > ========================================= > > Btw, since you touch nfs4_clnt.h anyways would ya mind fixing this along with > it ? > > http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=6507213 > > cheers > frankB >
