Skip to content

Commit f931608

Browse files
JohnoKingMcDutchie
authored andcommitted
sh_diropenat(): General improvements (#859)
- Avoid duplicating the file descriptor from openat when it's already >9 to increase performance and reliability. - Add a feature test which tests the errno resulting from using openat on a non-directory with O_DIRECTORY|O_SEARCH. On illumos (and presumably Solaris) openat can misleadingly fail with EACCES rather than ENOTDIR, so account for that by testing the given directory in question with stat(2). - Add a feature test to see if file descriptors obtained via O_SEARCH or O_PATH can actually be used with fchdir(2). This is also relevant for open(2) because of O_SEARCH's usage in the fallback code in sh_subshell(). - Ensure the close-on-exec bit is set for the file descriptor returned by sh_diropenat(). These changes allow for the elimination of the double openat and helps mitigate the performance loss for cd's performance as observed after the merge of 3d7b7e1: vide #816 (comment) Results obtained on Linux amd64 using CCFLAGS=-Os. Results for: 'typeset -i i; time for((i=0;i<500000;i++));do cd /dev; cd /;done' ksh-no-openat-9b0b2603: real 0m01.72s ksh-dev-de530132: real 0m03.55s ksh-this-commit: real 0m02.91s ksh-this-commit-no-subshell-inode-test: (same due to no subshells) real 0m02.90s ksh93u+: real 0m02.02s I'll admit cd still isn't as fast as before, yet the trade off is still worthwhile because ksh93 scripts are bound to use subshells (particulary command substitutions) far more often than cd. In the contrived benchmark cd is called 1,000,000 times, which hardly reflects real world scripts. This also doesn't account for the practice of calling cd from within a subshell, which is common. Results for: 'typeset -i i; time for((i=0;i<500000;i++));do (cd /dev); (cd /);done' ksh-no-openat-9b0b2603: real 0m06.73s ksh-dev-de530132: real 0m07.18s ksh-this-commit: real 0m06.34s ksh-this-commit-no-subshell-inode-test: real 0m04.77s ksh93u+: real 0m05.19s Here a small uplift can be seen relative to both of the previous 93u+m iterations, but because of the inode sanity check present since 5ee290c cd is still a bit slower in subshells relative to ksh93u+. (Perhaps in a future commit a faster alternative could be looked into.) Meanwhile the relevant shbench results are about the same (-l 10): ----------------------------------------------------------------------------------------------------------------------------------- name ksh-no-openat-9b0b2603 ksh-dev-de530132 ksh-this-commit ksh-this-commit-no-inode-test ksh93u+ ----------------------------------------------------------------------------------------------------------------------------------- fibonacci.ksh 0.247 [0.240-0.258] 0.227 [0.225-0.233] 0.235 [0.227-0.241] 0.235 [0.232-0.240] 0.258 [0.251-0.264] parens.ksh 0.191 [0.185-0.197] 0.097 [0.095-0.098] 0.097 [0.095-0.100] 0.099 [0.095-0.101] 0.227 [0.221-0.234] -----------------------------------------------------------------------------------------------------------------------------------
1 parent 6b8df74 commit f931608

File tree

4 files changed

+102
-11
lines changed

4 files changed

+102
-11
lines changed

src/cmd/ksh93/Mamfile

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ make install virtual
8383
exec - %{iffe_run} %{<}
8484
done
8585

86-
loop F options externs locale cmds poll rlimits
86+
loop F options externs fchdir locale cmds poll rlimits
8787
make FEATURE/%{F}
8888
makp features/%{F}
8989
exec - %{iffe_run} %{<}
@@ -268,6 +268,7 @@ make install virtual
268268
make include/io.h
269269
prev %{INCLUDE_AST}/sfio.h
270270
prev %{INCLUDE_AST}/ast.h
271+
prev FEATURE/fchdir
271272
done
272273

273274
make include/jobs.h
@@ -343,6 +344,7 @@ make install virtual
343344
prev include/test.h
344345
prev %{INCLUDE_AST}/ls.h
345346
prev include/builtins.h
347+
prev include/io.h
346348
prev include/name.h
347349
prev include/path.h
348350
prev include/variables.h

src/cmd/ksh93/bltins/cd_pwd.c

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
#include "path.h"
3434
#include "name.h"
3535
#include "builtins.h"
36+
#include "io.h"
3637
#include <ls.h>
3738
#include "test.h"
3839

@@ -48,20 +49,40 @@ static void rehash(Namval_t *np,void *data)
4849

4950
#if _lib_openat
5051
/*
51-
* Obtain a file handle to the directory "path" relative to directory "dir"
52+
* Obtain a file descriptor >9 to the directory "path" relative to directory "dir"
5253
*/
5354
int sh_diropenat(int dir, const char *path)
5455
{
55-
int fd,shfd;
56-
if((fd = openat(dir, path, O_DIRECTORY|O_NONBLOCK|O_cloexec)) < 0)
57-
#if O_SEARCH
58-
if(errno != EACCES || (fd = openat(dir, path, O_SEARCH|O_DIRECTORY|O_NONBLOCK|O_cloexec)) < 0)
56+
int fd, needs_cloexec = 0;
57+
if((fd = openat(dir, path, O_DIRECTORY|O_NONBLOCK|O_cloexec|O_SEARCH)) < 0)
58+
{
59+
#if !_openat_enotdir
60+
struct stat fs;
61+
int saverrno = errno;
62+
/* Some OSes (e.g. illumos) misleadingly fail with EACCES rather than ENOTDIR */
63+
if(errno==EACCES && !stat(path, &fs) && !S_ISDIR(fs.st_mode))
64+
errno = ENOTDIR;
65+
else
66+
errno = saverrno;
5967
#endif
60-
return fd;
61-
/* Move fd to a number > 10 and register the fd number with the shell */
62-
shfd = sh_fcntl(fd, F_dupfd_cloexec, 10);
63-
close(fd);
64-
return shfd;
68+
return fd;
69+
}
70+
if(fd < 10)
71+
{
72+
/* Duplicate the fd and register it with the shell */
73+
int shfd = sh_fcntl(fd, F_dupfd_cloexec, 10);
74+
close(fd);
75+
if(shfd < 0)
76+
return shfd;
77+
if(F_dupfd_cloexec == F_DUPFD)
78+
needs_cloexec = 1;
79+
fd = shfd;
80+
}
81+
else if(O_cloexec == 0)
82+
needs_cloexec = 1;
83+
if(needs_cloexec)
84+
sh_fcntl(fd,F_SETFD,FD_CLOEXEC);
85+
return fd;
6586
}
6687
#endif /* _lib_openat */
6788

src/cmd/ksh93/features/fchdir

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
tst fchdir_osearch_compat note{ can fchdir(2) use file descriptors obtained using O_PATH or O_SEARCH }end output{
2+
#include <ast.h>
3+
#include <wait.h>
4+
#include <stdio.h>
5+
#if !O_SEARCH
6+
#error No O_SEARCH or O_PATH detected; fchdir is unreliable
7+
#endif
8+
int main(void)
9+
{
10+
char dirname[64];
11+
int n;
12+
pid_t child;
13+
sprintf(dirname,".dir2.%u",(unsigned int)getpid());
14+
mkdir(dirname,0777);
15+
child = fork();
16+
if(child==0)
17+
{
18+
#if _lib_openat
19+
if((n = openat(AT_FDCWD, dirname, O_DIRECTORY|O_NONBLOCK|O_cloexec|O_SEARCH)) < 0)
20+
#else
21+
if((n = open(dirname, O_DIRECTORY|O_cloexec|O_SEARCH)) < 0) /* vulnerable to race, but that's not a concern */
22+
#endif
23+
return 1; /* couldn't even obtain a file descriptor */
24+
if(fchdir(n) < 0)
25+
return 1; /* couldn't change the directory (perhaps Linux <3.2.23) */
26+
return 0;
27+
}
28+
else if(child==-1)
29+
{
30+
rmdir(dirname);
31+
return 1;
32+
}
33+
/* a parent process removes the empty dir because that's safer than 'cd ..' */
34+
waitpid(child, &n, 0);
35+
rmdir(dirname);
36+
return WEXITSTATUS(n);
37+
}
38+
}end
39+
40+
tst openat_enotdir note{ does openat set ENOTDIR when it fails because the file isn't a directory }end output{
41+
#include <ast.h>
42+
#include <stdio.h>
43+
#include <errno.h>
44+
#if !_lib_openat
45+
#error openat(2) doesn't exist, so this test is pointless
46+
#endif
47+
int main(void)
48+
{
49+
char filename[64];
50+
int n, saverrno;
51+
pid_t child;
52+
sprintf(filename,".file.%u",(unsigned int)getpid());
53+
open(filename,O_RDWR|O_CREAT); /* let the OS close the fd after completion */
54+
openat(AT_FDCWD, filename, O_DIRECTORY|O_NONBLOCK|O_cloexec|O_SEARCH);
55+
saverrno = errno;
56+
unlink(filename);
57+
if(saverrno != ENOTDIR)
58+
return 1;
59+
return 0;
60+
}
61+
}end

src/cmd/ksh93/include/io.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626

2727
#include <ast.h>
2828
#include <sfio.h>
29+
#include "FEATURE/fchdir"
2930

3031
#ifndef IOBSIZE
3132
# define IOBSIZE (SFIO_BUFSIZE*sizeof(char*))
@@ -52,6 +53,12 @@
5253
struct ionod;
5354
#endif /* !ARG_RAW */
5455

56+
/* if O_SEARCH/O_PATH is unreliable for fchdir, it's not worth using */
57+
#if !_fchdir_osearch_compat
58+
#undef O_SEARCH
59+
#define O_SEARCH 0
60+
#endif
61+
5562
/*
5663
* Check if there is an editor active while avoiding repetitive #if flaggery.
5764
* The 0 definition is used to optimize out code if no editor is compiled in.

0 commit comments

Comments
 (0)