Boost subshell performance by avoiding expensive pwd related syscalls (#805)

In ksh93v- a sh.pwdfd variable was introduced, ostensibly for
implementing 'cd -f', 'pwd -f' and '${.sh.pwdfd}' as new features.
I might backport these in the future, but those aren't the focus of
this commit. Rather, what caught my interest was the effect of this
change on subshell performance (results from shbench[*] with 20
iterations, compiled with CCFLAGS='-O2 -fPIC'):

-------------------------------------------------------------
name           /tmp/ksh93u+2012-08-01  /tmp/ksh93u+m-dev
-------------------------------------------------------------
fibonacci.ksh  0.230 [0.227-0.235]     0.216 [0.21f1-0.222]
parens.ksh     0.208 [0.204-0.212]     0.173 [0.170-0.182]
-------------------------------------------------------------
name           /tmp/ksh93v-2012-08-24  /tmp/ksh93v-2014-12-24
-------------------------------------------------------------
fibonacci.ksh  0.269 [0.266-0.273]     0.247 [0.242-0.251]
parens.ksh     0.147 [0.145-0.149]     0.125 [0.121-0.127]
-------------------------------------------------------------
name           /tmp/ksh93u+redhat
-------------------------------------------------------------
fibonacci.ksh  0.264 [0.260-0.269]
parens.ksh     0.140 [0.137-0.144]
-------------------------------------------------------------

As can be seen above, 93u+ and 93u+m perform worse in the parens
benchmark than the ksh93v- release which implements the change, as
well as a copy of ksh93u+ compiled with Red Hat's patches for cd
(fdstatus, rmdirfix, cdfix and cdfix2 applied in that order).
However, 93u+ and 93u+m perform better in the fibonacci benchmark.

93v- is faster in the parens benchmark because it obtains the file
descriptor for the current working directory via openat(2) in
sh_diropenat, then caches it in sh.pwdfd. This fd is then reused in
sh_subshell(), which allows 93v- to avoid taxing open(2) and
fcntl(2) syscalls.

However, the 93v- implementation has a flaw. Unlike in 93u+, it
always duplicates sh.pwdfd with fcntl, even when it's a superfluous
operation. This is the cause of the performance regression in the
fibonacci benchmark.

To increase ksh93u+m's performance beyond both 93u+ and 93v-, I
have backported 93v-'s code, but with a crucial change:
- sh_subshell() no longer duplicates sh.pwdfd with fcntl during
  virtual subshell scope initialization. Rather, a sh_pwdupdate
  function is used in tandem with sh_diropenat in b_cd() to do all
  of this when the directory is being changed. This fixes the
  regression in the fibonacci benchmark. sh_pwdupdate will be
  called whenever sh.pwdfd changes to prevent file descriptor leaks
  while also avoiding erroneous close operations on the parent
  shell's file descriptor for the PWD.

Other changes applied include:
- The removal of support for systems which lack fchdir. ksh93v- did
  this twelve years ago in the oldest archived alpha release, which
  I think is a more than long enough deprecation window.
- If sh.pwdfd is -1 (i.e., the shell was initialized in a
  nonexistent directory), don't fork preemptively. It's more
  efficient to let cd check sp->pwdfd via a short function before
  attempting to change the directory. This allows subshells run in
  nonexistent directories to avoid forking, provided cd is never
  invoked.
- Removed <=1995 hackery from nv_open and env_init. During SH_INIT
  we should simply always invoke path_pwd(); giving $PWD NV_TAGGED
  is not just unnecessary, but possibly harmful when one attempts
  to use 'typeset -t' with the PWD variable.
- With the removal of that code, sh.pwd is guaranteed to get set
  during shell initialization (if it wasn't already). As such, the
  code no longer needs to cope with a NULL pwd and many such checks
  have been removed.
- sh.pwd no longer has a pointless and misleading const modifier.
  This allows for the elimination of many casts involving sh.pwd.
- sh.pwdfd is reset every time the pwd is (re)initialized in
  path_pwd().
- libast/features/fcntl.c: Account for systems which don't
  implement O_SEARCH and/or O_DIRECTORY. (I avoided backporting the
  ksh93v- feature test code because it's ginormous and filled with
  far too much compatibility hackery. If anyone wants to backport
  the mess that are the libast syscall intercepts, be my guest. In
  any case ksh doesn't need any of that to function correctly.)
- Goat sacrifices are hereby banned from 93u+m (for the curious,
  please see att#567).

With these changes ksh93u+m now handily beats the previous ksh
releases in the fibonacci and parens benchmarks (ksh2020 included
for comparison):

-------------------------------------------------------------
name           /tmp/ksh93u+2012-08-01  /tmp/ksh93v-2014-12-24
-------------------------------------------------------------
fibonacci.ksh  0.230 [0.224-0.236]     0.247 [0.244-0.252]
parens.ksh     0.207 [0.203-0.213]     0.124 [0.121-0.127]
-------------------------------------------------------------
name           /tmp/ksh2020            /tmp/ksh93u+m-dev
-------------------------------------------------------------
fibonacci.ksh  0.369 [0.364-0.375]     0.217 [0.213-0.222]
parens.ksh     0.165 [0.162-0.169]     0.173 [0.171-0.176]
-------------------------------------------------------------
name           /tmp/ksh93u+m-patch
-------------------------------------------------------------
fibonacci.ksh  0.202 [0.198-0.206]
parens.ksh     0.082 [0.080-0.085]
-------------------------------------------------------------

[*] shbench: http://fossil.0branch.com/csb

Author: JohnoKing

src/cmd/ksh93/bltins/cd_pwd.c

@@ -46,14 +46,32 @@ static void rehash(Namval_t *np,void *data)
 		nv_rehash(np,data);
 }
 
+/*
+ * Obtain a file handle to the directory "path" relative to directory "dir"
+ */
+int sh_diropenat(int dir, const char *path)
+{
+	int fd,shfd;
+	if((fd = openat(dir, path, O_DIRECTORY|O_NONBLOCK|O_cloexec)) < 0)
+#if O_SEARCH
+		if(errno != EACCES || (fd = openat(dir, path, O_SEARCH|O_DIRECTORY|O_NONBLOCK|O_cloexec)) < 0)
+#endif
+			return fd;
+	/* Move fd to a number > 10 and register the fd number with the shell */
+	shfd = sh_fcntl(fd, F_dupfd_cloexec, 10);
+	close(fd);
+	return shfd;
+}
+
 int	b_cd(int argc, char *argv[],Shbltin_t *context)
 {
 	char *dir;
 	Pathcomp_t *cdpath = 0;
 	const char *dp;
 	int saverrno=0;
 	int rval,pflag=0,eflag=0,ret=1;
-	char *oldpwd;
+	int newdirfd;
+	char *oldpwd, *cp;
 	Namval_t *opwdnod, *pwdnod;
 	NOT_USED(context);
 	while((rval = optget(argv,sh_optcd))) switch(rval)
@@ -118,13 +136,8 @@ int	b_cd(int argc, char *argv[],Shbltin_t *context)
 	 * If sh_subshell() in subshell.c cannot use fchdir(2) to restore the PWD using a saved file descriptor,
 	 * we must fork any virtual subshell now to avoid the possibility of ending up in the wrong PWD on exit.
 	 */
-	if(sh.subshell && !sh.subshare)
-	{
-#if _lib_fchdir
-		if(!test_inode(sh.pwd,e_dot))
-#endif
-			sh_subfork();
-	}
+	if(sh.subshell && !sh.subshare && (!sh_validate_subpwdfd() || !test_inode(sh.pwd,e_dot)))
+		sh_subfork();
 	/*
 	 * Do $CDPATH processing, except if the path is absolute or the first component is '.' or '..'
 	 */
@@ -181,21 +194,49 @@ int	b_cd(int argc, char *argv[],Shbltin_t *context)
 		}
 		if(!pflag)
 		{
-			char *cp;
 			stkseek(sh.stk,PATH_MAX+PATH_OFFSET);
 			if(*(cp=stkptr(sh.stk,PATH_OFFSET))=='/')
 				if(!pathcanon(cp,PATH_DOTDOT))
 					continue;
 		}
-		if((rval=chdir(path_relative(stkptr(sh.stk,PATH_OFFSET)))) >= 0)
-			goto success;
-		if(errno!=ENOENT && saverrno==0)
+		cp = path_relative(stkptr(sh.stk,PATH_OFFSET));
+		rval = newdirfd = sh_diropenat((sh.pwdfd>0)?sh.pwdfd:AT_FDCWD,cp);
+		if(newdirfd>0)
+		{
+			/* chdir for directories on HSM/tapeworms may take minutes */
+			if((rval=fchdir(newdirfd)) >= 0)
+			{
+				sh_pwdupdate(newdirfd);
+				goto success;
+			}
+			sh_close(newdirfd);
+		}
+#if !O_SEARCH
+		else if((rval=chdir(cp)) >= 0)
+			sh_pwdupdate(sh_diropenat(AT_FDCWD,cp));
+#endif
+		if(saverrno==0)
 			saverrno=errno;
 	}
 	while(cdpath);
 	if(rval<0 && *dir=='/' && *(path_relative(stkptr(sh.stk,PATH_OFFSET)))!='/')
-		rval = chdir(dir);
-	/* use absolute chdir() if relative chdir() fails */
+	{
+		rval = newdirfd = sh_diropenat((sh.pwdfd>0)?sh.pwdfd:AT_FDCWD,dir);
+		if(newdirfd>0)
+		{
+			/* chdir for directories on HSM/tapeworms may take minutes */
+			if((rval=fchdir(newdirfd)) >= 0)
+			{
+				sh_pwdupdate(newdirfd);
+				goto success;
+			}
+			sh_close(newdirfd);
+		}
+#if !O_SEARCH
+		else if((rval=chdir(dir)) >= 0)
+			sh_pwdupdate(sh_diropenat(AT_FDCWD,dir));
+#endif
+	}
 	if(rval<0)
 	{
 		if(saverrno)
@@ -221,7 +262,7 @@ int	b_cd(int argc, char *argv[],Shbltin_t *context)
 	if(*dp && (*dp!='.'||dp[1]) && strchr(dir,'/'))
 		sfputr(sfstdout,dir,'\n');
 	nv_putval(opwdnod,oldpwd,NV_RDONLY);
-	free((void*)sh.pwd);
+	free(sh.pwd);
 	if(*dir == '/')
 	{
 		size_t len = strlen(dir);

src/cmd/ksh93/features/externs

@@ -1,6 +1,6 @@
 hdr	nc
 mem	exception.name,_exception.name math.h
-lib	setreuid,setregid,nice,fork,fchdir
+lib	setreuid,setregid,nice,fork
 lib	pathnative,pathposix
 lib	memcntl sys/mman.h
 

src/cmd/ksh93/include/defs.h

@@ -113,6 +113,7 @@ extern struct dolnod	*sh_arguse(void);
 extern char		*sh_checkid(char*,char*);
 extern void		sh_chktrap(void);
 extern int		sh_debug(const char*,const char*,const char*,char *const[],int);
+extern int		sh_diropenat(int,const char *);
 extern char 		**sh_envgen(void);
 extern Sfdouble_t	sh_arith(const char*);
 extern void		*sh_arithcomp(char*);
@@ -140,6 +141,8 @@ extern Dt_t		*sh_subfuntree(int);
 extern void		sh_subjobcheck(pid_t);
 extern int		sh_subsavefd(int);
 extern void		sh_subtmpfile(void);
+extern void		sh_pwdupdate(int);
+extern int		sh_validate_subpwdfd(void);
 extern char 		*sh_substitute(const char*,const char*,char*);
 extern const char	*_sh_translate(const char*);
 extern int		sh_trace(char*[],int);

src/cmd/ksh93/include/shell.h

@@ -366,7 +366,8 @@ struct Shell_s
 	int		offsets[10];
 	Sfio_t		**sftable;
 	unsigned char	*fdstatus;
-	const char	*pwd;
+	char		*pwd;
+	int		pwdfd;		/* file descriptor for pwd */
 	void		*jmpbuffer;
 	void		*mktype;
 	Sfio_t		*strbuf;

src/cmd/ksh93/sh/fault.c

@@ -685,6 +685,8 @@ noreturn void sh_done(int sig)
 	if(sh_isoption(SH_NOEXEC))
 		kiaclose((Lex_t*)sh.lex_context);
 #endif /* SHOPT_KIA */
+	if(sh.pwdfd > 0)
+		close(sh.pwdfd);
 	/* Exit with portable 8-bit status (128 + signum) if last child process exits due to signal */
 	if(sh.chldexitsig)
 		savxit = savxit & ~SH_EXITSIG | 0200;

src/cmd/ksh93/sh/init.c

@@ -1902,11 +1902,7 @@ static char *env_init(void)
 	}
 	if(save_env_n)
 		sh.save_env_n = save_env_n;
-	if(nv_isnull(PWDNOD) || nv_isattr(PWDNOD,NV_TAGGED))
-	{
-		nv_offattr(PWDNOD,NV_TAGGED);
-		path_pwd();
-	}
+	path_pwd();
 	if((cp = nv_getval(SHELLNOD)) && (sh_type(cp)&SH_TYPE_RESTRICTED))
 		sh_onoption(SH_RESTRICTED); /* restricted shell */
 	/*

src/cmd/ksh93/sh/name.c

@@ -1511,11 +1511,7 @@ Namval_t *nv_open(const char *name, Dt_t *root, int flags)
 	{
 		cp++;
 		if(sh_isstate(SH_INIT))
-		{
 			nv_putval(np, cp, NV_RDONLY);
-			if(np==PWDNOD)
-				nv_onattr(np,NV_TAGGED);
-		}
 		else
 		{
 			char *sub=0, *prefix= sh.prefix;

src/cmd/ksh93/sh/path.c

@@ -213,8 +213,8 @@ char *path_pwd(void)
 	if(sh.pwd)
 	{
 		if(*sh.pwd=='/')
-			return (char*)sh.pwd;
-		free((void*)sh.pwd);
+			return sh.pwd;
+		free(sh.pwd);
 	}
 	/* First see if PWD variable is correct */
 	pwdnod = sh_scoped(PWDNOD);
@@ -249,7 +249,9 @@ char *path_pwd(void)
 	if(!tofree)
 		cp = sh_strdup(cp);
 	sh.pwd = cp;
-	return (char*)sh.pwd;
+	/* Set sh.pwdfd */
+	sh_pwdupdate(sh_diropenat(AT_FDCWD,cp));
+	return sh.pwd;
 }
 
 /*

src/cmd/ksh93/sh/subshell.c

@@ -39,14 +39,6 @@
 #   define PIPE_BUF	512
 #endif
 
-#ifndef O_SEARCH
-#   ifdef O_PATH
-#	define O_SEARCH	O_PATH
-#   else
-#	define O_SEARCH	O_RDONLY
-#   endif
-#endif
-
 /*
  * Note that the following structure must be the same
  * size as the Dtlink_t structure
@@ -90,11 +82,8 @@ static struct subshell
 	char		comsub;
 	unsigned int	rand_seed;          /* parent shell $RANDOM seed */
 	int		rand_last;          /* last random number from $RANDOM in parent shell */
-	int		rand_state;         /* 0 means sp->rand_seed hasn't been set, 1 is the opposite */
-#if _lib_fchdir
-	int		pwdfd;	/* file descriptor for PWD */
-	char		pwdclose;
-#endif /* _lib_fchdir */
+	char		rand_state;         /* 0 means sp->rand_seed hasn't been set, 1 is the opposite */
+	int		pwdfd;              /* parent shell's file descriptor for PWD */
 } *subshell_data;
 
 static unsigned int subenv;
@@ -456,6 +445,28 @@ void sh_subjobcheck(pid_t pid)
 	}
 }
 
