| Message ID | 20190401125018.10009-1-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/7] fsstress: allow fsync on directories too | expand |
On Mon, Apr 01, 2019 at 01:50:18PM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > Currently the fsync function can only be performed against regular files. > Allow it to operate on directories too, to increase test coverage and > allow for chances of finding bugs in a filesystem's implementation of > fsync against directories. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > > V2: Added helper functions to open and close files or directories. not exactly what I meant, more below.... > > ltp/fsstress.c | 42 +++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 39 insertions(+), 3 deletions(-) > > diff --git a/ltp/fsstress.c b/ltp/fsstress.c > index 2223fd7d..1169b840 100644 > --- a/ltp/fsstress.c > +++ b/ltp/fsstress.c > @@ -303,6 +303,7 @@ int attr_remove_path(pathname_t *, const char *, int); > int attr_set_path(pathname_t *, const char *, const char *, const int, int); > void check_cwd(void); > void cleanup_flist(void); > +void close_file_or_dir(int, DIR *); > int creat_path(pathname_t *, mode_t); > void dcache_enter(int, int); > void dcache_init(void); > @@ -324,6 +325,7 @@ void make_freq_table(void); > int mkdir_path(pathname_t *, mode_t); > int mknod_path(pathname_t *, mode_t, dev_t); > void namerandpad(int, char *, int); > +int open_file_or_dir(pathname_t *, int, DIR **); > int open_path(pathname_t *, int); > DIR *opendir_path(pathname_t *); > void process_freq(char *); > @@ -852,6 +854,15 @@ cleanup_flist(void) > } > } > > +void > +close_file_or_dir(int fd, DIR *dir) > +{ > + if (dir) > + closedir(dir); > + else > + close(fd); > +} > + > int > creat_path(pathname_t *name, mode_t mode) > { > @@ -1385,6 +1396,30 @@ namerandpad(int id, char *buf, int i) > } > > int > +open_file_or_dir(pathname_t *name, int flags, DIR **dir) > +{ > + int fd; > + > + fd = open_path(name, flags); > + if (fd < 0 && errno == EISDIR) { > + *dir = opendir_path(name); Why this function, and not just open(O_DIRECTORY)? None of the code that uses this function actually uses the DIR * that is returned, just the fd that is extracted from it. So why no just open() the directory directly and avoid having all the mess with dir streams that aren't needed? > + if (*dir) { > + fd = dirfd(*dir); > + if (fd < 0) { > + int e = errno; > + > + closedir(*dir); > + *dir = NULL; > + errno = e; > + } > + } > + } else { > + *dir = NULL; > + } > + return fd; Excessive nesting. I think this should be as simple as: fd = open_path(name, flags); if (fd < 0 && errno != EISDIR) { return -1; return open_path(name, flags | O_DIRECTORY); And the close_file_or_dir() function is completely unnecessary. > + > +int > open_path(pathname_t *name, int oflag) > { > char buf[NAME_MAX + 1]; > @@ -3440,15 +3475,16 @@ fsync_f(int opno, long r) > pathname_t f; > int fd; > int v; > + DIR *dir; > > init_pathname(&f); > - if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) { > + if (!get_fname(FT_REGFILE | FT_DIRm, r, &f, NULL, NULL, &v)) { > if (v) > printf("%d/%d: fsync - no filename\n", procid, opno); > free_pathname(&f); > return; > } > - fd = open_path(&f, O_WRONLY); > + fd = open_file_or_dir(&f, O_WRONLY, &dir); > e = fd < 0 ? errno : 0; > check_cwd(); > if (fd < 0) { This whole hunk - from init_pathname to check_cwd - was what I was suggesting you factor out, not just the open_path() code. Cheers, Dave.
On Wed, Apr 3, 2019 at 3:18 AM Dave Chinner <david@fromorbit.com> wrote: > > On Mon, Apr 01, 2019 at 01:50:18PM +0100, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > Currently the fsync function can only be performed against regular files. > > Allow it to operate on directories too, to increase test coverage and > > allow for chances of finding bugs in a filesystem's implementation of > > fsync against directories. > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > --- > > > > V2: Added helper functions to open and close files or directories. > > not exactly what I meant, more below.... > > > > ltp/fsstress.c | 42 +++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 39 insertions(+), 3 deletions(-) > > > > diff --git a/ltp/fsstress.c b/ltp/fsstress.c > > index 2223fd7d..1169b840 100644 > > --- a/ltp/fsstress.c > > +++ b/ltp/fsstress.c > > @@ -303,6 +303,7 @@ int attr_remove_path(pathname_t *, const char *, int); > > int attr_set_path(pathname_t *, const char *, const char *, const int, int); > > void check_cwd(void); > > void cleanup_flist(void); > > +void close_file_or_dir(int, DIR *); > > int creat_path(pathname_t *, mode_t); > > void dcache_enter(int, int); > > void dcache_init(void); > > @@ -324,6 +325,7 @@ void make_freq_table(void); > > int mkdir_path(pathname_t *, mode_t); > > int mknod_path(pathname_t *, mode_t, dev_t); > > void namerandpad(int, char *, int); > > +int open_file_or_dir(pathname_t *, int, DIR **); > > int open_path(pathname_t *, int); > > DIR *opendir_path(pathname_t *); > > void process_freq(char *); > > @@ -852,6 +854,15 @@ cleanup_flist(void) > > } > > } > > > > +void > > +close_file_or_dir(int fd, DIR *dir) > > +{ > > + if (dir) > > + closedir(dir); > > + else > > + close(fd); > > +} > > + > > int > > creat_path(pathname_t *name, mode_t mode) > > { > > @@ -1385,6 +1396,30 @@ namerandpad(int id, char *buf, int i) > > } > > > > int > > +open_file_or_dir(pathname_t *name, int flags, DIR **dir) > > +{ > > + int fd; > > + > > + fd = open_path(name, flags); > > + if (fd < 0 && errno == EISDIR) { > > + *dir = opendir_path(name); > > Why this function, and not just open(O_DIRECTORY)? The reason I did it is because if flags are O_WRONLY (or O_RDWR), opening a directory always fails (even with O_DIRECTORY) with errno EISDIR. If the access flags are O_RDONLY, opening a directory succeeds regardless of O_DIRECTORY being passed to open(2). Since I supposed fsetxattr(2) and fsync(2) could be considered write operations, I went for the use of opendir_path(), using the dir stream and all that hassle. But now I just tested if fsync and fsetxattr work if the file is open O_RDONLY, and to my surprise they do. Test program: https://friendpaste.com/7gUN4kB3ZfHwVdlDggodUY Output with O_WRONLY | O_DIRECTORY passed to open(2): $ mkfs.xfs -f /dev/sdc $ mount /dev/sdc /mnt $ touch /mnt/file $ mkdir /mnt/dir $ ./test_open /mnt/file open error 20: Not a directory $ ./test_open /mnt/dir open error 21: Is a directory Output with only O_RDONLY passed to open(2): $ mkfs.xfs -f /dev/sdc $ mount /dev/sdc /mnt $ touch /mnt/file $ mkdir /mnt/dir $ ./test_open /mnt/file file open fd = 3, setxattr(fd) = 0, fsync(fd) = 0 $ ./test_open /mnt/dir file open fd = 3, setxattr(fd) = 0, fsync(fd) = 0 Perhaps I did something wrong, as it's late here right now. So should we go for ensuring O_RDONLY is used when the first open fails with EISDIR (and add some comment explaining this in case I'm not the only for which this was a surprise) Thanks. > None of the code that uses this function actually uses the DIR * > that is returned, just the fd that is extracted from it. So why no > just open() the directory directly and avoid having all the mess > with dir streams that aren't needed? > > > > + if (*dir) { > > + fd = dirfd(*dir); > > + if (fd < 0) { > > + int e = errno; > > + > > + closedir(*dir); > > + *dir = NULL; > > + errno = e; > > + } > > + } > > + } else { > > + *dir = NULL; > > + } > > + return fd; > > Excessive nesting. I think this should be as simple as: > > fd = open_path(name, flags); > if (fd < 0 && errno != EISDIR) { > return -1; > return open_path(name, flags | O_DIRECTORY); > > And the close_file_or_dir() function is completely unnecessary. > > > > + > > +int > > open_path(pathname_t *name, int oflag) > > { > > char buf[NAME_MAX + 1]; > > @@ -3440,15 +3475,16 @@ fsync_f(int opno, long r) > > pathname_t f; > > int fd; > > int v; > > + DIR *dir; > > > > init_pathname(&f); > > - if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) { > > + if (!get_fname(FT_REGFILE | FT_DIRm, r, &f, NULL, NULL, &v)) { > > if (v) > > printf("%d/%d: fsync - no filename\n", procid, opno); > > free_pathname(&f); > > return; > > } > > - fd = open_path(&f, O_WRONLY); > > + fd = open_file_or_dir(&f, O_WRONLY, &dir); > > e = fd < 0 ? errno : 0; > > check_cwd(); > > if (fd < 0) { > > This whole hunk - from init_pathname to check_cwd - was what I was > suggesting you factor out, not just the open_path() code. > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Wed, Apr 03, 2019 at 05:35:20PM +0000, Filipe Manana wrote: > On Wed, Apr 3, 2019 at 3:18 AM Dave Chinner <david@fromorbit.com> wrote: > > > > On Mon, Apr 01, 2019 at 01:50:18PM +0100, fdmanana@kernel.org wrote: > > > From: Filipe Manana <fdmanana@suse.com> > > > > > > Currently the fsync function can only be performed against regular files. > > > Allow it to operate on directories too, to increase test coverage and > > > allow for chances of finding bugs in a filesystem's implementation of > > > fsync against directories. > > > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > > --- > > > > > > V2: Added helper functions to open and close files or directories. > > > > not exactly what I meant, more below.... > > > > > > ltp/fsstress.c | 42 +++++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 39 insertions(+), 3 deletions(-) > > > > > > diff --git a/ltp/fsstress.c b/ltp/fsstress.c > > > index 2223fd7d..1169b840 100644 > > > --- a/ltp/fsstress.c > > > +++ b/ltp/fsstress.c > > > @@ -303,6 +303,7 @@ int attr_remove_path(pathname_t *, const char *, int); > > > int attr_set_path(pathname_t *, const char *, const char *, const int, int); > > > void check_cwd(void); > > > void cleanup_flist(void); > > > +void close_file_or_dir(int, DIR *); > > > int creat_path(pathname_t *, mode_t); > > > void dcache_enter(int, int); > > > void dcache_init(void); > > > @@ -324,6 +325,7 @@ void make_freq_table(void); > > > int mkdir_path(pathname_t *, mode_t); > > > int mknod_path(pathname_t *, mode_t, dev_t); > > > void namerandpad(int, char *, int); > > > +int open_file_or_dir(pathname_t *, int, DIR **); > > > int open_path(pathname_t *, int); > > > DIR *opendir_path(pathname_t *); > > > void process_freq(char *); > > > @@ -852,6 +854,15 @@ cleanup_flist(void) > > > } > > > } > > > > > > +void > > > +close_file_or_dir(int fd, DIR *dir) > > > +{ > > > + if (dir) > > > + closedir(dir); > > > + else > > > + close(fd); > > > +} > > > + > > > int > > > creat_path(pathname_t *name, mode_t mode) > > > { > > > @@ -1385,6 +1396,30 @@ namerandpad(int id, char *buf, int i) > > > } > > > > > > int > > > +open_file_or_dir(pathname_t *name, int flags, DIR **dir) > > > +{ > > > + int fd; > > > + > > > + fd = open_path(name, flags); > > > + if (fd < 0 && errno == EISDIR) { > > > + *dir = opendir_path(name); > > > > Why this function, and not just open(O_DIRECTORY)? > > The reason I did it is because if flags are O_WRONLY (or O_RDWR), > opening a directory always fails (even with O_DIRECTORY) with errno > EISDIR. > If the access flags are O_RDONLY, opening a directory succeeds > regardless of O_DIRECTORY being passed to open(2). > > Since I supposed fsetxattr(2) and fsync(2) could be considered write > operations, I went for the use of opendir_path(), using the dir stream > and all that hassle. > > But now I just tested if fsync and fsetxattr work if the file is open > O_RDONLY, and to my surprise they do. > > Test program: > > https://friendpaste.com/7gUN4kB3ZfHwVdlDggodUY > > Output with O_WRONLY | O_DIRECTORY passed to open(2): > > $ mkfs.xfs -f /dev/sdc > $ mount /dev/sdc /mnt > > $ touch /mnt/file > $ mkdir /mnt/dir > > $ ./test_open /mnt/file > open error 20: Not a directory > $ ./test_open /mnt/dir > open error 21: Is a directory > > Output with only O_RDONLY passed to open(2): > > $ mkfs.xfs -f /dev/sdc > $ mount /dev/sdc /mnt > > $ touch /mnt/file > $ mkdir /mnt/dir > > $ ./test_open /mnt/file > file open fd = 3, setxattr(fd) = 0, fsync(fd) = 0 > $ ./test_open /mnt/dir > file open fd = 3, setxattr(fd) = 0, fsync(fd) = 0 > > Perhaps I did something wrong, as it's late here right now. Pretty sure this is because setxattr() checks inode permissions, not file descriptor read/write modes to determine whether an xattr can be set/modified/removed. i.e. you own the directory, have write permissions to the directory, and so you can minipulate xattrs on it. FWIW, why even bother with opening fd's at all here? you could just use set/get/removexattr() directly on the path name and not have to worry about opening the file... > So should we go for ensuring O_RDONLY is used when the first open > fails with EISDIR (and add some comment explaining this in case I'm > not the only for which this was a surprise) If we actually need to open the fd, then yes - using opendir()/dirfd() is essentially doing that for you behind the scenes. Cheers, Dave.
diff --git a/ltp/fsstress.c b/ltp/fsstress.c index 2223fd7d..1169b840 100644 --- a/ltp/fsstress.c +++ b/ltp/fsstress.c @@ -303,6 +303,7 @@ int attr_remove_path(pathname_t *, const char *, int); int attr_set_path(pathname_t *, const char *, const char *, const int, int); void check_cwd(void); void cleanup_flist(void); +void close_file_or_dir(int, DIR *); int creat_path(pathname_t *, mode_t); void dcache_enter(int, int); void dcache_init(void); @@ -324,6 +325,7 @@ void make_freq_table(void); int mkdir_path(pathname_t *, mode_t); int mknod_path(pathname_t *, mode_t, dev_t); void namerandpad(int, char *, int); +int open_file_or_dir(pathname_t *, int, DIR **); int open_path(pathname_t *, int); DIR *opendir_path(pathname_t *); void process_freq(char *); @@ -852,6 +854,15 @@ cleanup_flist(void) } } +void +close_file_or_dir(int fd, DIR *dir) +{ + if (dir) + closedir(dir); + else + close(fd); +} + int creat_path(pathname_t *name, mode_t mode) { @@ -1385,6 +1396,30 @@ namerandpad(int id, char *buf, int i) } int +open_file_or_dir(pathname_t *name, int flags, DIR **dir) +{ + int fd; + + fd = open_path(name, flags); + if (fd < 0 && errno == EISDIR) { + *dir = opendir_path(name); + if (*dir) { + fd = dirfd(*dir); + if (fd < 0) { + int e = errno; + + closedir(*dir); + *dir = NULL; + errno = e; + } + } + } else { + *dir = NULL; + } + return fd; +} + +int open_path(pathname_t *name, int oflag) { char buf[NAME_MAX + 1]; @@ -3440,15 +3475,16 @@ fsync_f(int opno, long r) pathname_t f; int fd; int v; + DIR *dir; init_pathname(&f); - if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) { + if (!get_fname(FT_REGFILE | FT_DIRm, r, &f, NULL, NULL, &v)) { if (v) printf("%d/%d: fsync - no filename\n", procid, opno); free_pathname(&f); return; } - fd = open_path(&f, O_WRONLY); + fd = open_file_or_dir(&f, O_WRONLY, &dir); e = fd < 0 ? errno : 0; check_cwd(); if (fd < 0) { @@ -3462,7 +3498,7 @@ fsync_f(int opno, long r) if (v) printf("%d/%d: fsync %s %d\n", procid, opno, f.path, e); free_pathname(&f); - close(fd); + close_file_or_dir(fd, dir); } void