Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 27 May 2011 22:05:10 +0000 (UTC)
From:      Rick Macklem <rmacklem@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r222389 - in head/sys/fs: nfs nfsclient nfsserver
Message-ID:  <201105272205.p4RM5Aih054896@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: rmacklem
Date: Fri May 27 22:05:10 2011
New Revision: 222389
URL: http://svn.freebsd.org/changeset/base/222389

Log:
  Fix the new NFS client so that it handles NFSv4 state
  correctly during a forced dismount. This required that
  the exclusive and shared (refcnt) sleep lock functions check
  for MNTK_UMOUNTF before sleeping, so that they won't block
  while nfscl_umount() is getting rid of the state. As
  such, a "struct mount *" argument was added to the locking
  functions. I believe the only remaining case where a forced
  dismount can get hung in the kernel is when a thread is
  already attempting to do a TCP connect to a dead server
  when the krpc client structure called nr_client is NULL.
  This will only happen just after a "mount -u" with options
  that force a new TCP connection is done, so it shouldn't
  be a problem in practice.
  
  MFC after:	2 weeks

Modified:
  head/sys/fs/nfs/nfs_commonsubs.c
  head/sys/fs/nfs/nfs_var.h
  head/sys/fs/nfsclient/nfs_clcomsubs.c
  head/sys/fs/nfsclient/nfs_clstate.c
  head/sys/fs/nfsserver/nfs_nfsdsocket.c
  head/sys/fs/nfsserver/nfs_nfsdstate.c

Modified: head/sys/fs/nfs/nfs_commonsubs.c
==============================================================================
--- head/sys/fs/nfs/nfs_commonsubs.c	Fri May 27 21:45:21 2011	(r222388)
+++ head/sys/fs/nfs/nfs_commonsubs.c	Fri May 27 22:05:10 2011	(r222389)
@@ -1726,11 +1726,13 @@ nfsmout:
  * Any usecnt must be decremented by calling nfsv4_relref() before
  * calling nfsv4_lock(). It was done this way, so nfsv4_lock() could
  * be called in a loop.
