Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 10 Jan 2002 19:23:13 -0800 (PST)
From:      Matthew Dillon <dillon@apollo.backplane.com>
To:        Alfred Perlstein <bright@mu.org>, "Alan L. Cox" <alc@imimic.com>
Cc:        FreeBSD-hackers@FreeBSD.ORG
Subject:   Need review of NFS patch set for server .. missing/wrong vput() issues
Message-ID:  <200201110323.g0B3NDt38028@apollo.backplane.com>

next in thread | raw e-mail | index | archive | help
    Heya Alfred, Alan, hackers.  Ok, I've been tracking down a bug with A
    russian news admin and I believe I may have found it (he's testing it),
    plus a few other bugs.

    I would like a review of this (for -stable, but applies to -current too):

    Patch section 1

	Here we were previously vput()ing nd.ni_vp only if error == 0.
	If error is returned non-zero from namei() this would normally be
	correct.  However, we force error on a number of occassions after
	namei() succeeds, in which case nd.ni_vp may be non-NULL and we
	must release it.  This fixes it so nd.ni_vp is vput()'d if it is
	non-NULL whether an error is specified at this point or not.

	(I believe this may have been Alexey's 'NFS hangs in inode state'
	problem, which occurs if you are running innd over an NFS filesystem)

    Patch section's 2 & 3

	Here namei() is called only with LOCKPARENT, which means that the
	leaf is not locked.  So when releasing the vnodes we should not
	have the if (vp == dvp) test, we should just vput() the dvp and
	vrele the vp.

	(Normally when you LOCKPARENT|LOCKLEAF and vp == dvp, only one
	vput should be performed, thus the test.  But we aren't locking
	the leaf here).

	GURUS:  Am I reading this correctly or does LOCKPARENT without a
	LOCKLEAF do something weird when dvp == vp?


Index: nfs/nfs_serv.c
===================================================================
RCS file: /home/ncvs/src/sys/nfs/Attic/nfs_serv.c,v
retrieving revision 1.93.2.2
diff -u -r1.93.2.2 nfs_serv.c
--- nfs/nfs_serv.c	28 Dec 2001 19:57:40 -0000	1.93.2.2
+++ nfs/nfs_serv.c	11 Jan 2002 03:15:51 -0000
@@ -2015,6 +2015,8 @@
 		error = VFS_VPTOFH(vp, &fhp->fh_fid);
 		if (!error)
 			error = VOP_GETATTR(vp, vap, cred, procp);
+	}
+	if (vp) {
 		vput(vp);
 		vp = NULL;
 		nd.ni_vp = NULL;
@@ -2467,14 +2469,15 @@
 		vrele(dirp);
 	if (vp)
 		vrele(vp);
-	if (nd.ni_dvp) {
-		if (nd.ni_dvp == nd.ni_vp)
-			vrele(nd.ni_dvp);
-		else
-			vput(nd.ni_dvp);
-	}
+
+	/*
+	 * since leaf is not locked, we can vput the parent even if
+	 * vp == dvp.
+	 */
 	if (nd.ni_vp)
 		vrele(nd.ni_vp);
+	if (nd.ni_dvp)
+		vput(nd.ni_dvp);
 	return(error);
 }
 
@@ -2636,10 +2639,11 @@
 nfsmout:
 	NDFREE(&nd, NDF_ONLY_PNBUF);
 	if (nd.ni_dvp) {
-		if (nd.ni_dvp == nd.ni_vp)
-			vrele(nd.ni_dvp);
-		else
-			vput(nd.ni_dvp);
+		/*
+		 * since leaf is not locked, we can vput the parent even if
+		 * vp == dvp.
+		 */
+		vput(nd.ni_dvp);
 	}
 	if (nd.ni_vp)
 		vrele(nd.ni_vp);

To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-hackers" in the body of the message




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