+/*
+ * Set the file descriptor for the current shell's PWD without wiping
+ * out the stored file descriptor for the parent shell's PWD.
+ */
+void sh_pwdupdate(int fd)
+{
+	struct subshell *sp = subshell_data;
+	if(!(sh.subshell && !sh.subshare && sh.pwdfd == sp->pwdfd) && sh.pwdfd > 0)
+		sh_close(sh.pwdfd);
+	sh.pwdfd = fd;
+}
+
+/*
+ * Check if the parent shell has a valid PWD fd to return to.
+ * Only for use by cd inside of virtual subshells.
+ */
+int sh_validate_subpwdfd(void)
+{
+	struct subshell *sp = subshell_data;
+	return sp->pwdfd > 0;
+}
+
 /*
  * Run command tree <t> in a virtual subshell
  * If comsub is not null, then output will be placed in temp file (or buffer)
@@ -504,8 +515,6 @@ Sfio_t *sh_subshell(Shnode_t *t, volatile int flags, int comsub)
 		path_get(e_dot);
 		sh.pathinit = 0;
 	}
-	if(!sh.pwd)
-		path_pwd();
 	sp->bckpid = sh.bckpid;
 	if(comsub)
 		sh_stats(STAT_COMSUB);
@@ -520,38 +529,9 @@ Sfio_t *sh_subshell(Shnode_t *t, volatile int flags, int comsub)
 		sh.comsub = comsub;
 	if(!sh.subshare)
 	{
-		struct subshell *xp;
 		char *save_debugtrap = 0;
-#if _lib_fchdir
-		sp->pwdfd = -1;
-		for(xp=sp->prev; xp; xp=xp->prev)
-		{
-			if(xp->pwdfd>0 && xp->pwd && strcmp(xp->pwd,sh.pwd)==0)
-			{
-				sp->pwdfd = xp->pwdfd;
-				break;
-			}
-		}
-		if(sp->pwdfd<0)
-		{
-			int n = open(e_dot,O_SEARCH);
-			if(n>=0)
-			{
-				sp->pwdfd = n;
-				if(n<10)
-				{
-					sp->pwdfd = sh_fcntl(n,F_DUPFD,10);
-					close(n);
-				}
-				if(sp->pwdfd>0)
-				{
-					fcntl(sp->pwdfd,F_SETFD,FD_CLOEXEC);
-					sp->pwdclose = 1;
-				}
-			}
-		}
-#endif /* _lib_fchdir */
-		sp->pwd = (sh.pwd?sh_strdup(sh.pwd):0);
+		sp->pwd = sh_strdup(sh.pwd);
+		sp->pwdfd = sh.pwdfd;
 		sp->mask = sh.mask;
 		sh_stats(STAT_SUBSHELL);
 		/* save trap table */