- * The last argument is set to indicate if the call slept, iff not NULL.
+ * The isleptp argument is set to indicate if the call slept, iff not NULL
+ * and the mp argument indicates to check for a forced dismount, iff not
+ * NULL.
  */
 APPLESTATIC int
 nfsv4_lock(struct nfsv4lock *lp, int iwantlock, int *isleptp,
-    void *mutex)
+    void *mutex, struct mount *mp)
 {
 
 	if (isleptp)
@@ -1751,6 +1753,10 @@ nfsv4_lock(struct nfsv4lock *lp, int iwa
 	    lp->nfslock_lock |= NFSV4LOCK_LOCKWANTED;
 	}
 	while (lp->nfslock_lock & (NFSV4LOCK_LOCK | NFSV4LOCK_LOCKWANTED)) {
+		if (mp != NULL && (mp->mnt_kern_flag & MNTK_UNMOUNTF) != 0) {
+			lp->nfslock_lock &= ~NFSV4LOCK_LOCKWANTED;
+			return (0);
+		}
 		lp->nfslock_lock |= NFSV4LOCK_WANTED;
 		if (isleptp)
 			*isleptp = 1;
@@ -1801,9 +1807,12 @@ nfsv4_relref(struct nfsv4lock *lp)
  * not wait for threads that want the exclusive lock. If priority needs
  * to be given to threads that need the exclusive lock, a call to nfsv4_lock()
  * with the 2nd argument == 0 should be done before calling nfsv4_getref().
+ * If the mp argument is not NULL, check for MNTK_UNMOUNTF being set and
+ * return without getting a refcnt for that case.
  */
 APPLESTATIC void
-nfsv4_getref(struct nfsv4lock *lp, int *isleptp, void *mutex)
+nfsv4_getref(struct nfsv4lock *lp, int *isleptp, void *mutex,
+    struct mount *mp)
 {
 
 	if (isleptp)
@@ -1813,12 +1822,16 @@ nfsv4_getref(struct nfsv4lock *lp, int *
 	 * Wait for a lock held.
 	 */
 	while (lp->nfslock_lock & NFSV4LOCK_LOCK) {
+		if (mp != NULL && (mp->mnt_kern_flag & MNTK_UNMOUNTF) != 0)
+			return;
 		lp->nfslock_lock |= NFSV4LOCK_WANTED;
 		if (isleptp)
 			*isleptp = 1;
 		(void) nfsmsleep(&lp->nfslock_lock, mutex,
 		    PZERO - 1, "nfsv4lck", NULL);
 	}
+	if (mp != NULL && (mp->mnt_kern_flag & MNTK_UNMOUNTF) != 0)
+		return;
 
 	lp->nfslock_usecnt++;
 }

Modified: head/sys/fs/nfs/nfs_var.h
==============================================================================
--- head/sys/fs/nfs/nfs_var.h	Fri May 27 21:45:21 2011	(r222388)
+++ head/sys/fs/nfs/nfs_var.h	Fri May 27 22:05:10 2011	(r222389)
@@ -247,10 +247,10 @@ int nfsv4_loadattr(struct nfsrv_descript
     struct nfsv3_pathconf *, struct statfs *, struct nfsstatfs *,
     struct nfsfsinfo *, NFSACL_T *,
     int, int *, u_int32_t *, u_int32_t *, NFSPROC_T *, struct ucred *);
-int nfsv4_lock(struct nfsv4lock *, int, int *, void *);
+int nfsv4_lock(struct nfsv4lock *, int, int *, void *, struct mount *);
 void nfsv4_unlock(struct nfsv4lock *, int);
 void nfsv4_relref(struct nfsv4lock *);
-void nfsv4_getref(struct nfsv4lock *, int *, void *);
+void nfsv4_getref(struct nfsv4lock *, int *, void *, struct mount *);
 int nfsv4_getref_nonblock(struct nfsv4lock *);
 int nfsv4_testlock(struct nfsv4lock *);
 int nfsrv_mtostr(struct nfsrv_descript *, char *, int);

Modified: head/sys/fs/nfsclient/nfs_clcomsubs.c
==============================================================================
--- head/sys/fs/nfsclient/nfs_clcomsubs.c	Fri May 27 21:45:21 2011	(r222388)
+++ head/sys/fs/nfsclient/nfs_clcomsubs.c	Fri May 27 22:05:10 2011	(r222389)
@@ -482,7 +482,7 @@ nfscl_lockexcl(struct nfsv4lock *lckp, v
 	int igotlock;
 
 	do {
-		igotlock = nfsv4_lock(lckp, 1, NULL, mutex);
+		igotlock = nfsv4_lock(lckp, 1, NULL, mutex, NULL);
 	} while (!igotlock);
 }
 

Modified: head/sys/fs/nfsclient/nfs_clstate.c
==============================================================================
--- head/sys/fs/nfsclient/nfs_clstate.c	Fri May 27 21:45:21 2011	(r222388)
+++ head/sys/fs/nfsclient/nfs_clstate.c	Fri May 27 22:05:10 2011	(r222389)
@@ -687,11 +687,14 @@ nfscl_getcl(vnode_t vp, struct ucred *cr
 	struct nfsclclient *clp;
 	struct nfsclclient *newclp = NULL;
 	struct nfscllockowner *lp, *nlp;
-	struct nfsmount *nmp = VFSTONFS(vnode_mount(vp));
+	struct mount *mp;
+	struct nfsmount *nmp;
 	char uuid[HOSTUUIDLEN];
 	int igotlock = 0, error, trystalecnt, clidinusedelay, i;
 	u_int16_t idlen = 0;
 
+	mp = vnode_mount(vp);
+	nmp = VFSTONFS(mp);
 	if (cred != NULL) {
 		getcredhostuuid(cred, uuid, sizeof uuid);
 		idlen = strlen(uuid);
@@ -704,6 +707,17 @@ nfscl_getcl(vnode_t vp, struct ucred *cr
 		    M_WAITOK);
 	}
 	NFSLOCKCLSTATE();
+	/*
+	 * If a forced dismount is already in progress, don't
+	 * allocate a new clientid and get out now. For the case where
+	 * clp != NULL, this is a harmless optimization.
+	 */
+	if ((mp->mnt_kern_flag & MNTK_UNMOUNTF) != 0) {
+		NFSUNLOCKCLSTATE();
+		if (newclp != NULL)
+			free(newclp, M_NFSCLCLIENT);
+		return (EBADF);
+	}
 	clp = nmp->nm_clp;
 	if (clp == NULL) {
 		if (newclp == NULL) {
@@ -736,9 +750,21 @@ nfscl_getcl(vnode_t vp, struct ucred *cr
 	NFSLOCKCLSTATE();
 	while ((clp->nfsc_flags & NFSCLFLAGS_HASCLIENTID) == 0 && !igotlock)
 		igotlock = nfsv4_lock(&clp->nfsc_lock, 1, NULL,
-		    NFSCLSTATEMUTEXPTR);
+		    NFSCLSTATEMUTEXPTR, mp);
 	if (!igotlock)
-		nfsv4_getref(&clp->nfsc_lock, NULL, NFSCLSTATEMUTEXPTR);
+		nfsv4_getref(&clp->nfsc_lock, NULL, NFSCLSTATEMUTEXPTR, mp);
+	if (igotlock == 0 && (mp->mnt_kern_flag & MNTK_UNMOUNTF) != 0) {
+		/*
+		 * Both nfsv4_lock() and nfsv4_getref() know to check
+		 * for MNTK_UNMOUNTF and return without sleeping to
+		 * wait for the exclusive lock to be released, since it
+		 * might be held by nfscl_umount() and we need to get out
+		 * now for that case and not wait until nfscl_umount()
+		 * releases it.
+		 */
+		NFSUNLOCKCLSTATE();
+		return (EBADF);
+	}
 	NFSUNLOCKCLSTATE();
 
 	/*
@@ -1713,6 +1739,7 @@ nfscl_cleanupkext(struct nfsclclient *cl
 }
 #endif	/* APPLEKEXT || __FreeBSD__ */
 
+static int	fake_global;	/* Used to force visibility of MNTK_UNMOUNTF */
 /*
  * Called from nfs umount to free up the clientid.
  */
@@ -1723,6 +1750,33 @@ nfscl_umount(struct nfsmount *nmp, NFSPR
 	struct ucred *cred;
 	int igotlock;
 
+	/*
+	 * For the case that matters, this is the thread that set
+	 * MNTK_UNMOUNTF, so it will see it set. The code that follows is
+	 * done to ensure that any thread executing nfscl_getcl() after
+	 * this time, will see MNTK_UNMOUNTF set. nfscl_getcl() uses the
+	 * mutex for NFSLOCKCLSTATE(), so it is "m" for the following
+	 * explanation, courtesy of Alan Cox.
+	 * What follows is a snippet from Alan Cox's email at:
+	 * http://docs.FreeBSD.org/cgi/
+	 *     mid.cgi?BANLkTikR3d65zPHo9==08ZfJ2vmqZucEvw
+	 * 
+	 * 1. Set MNTK_UNMOUNTF
+	 * 2. Acquire a standard FreeBSD mutex "m".
+	 * 3. Update some data structures.
+	 * 4. Release mutex "m".
+	 * 
+	 * Then, other threads that acquire "m" after step 4 has occurred will
+	 * see MNTK_UNMOUNTF as set.  But, other threads that beat thread X to
+	 * step 2 may or may not see MNTK_UNMOUNTF as set.
+	 */
+	NFSLOCKCLSTATE();
+	if ((nmp->nm_mountp->mnt_kern_flag & MNTK_UNMOUNTF) != 0) {
+		fake_global++;
+		NFSUNLOCKCLSTATE();
+		NFSLOCKCLSTATE();
+	}
+
 	clp = nmp->nm_clp;
 	if (clp != NULL) {
 		if ((clp->nfsc_flags & NFSCLFLAGS_INITED) == 0)
@@ -1734,12 +1788,16 @@ nfscl_umount(struct nfsmount *nmp, NFSPR
 		 */
 		clp->nfsc_flags |= NFSCLFLAGS_UMOUNT;
 		while (clp->nfsc_flags & NFSCLFLAGS_HASTHREAD)
-			(void) tsleep((caddr_t)clp, PWAIT, "nfsclumnt", hz);
+			(void)mtx_sleep(clp, NFSCLSTATEMUTEXPTR, PWAIT,
+			    "nfsclumnt", hz);
 	
-		NFSLOCKCLSTATE();
+		/*
+		 * Now, get the exclusive lock on the client state, so
+		 * that no uses of the state are still in progress.
+		 */
 		do {
 			igotlock = nfsv4_lock(&clp->nfsc_lock, 1, NULL,
-			    NFSCLSTATEMUTEXPTR);
+			    NFSCLSTATEMUTEXPTR, NULL);
 		} while (!igotlock);
 		NFSUNLOCKCLSTATE();
 	
@@ -1756,8 +1814,8 @@ nfscl_umount(struct nfsmount *nmp, NFSPR
 		nmp->nm_clp = NULL;
 		NFSFREECRED(cred);
 		FREE((caddr_t)clp, M_NFSCLCLIENT);
-	}
-
+	} else
+		NFSUNLOCKCLSTATE();
 }
 
 /*
@@ -1790,7 +1848,7 @@ nfscl_recover(struct nfsclclient *clp, s
 	clp->nfsc_flags |= NFSCLFLAGS_RECVRINPROG;
 	do {
 		igotlock = nfsv4_lock(&clp->nfsc_lock, 1, NULL,
-		    NFSCLSTATEMUTEXPTR);
+		    NFSCLSTATEMUTEXPTR, NULL);
 	} while (!igotlock);
 	NFSUNLOCKCLSTATE();
 
@@ -2105,7 +2163,7 @@ nfscl_hasexpired(struct nfsclclient *clp
 	clp->nfsc_flags |= NFSCLFLAGS_EXPIREIT;
 	do {
 		igotlock = nfsv4_lock(&clp->nfsc_lock, 1, NULL,
-		    NFSCLSTATEMUTEXPTR);
+		    NFSCLSTATEMUTEXPTR, NULL);
 	} while (!igotlock && (clp->nfsc_flags & NFSCLFLAGS_EXPIREIT));
 	if ((clp->nfsc_flags & NFSCLFLAGS_EXPIREIT) == 0) {
 		if (igotlock)
@@ -2464,7 +2522,7 @@ tryagain:
 				}
 				while (!igotlock) {
 				    igotlock = nfsv4_lock(&clp->nfsc_lock, 1,
-					&islept, NFSCLSTATEMUTEXPTR);
+					&islept, NFSCLSTATEMUTEXPTR, NULL);
 				    if (islept)
 					goto tryagain;
 				}
@@ -2556,14 +2614,18 @@ tryagain:
 		}
 #endif	/* APPLEKEXT || __FreeBSD__ */
 
+		NFSLOCKCLSTATE();
 		if ((clp->nfsc_flags & NFSCLFLAGS_RECOVER) == 0)
-		    (void) tsleep((caddr_t)clp, PWAIT, "nfscl", hz);
+			(void)mtx_sleep(clp, NFSCLSTATEMUTEXPTR, PWAIT, "nfscl",
+			    hz);
 		if (clp->nfsc_flags & NFSCLFLAGS_UMOUNT) {
-			NFSFREECRED(cred);
 			clp->nfsc_flags &= ~NFSCLFLAGS_HASTHREAD;
+			NFSUNLOCKCLSTATE();
+			NFSFREECRED(cred);
 			wakeup((caddr_t)clp);
 			return;
 		}
+		NFSUNLOCKCLSTATE();
 	}
 }
 
@@ -3864,7 +3926,7 @@ nfscl_removedeleg(vnode_t vp, NFSPROC_T 
 			islept = 0;
 			while (!igotlock) {
 			    igotlock = nfsv4_lock(&clp->nfsc_lock, 1,
-				&islept, NFSCLSTATEMUTEXPTR);
+				&islept, NFSCLSTATEMUTEXPTR, NULL);
 			    if (islept)
 				break;
 			}
@@ -3963,7 +4025,7 @@ nfscl_renamedeleg(vnode_t fvp, nfsv4stat
 			islept = 0;
 			while (!igotlock) {
 			    igotlock = nfsv4_lock(&clp->nfsc_lock, 1,
-				&islept, NFSCLSTATEMUTEXPTR);
+				&islept, NFSCLSTATEMUTEXPTR, NULL);
 			    if (islept)
 				break;
 			}
@@ -4043,7 +4105,7 @@ nfscl_getref(struct nfsmount *nmp)
 		NFSUNLOCKCLSTATE();
 		return (0);
 	}
-	nfsv4_getref(&clp->nfsc_lock, NULL, NFSCLSTATEMUTEXPTR);
+	nfsv4_getref(&clp->nfsc_lock, NULL, NFSCLSTATEMUTEXPTR, NULL);
 	NFSUNLOCKCLSTATE();
 	return (1);
 }

Modified: head/sys/fs/nfsserver/nfs_nfsdsocket.c
==============================================================================
--- head/sys/fs/nfsserver/nfs_nfsdsocket.c	Fri May 27 21:45:21 2011	(r222388)
+++ head/sys/fs/nfsserver/nfs_nfsdsocket.c	Fri May 27 22:05:10 2011	(r222389)
@@ -525,10 +525,10 @@ nfsrvd_compound(struct nfsrv_descript *n
 	NFSLOCKV4ROOTMUTEX();
 	if (nfsrv_stablefirst.nsf_flags & NFSNSF_NEEDLOCK)
 		igotlock = nfsv4_lock(&nfsv4rootfs_lock, 1, NULL,
-		    NFSV4ROOTLOCKMUTEXPTR);
+		    NFSV4ROOTLOCKMUTEXPTR, NULL);
 	else
 		igotlock = nfsv4_lock(&nfsv4rootfs_lock, 0, NULL,
-		    NFSV4ROOTLOCKMUTEXPTR);
+		    NFSV4ROOTLOCKMUTEXPTR, NULL);
 	NFSUNLOCKV4ROOTMUTEX();
 	if (igotlock) {
 		/*
@@ -576,7 +576,7 @@ nfsrvd_compound(struct nfsrv_descript *n
 		 */
 		NFSLOCKV4ROOTMUTEX();
 		nfsv4_getref(&nfsv4rootfs_lock, NULL,
-		    NFSV4ROOTLOCKMUTEXPTR);
+		    NFSV4ROOTLOCKMUTEXPTR, NULL);
 		NFSUNLOCKV4ROOTMUTEX();
 	}
 

