Re: [PATCH v2 1/7] fsstress: allow fsync on directories too

[Dave Chinner <david@fromorbit.com>]

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.
-- 
Dave Chinner
david@fromorbit.com