5064742 better to have mi_srvsettime protected by mi_recovlock, not s_lock

3 files changed, 23 insertions, 12 deletions

diff --git a/usr/src/uts/common/fs/nfs/nfs4_recovery.c b/usr/src/uts/common/fs/nfs/nfs4_recovery.c
index db1abb9fba..9847c6c230 100644
--- a/usr/src/uts/common/fs/nfs/nfs4_recovery.c
+++ b/usr/src/uts/common/fs/nfs/nfs4_recovery.c
@@ -855,8 +855,8 @@ get_sp:
 	sp = find_nfs4_server(mi);
 	if (sp != NULL) {
 		sp->s_otw_call_count++;
-		droplock_time = gethrestime_sec();
 		mutex_exit(&sp->s_lock);
+		droplock_time = gethrestime_sec();
 	}
 	nfs_rw_exit(&mi->mi_recovlock);
 
@@ -878,15 +878,8 @@ get_sp:
 	 * If the mntinfo4_t hasn't changed nfs4_sever_ts then
 	 * there's no point in double checking to make sure it
 	 * has switched.
-	 * XXX is there a better lock to use for mi_srvsettime
-	 * instead of s_lock?  mi_recovlock as READER would
-	 * be perfect.
 	 */
-	if (sp != NULL)
-		mutex_enter(&sp->s_lock);
 	if (sp == NULL || droplock_time < mi->mi_srvsettime) {
-		if (sp != NULL)
-			mutex_exit(&sp->s_lock);
 		tsp = find_nfs4_server(mi);
 		if (tsp != sp) {
 			/* try again */
@@ -911,9 +904,6 @@ get_sp:
 				tsp = NULL;
 			}
 		}
-	} else {
-		if (sp != NULL)
-			mutex_exit(&sp->s_lock);
 	}
 
 	if (sp != NULL) {
diff --git a/usr/src/uts/common/fs/nfs/nfs4_vfsops.c b/usr/src/uts/common/fs/nfs/nfs4_vfsops.c
index 508da17358..8a73b69a18 100644
--- a/usr/src/uts/common/fs/nfs/nfs4_vfsops.c
+++ b/usr/src/uts/common/fs/nfs/nfs4_vfsops.c
@@ -2425,7 +2425,9 @@ int nfs4_num_sclid_retries = NFS4_NUM_SCLID_RETRIES;
  * already set.
  *
  * The recovery boolean should be set to TRUE if this function was called
- * by the recovery code, and FALSE otherwise.
+ * by the recovery code, and FALSE otherwise.  This is used to determine
+ * if we need to call nfs4_start/end_op as well as grab the mi_recovlock
+ * for adding a mntinfo4_t to a nfs4_server_t.
  *
  * Error is returned via 'n4ep'.  If there was a 'n4ep->stat' error, then
  * 'n4ep->error' is set to geterrno4(n4ep->stat).
@@ -2448,6 +2450,9 @@ nfs4setclientid(mntinfo4_t *mi, cred_t *cr, bool_t recovery, nfs4_error_t *n4ep)
 
 recov_retry:
 	nfs4_error_zinit(n4ep);
+	if (!recovery)
+		(void) nfs_rw_enter_sig(&mi->mi_recovlock, RW_READER, 0);
+
 	/* This locks np if it is found */
 	np = servinfo4_to_nfs4_server(svp);
 	ASSERT(np == NULL || MUTEX_HELD(&np->s_lock));
@@ -2465,11 +2470,20 @@ recov_retry:
 		 */
 		/* add mi to np's mntinfo4_list */
 		nfs4_add_mi_to_server(np, mi);
+		if (!recovery)
+			nfs_rw_exit(&mi->mi_recovlock);
 		mutex_exit(&np->s_lock);
 		nfs4_server_rele(np);
 		return;
 	}
 
+	/*
+	 * Drop the mi_recovlock since nfs4_start_op will
+	 * acquire it again for us.
+	 */
+	if (!recovery)
+		nfs_rw_exit(&mi->mi_recovlock);
+
 	if (!np)
 		np = new_nfs4_server(svp, cr);
 	else
@@ -2843,6 +2857,8 @@ nfs4_add_mi_to_server(nfs4_server_t *sp, mntinfo4_t *mi)
 	mntinfo4_t *tmi;
 	int in_list = 0;
 
+	ASSERT(nfs_rw_lock_held(&mi->mi_recovlock, RW_READER) ||
+	    nfs_rw_lock_held(&mi->mi_recovlock, RW_WRITER));
 	ASSERT(sp != &nfs4_server_lst);
 	ASSERT(MUTEX_HELD(&sp->s_lock));
 
diff --git a/usr/src/uts/common/nfs/nfs4_clnt.h b/usr/src/uts/common/nfs/nfs4_clnt.h
index 52088983cb..0d0a4d6ed0 100644
--- a/usr/src/uts/common/nfs/nfs4_clnt.h
+++ b/usr/src/uts/common/nfs/nfs4_clnt.h
@@ -848,6 +848,9 @@ typedef struct nfs4_debug_msg {
  *		mi_open_files
  *		mi_srvsettime
  *
+ *	The mntinfo4_t::mi_recovlock protects the following fields:
+ *		mi_srvsettime
+ *
  *	Locking order:
  *	  mi4_globals::mig_lock > mi_async_lock
  *	  mi_async_lock > nfs4_server_t::s_lock > mi_lock
@@ -856,6 +859,8 @@ typedef struct nfs4_debug_msg {
  *	  rnode4_t::r_rwlock > mi_rename_lock
  *	  nfs_rtable4_lock > mi_lock
  *	  nfs4_server_t::s_lock > mi_msg_list_lock
+ *	  mi_recovlock > nfs4_server_t::s_lock
+ *	  mi_recovlock > nfs4_server_lst_lock
  *
  * The 'mi_oo_list' represents the hash buckets that contain the
  * nfs4_open_owenrs for this particular mntinfo4.