Modified: head/sys/fs/nfsserver/nfs_nfsdstate.c
==============================================================================
--- head/sys/fs/nfsserver/nfs_nfsdstate.c	Fri May 27 21:45:21 2011	(r222388)
+++ head/sys/fs/nfsserver/nfs_nfsdstate.c	Fri May 27 22:05:10 2011	(r222389)
@@ -169,7 +169,7 @@ nfsrv_setclient(struct nfsrv_descript *n
 	nfsv4_relref(&nfsv4rootfs_lock);
 	do {
 		igotlock = nfsv4_lock(&nfsv4rootfs_lock, 1, NULL,
-		    NFSV4ROOTLOCKMUTEXPTR);
+		    NFSV4ROOTLOCKMUTEXPTR, NULL);
 	} while (!igotlock);
 	NFSUNLOCKV4ROOTMUTEX();
 
@@ -419,7 +419,7 @@ nfsrv_getclient(nfsquad_t clientid, int 
 		nfsv4_relref(&nfsv4rootfs_lock);
 		do {
 			igotlock = nfsv4_lock(&nfsv4rootfs_lock, 1, NULL,
-			    NFSV4ROOTLOCKMUTEXPTR);
+			    NFSV4ROOTLOCKMUTEXPTR, NULL);
 		} while (!igotlock);
 		NFSUNLOCKV4ROOTMUTEX();
 	} else if (opflags != CLOPS_RENEW) {
@@ -548,7 +548,7 @@ nfsrv_adminrevoke(struct nfsd_clid *revo
 	NFSLOCKV4ROOTMUTEX();
 	do {
 		igotlock = nfsv4_lock(&nfsv4rootfs_lock, 1, NULL,
-		    NFSV4ROOTLOCKMUTEXPTR);
+		    NFSV4ROOTLOCKMUTEXPTR, NULL);
 	} while (!igotlock);
 	NFSUNLOCKV4ROOTMUTEX();
 
@@ -608,7 +608,7 @@ nfsrv_dumpclients(struct nfsd_dumpclient
 	 * exclusive lock cannot be acquired while dumping the clients.
 	 */
 	NFSLOCKV4ROOTMUTEX();
-	nfsv4_getref(&nfsv4rootfs_lock, NULL, NFSV4ROOTLOCKMUTEXPTR);
+	nfsv4_getref(&nfsv4rootfs_lock, NULL, NFSV4ROOTLOCKMUTEXPTR, NULL);
 	NFSUNLOCKV4ROOTMUTEX();
 	NFSLOCKSTATE();
 	/*
@@ -709,7 +709,7 @@ nfsrv_dumplocks(vnode_t vp, struct nfsd_
 	 * exclusive lock on it cannot be acquired while dumping the locks.
 	 */
 	NFSLOCKV4ROOTMUTEX();
-	nfsv4_getref(&nfsv4rootfs_lock, NULL, NFSV4ROOTLOCKMUTEXPTR);
+	nfsv4_getref(&nfsv4rootfs_lock, NULL, NFSV4ROOTLOCKMUTEXPTR, NULL);
 	NFSUNLOCKV4ROOTMUTEX();
 	NFSLOCKSTATE();
 	if (!ret)
@@ -4254,7 +4254,7 @@ nfsrv_clientconflict(struct nfsclient *c
 		nfsv4_relref(&nfsv4rootfs_lock);
 		do {
 			gotlock = nfsv4_lock(&nfsv4rootfs_lock, 1, NULL,
-			    NFSV4ROOTLOCKMUTEXPTR);
+			    NFSV4ROOTLOCKMUTEXPTR, NULL);
 		} while (!gotlock);
 		NFSUNLOCKV4ROOTMUTEX();
 		*haslockp = 1;
@@ -4422,7 +4422,7 @@ nfsrv_delegconflict(struct nfsstate *stp
 		nfsv4_relref(&nfsv4rootfs_lock);
 		do {
 			gotlock = nfsv4_lock(&nfsv4rootfs_lock, 1, NULL,
-			    NFSV4ROOTLOCKMUTEXPTR);
+			    NFSV4ROOTLOCKMUTEXPTR, NULL);
 		} while (!gotlock);
 		NFSUNLOCKV4ROOTMUTEX();
 		*haslockp = 1;
@@ -4616,7 +4616,7 @@ nfsd_recalldelegation(vnode_t vp, NFSPRO
 	 * exclusive lock cannot be acquired by another thread.
 	 */
 	NFSLOCKV4ROOTMUTEX();
-	nfsv4_getref(&nfsv4rootfs_lock, NULL, NFSV4ROOTLOCKMUTEXPTR);
+	nfsv4_getref(&nfsv4rootfs_lock, NULL, NFSV4ROOTLOCKMUTEXPTR, NULL);
 	NFSUNLOCKV4ROOTMUTEX();
 
 	/*
@@ -5179,7 +5179,7 @@ nfsrv_locklf(struct nfslockfile *lfp)
 	lfp->lf_usecount++;
 	do {
 		gotlock = nfsv4_lock(&lfp->lf_locallock_lck, 1, NULL,
-		    NFSSTATEMUTEXPTR);
+		    NFSSTATEMUTEXPTR, NULL);
 	} while (gotlock == 0);
 	lfp->lf_usecount--;
 }



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201105272205.p4RM5Aih054896>