@@ -623,21 +603,6 @@ Sfio_t *sh_subshell(Shnode_t *t, volatile int flags, int comsub)
 		if(sh.savesig < 0)
 		{
 			sh.savesig = 0;
-#if _lib_fchdir
-			if(sp->pwdfd < 0 && !sh.subshare)	/* if we couldn't get a file descriptor to our PWD ... */
-				sh_subfork();			/* ...we have to fork, as we cannot fchdir back to it. */
-#else
-			if(!sh.subshare)
-			{
-				if(sp->pwd && access(sp->pwd,X_OK)<0)
-				{
-					free(sp->pwd);
-					sp->pwd = NULL;
-				}
-				if(!sp->pwd)
-					sh_subfork();
-			}
-#endif /* _lib_fchdir */
 			/* Virtual subshells are not safe to suspend (^Z, SIGTSTP) in the interactive main shell. */
 			if(sh_isstate(SH_INTERACTIVE))
 			{
@@ -821,25 +786,29 @@ Sfio_t *sh_subshell(Shnode_t *t, volatile int flags, int comsub)
 			free(savsig);
 		}
 		sh.options = sp->options;
-		/* restore the present working directory */
-#if _lib_fchdir
-		if(sp->pwdfd > 0 && fchdir(sp->pwdfd) < 0)
-#else
-		if(sp->pwd && strcmp(sp->pwd,sh.pwd) && chdir(sp->pwd) < 0)
-#endif /* _lib_fchdir */
+		if(sh.pwdfd != sp->pwdfd)
 		{
-			saveerrno = errno;
-			fatalerror = 2;
+			/*
+			 * Restore the parent shell's present working directory.
+			 * Note: cd will always fork if sp->pwdfd is -1 (after calling sh_validate_subpwdfd()),
+			 * which only occurs when a subshell is started with sh.pwdfd == -1. As such, in this
+			 * if block sp->pwdfd is always > 0 (whilst sh.pwdfd is guaranteed to differ, and
+			 * might not be valid).
+			 */
+			if(fchdir(sp->pwdfd) < 0)
+			{
+				/* Couldn't fchdir back; close the fd and cope with the error */
+				sh_close(sp->pwdfd);
+				saveerrno = errno;
+				fatalerror = 2;
+			}
+			else if(sp->pwd && strcmp(sp->pwd,sh.pwd))
+				path_newdir(sh.pathlist);
+			if(fatalerror != 2)
+				sh_pwdupdate(sp->pwdfd);
 		}
-		else if(sp->pwd && strcmp(sp->pwd,sh.pwd))
-			path_newdir(sh.pathlist);
-		if(sh.pwd)
-			free((void*)sh.pwd);
+		free(sh.pwd);
 		sh.pwd = sp->pwd;
-#if _lib_fchdir
-		if(sp->pwdclose)
-			close(sp->pwdfd);
-#endif /* _lib_fchdir */
 		if(sp->mask!=sh.mask)
 			umask(sh.mask=sp->mask);
 		if(sh.coutpipe!=sp->coutpipe)
@@ -913,8 +882,7 @@ Sfio_t *sh_subshell(Shnode_t *t, volatile int flags, int comsub)
 				UNREACHABLE();
 			case 2:
 				/* reinit PWD as it will be wrong */
-				if(sh.pwd)
-					free((void*)sh.pwd);
+				free(sh.pwd);
 				sh.pwd = NULL;
 				path_pwd();
 				errno = saveerrno;