[PATCH 1/2] vfs: avoid spurious vref/vrele for absolute lookups

[Mateusz Guzik]

From: Mateusz Guzik <mjg at freebsd.org>

namei used to vref fd_cdir, which was immediatley vrele'd on entry to
the loop.

Simplify error handling and remove type checking for ni_startdir vnode.
It is only set by nfs which does the check on its own. Assert the
correct type instead.
---
 sys/kern/vfs_lookup.c | 92 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 51 insertions(+), 41 deletions(-)

diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
index 5dc07dc..c5218ec 100644
--- a/sys/kern/vfs_lookup.c
+++ b/sys/kern/vfs_lookup.c
@@ -109,6 +109,27 @@ namei_cleanup_cnp(struct componentname *cnp)
 #endif
 }
 
+static int
+namei_handle_root(struct nameidata *ndp, struct vnode **dpp)
+{
+	struct componentname *cnp = &ndp->ni_cnd;
+
+	if (ndp->ni_strictrelative != 0) {
+#ifdef KTRACE
+		if (KTRPOINT(curthread, KTR_CAPFAIL))
+			ktrcapfail(CAPFAIL_LOOKUP, NULL, NULL);
+#endif
+		return (ENOTCAPABLE);
+	}
+	while (*(cnp->cn_nameptr) == '/') {
+		cnp->cn_nameptr++;
+		ndp->ni_pathlen--;
+	}
+	*dpp = ndp->ni_rootdir;
+	VREF(*dpp);
+	return (0);
+}
+
 /*
  * Convert a pathname into a pointer to a locked vnode.
  *
@@ -148,6 +169,8 @@ namei(struct nameidata *ndp)
 	    ("namei: nameiop contaminated with flags"));
 	KASSERT((cnp->cn_flags & OPMASK) == 0,
 	    ("namei: flags contaminated with nameiops"));
+	if (ndp->ni_startdir != NULL)
+		MPASS(ndp->ni_startdir->v_type == VDIR);
 	if (!lookup_shared)
 		cnp->cn_flags &= ~LOCKSHARED;
 	fdp = p->p_fd;
@@ -220,12 +243,16 @@ namei(struct nameidata *ndp)
 	if (cnp->cn_flags & AUDITVNODE2)
 		AUDIT_ARG_UPATH2(td, ndp->ni_dirfd, cnp->cn_pnbuf);
 
-	dp = NULL;
-	if (cnp->cn_pnbuf[0] != '/') {
+	cnp->cn_nameptr = cnp->cn_pnbuf;
+	if (cnp->cn_pnbuf[0] == '/') {
+		error = namei_handle_root(ndp, &dp);
+	} else {
 		if (ndp->ni_startdir != NULL) {
 			dp = ndp->ni_startdir;
-			error = 0;
-		} else if (ndp->ni_dirfd != AT_FDCWD) {
+		} else if (ndp->ni_dirfd == AT_FDCWD) {
+			dp = fdp->fd_cdir;
+			VREF(dp);
+		} else {
 			cap_rights_t rights;
 
 			rights = ndp->ni_rightsneeded;
@@ -251,51 +278,22 @@ namei(struct nameidata *ndp)
 				ndp->ni_strictrelative = 1;
 			}
 #endif
-		}
-		if (error != 0 || dp != NULL) {
-			FILEDESC_SUNLOCK(fdp);
-			if (error == 0 && dp->v_type != VDIR) {
-				vrele(dp);
+			if (error == 0 && dp->v_type != VDIR)
 				error = ENOTDIR;
-			}
-		}
-		if (error) {
-			namei_cleanup_cnp(cnp);
-			return (error);
 		}
 	}
-	if (dp == NULL) {
-		dp = fdp->fd_cdir;
-		VREF(dp);
-		FILEDESC_SUNLOCK(fdp);
-		if (ndp->ni_startdir != NULL)
+	FILEDESC_SUNLOCK(fdp);
+	if (error != 0) {
+		if (dp != NULL)
+			vrele(dp);
+		if (ndp->ni_startdir != NULL && dp != ndp->ni_startdir)
 			vrele(ndp->ni_startdir);
+		namei_cleanup_cnp(cnp);
+		return (error);
 	}
 	SDT_PROBE(vfs, namei, lookup, entry, dp, cnp->cn_pnbuf,
 	    cnp->cn_flags, 0, 0);
 	for (;;) {
-		/*
-		 * Check if root directory should replace current directory.
-		 * Done at start of translation and after symbolic link.
-		 */
-		cnp->cn_nameptr = cnp->cn_pnbuf;
-		if (*(cnp->cn_nameptr) == '/') {
-			vrele(dp);
-			if (ndp->ni_strictrelative != 0) {
-#ifdef KTRACE
-				if (KTRPOINT(curthread, KTR_CAPFAIL))
-					ktrcapfail(CAPFAIL_LOOKUP, NULL, NULL);
-#endif
-				namei_cleanup_cnp(cnp);
-				return (ENOTCAPABLE);
-			}
-			while (*(cnp->cn_nameptr) == '/') {
-				cnp->cn_nameptr++;
-				ndp->ni_pathlen--;
-			}
-			dp = ndp->ni_rootdir;
-			VREF(dp);
-		}
 		ndp->ni_startdir = dp;
 		error = lookup(ndp);
 		if (error) {
@@ -370,6 +368,18 @@ namei(struct nameidata *ndp)
 		ndp->ni_pathlen += linklen;
 		vput(ndp->ni_vp);
 		dp = ndp->ni_dvp;
+		/*
+		 * Check if root directory should replace current directory.
+		 */
+		cnp->cn_nameptr = cnp->cn_pnbuf;
+		if (*(cnp->cn_nameptr) == '/') {
+			vrele(dp);
+			error = namei_handle_root(ndp, &dp);
+			if (error != 0) {
+				namei_cleanup_cnp(cnp);
+				return (error);
+			}
+		}
 	}
 	namei_cleanup_cnp(cnp);
 	vput(ndp->ni_vp);
-- 
2.4.5