Re: [PATCH] fsstress: add renameat2 support

[Brian Foster <bfoster@redhat.com>]

On Fri, Oct 11, 2019 at 03:56:01PM +0800, kaixuxia wrote:
> Support the renameat2 syscall in fsstress.
> 
> Signed-off-by: kaixuxia <kaixuxia@tencent.com>
> ---
>  ltp/fsstress.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 79 insertions(+), 11 deletions(-)
> 
> diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> index 51976f5..21529a2 100644
> --- a/ltp/fsstress.c
> +++ b/ltp/fsstress.c
> @@ -44,6 +44,16 @@ io_context_t	io_ctx;
>  #define IOV_MAX 1024
>  #endif
>  
> +#ifndef RENAME_NOREPLACE
> +#define RENAME_NOREPLACE	(1 << 0)	/* Don't overwrite target */
> +#endif
> +#ifndef RENAME_EXCHANGE
> +#define RENAME_EXCHANGE		(1 << 1)	/* Exchange source and dest */
> +#endif
> +#ifndef RENAME_WHITEOUT
> +#define RENAME_WHITEOUT		(1 << 2)	/* Whiteout source */
> +#endif
> +
>  #define FILELEN_MAX		(32*4096)
>  
>  typedef enum {
> @@ -85,6 +95,9 @@ typedef enum {
>  	OP_READV,
>  	OP_REMOVEFATTR,
>  	OP_RENAME,
> +	OP_RNOREPLACE,
> +	OP_REXCHANGE,
> +	OP_RWHITEOUT,
>  	OP_RESVSP,
>  	OP_RMDIR,
>  	OP_SETATTR,
> @@ -203,6 +216,9 @@ void	readlink_f(int, long);
>  void	readv_f(int, long);
>  void	removefattr_f(int, long);
>  void	rename_f(int, long);
> +void    rnoreplace_f(int, long);
> +void    rexchange_f(int, long);
> +void    rwhiteout_f(int, long);
>  void	resvsp_f(int, long);
>  void	rmdir_f(int, long);
>  void	setattr_f(int, long);
> @@ -262,6 +278,9 @@ opdesc_t	ops[] = {
>  	/* remove (delete) extended attribute */
>  	{ OP_REMOVEFATTR, "removefattr", removefattr_f, 1, 1 },
>  	{ OP_RENAME, "rename", rename_f, 2, 1 },
> +	{ OP_RNOREPLACE, "rnoreplace", rnoreplace_f, 2, 1 },
> +	{ OP_REXCHANGE, "rexchange", rexchange_f, 2, 1 },
> +	{ OP_RWHITEOUT, "rwhiteout", rwhiteout_f, 2, 1 },
>  	{ OP_RESVSP, "resvsp", resvsp_f, 1, 1 },
>  	{ OP_RMDIR, "rmdir", rmdir_f, 1, 1 },
>  	/* set attribute flag (FS_IOC_SETFLAGS ioctl) */
> @@ -354,7 +373,7 @@ int	open_path(pathname_t *, int);
>  DIR	*opendir_path(pathname_t *);
>  void	process_freq(char *);
>  int	readlink_path(pathname_t *, char *, size_t);
> -int	rename_path(pathname_t *, pathname_t *);
> +int	rename_path(pathname_t *, pathname_t *, int);
>  int	rmdir_path(pathname_t *);
>  void	separate_pathname(pathname_t *, char *, pathname_t *);
>  void	show_ops(int, char *);
> @@ -1519,7 +1538,7 @@ readlink_path(pathname_t *name, char *lbuf, size_t lbufsiz)
>  }
>  
>  int
> -rename_path(pathname_t *name1, pathname_t *name2)
> +rename_path(pathname_t *name1, pathname_t *name2, int mode)
>  {
>  	char		buf1[NAME_MAX + 1];
>  	char		buf2[NAME_MAX + 1];
> @@ -1528,14 +1547,14 @@ rename_path(pathname_t *name1, pathname_t *name2)
>  	pathname_t	newname2;
>  	int		rval;
>  
> -	rval = rename(name1->path, name2->path);
> +	rval = syscall(__NR_renameat2, AT_FDCWD, name1->path, AT_FDCWD, name2->path, mode);

Perhaps we should preserve the rename() call with an ifdef or something
(similar to src/renameat2.c) in the event that renameat2 is not
available. In fact, I suppose we'd want to compile out the new renameat2
based ops as well so the user doesn't have to disable them where they
aren't supported.

>  	if (rval >= 0 || errno != ENAMETOOLONG)
>  		return rval;
>  	separate_pathname(name1, buf1, &newname1);
>  	separate_pathname(name2, buf2, &newname2);
>  	if (strcmp(buf1, buf2) == 0) {
>  		if (chdir(buf1) == 0) {
> -			rval = rename_path(&newname1, &newname2);
> +			rval = rename_path(&newname1, &newname2, mode);
>  			assert(chdir("..") == 0);
>  		}
>  	} else {
> @@ -1555,7 +1574,7 @@ rename_path(pathname_t *name1, pathname_t *name2)
>  			append_pathname(&newname2, "../");
>  			append_pathname(&newname2, name2->path);
>  			if (chdir(buf1) == 0) {
> -				rval = rename_path(&newname1, &newname2);
> +				rval = rename_path(&newname1, &newname2, mode);
>  				assert(chdir("..") == 0);
>  			}
>  		} else {
> @@ -1563,7 +1582,7 @@ rename_path(pathname_t *name1, pathname_t *name2)
>  			append_pathname(&newname1, "../");
>  			append_pathname(&newname1, name1->path);
>  			if (chdir(buf2) == 0) {
> -				rval = rename_path(&newname1, &newname2);
> +				rval = rename_path(&newname1, &newname2, mode);
>  				assert(chdir("..") == 0);
>  			}
>  		}
> @@ -4215,8 +4234,18 @@ out:
>  	free_pathname(&f);
>  }
>  
> +struct print_flags renameat2_flags [] = {
> +	{ RENAME_NOREPLACE, "NOREPLACE"},
> +	{ RENAME_EXCHANGE, "EXCHANGE"},
> +	{ RENAME_WHITEOUT, "WHITEOUT"},
> +	{ -1, NULL}
> +};
> +
> +#define translate_renameat2_flags(mode)	\
> +	({translate_flags(mode, "|", renameat2_flags);})
> +
>  void
> -rename_f(int opno, long r)
> +do_renameat2(int opno, long r, int mode)
>  {
>  	fent_t		*dfep;
>  	int		e;
> @@ -4229,6 +4258,7 @@ rename_f(int opno, long r)
>  	int		parid;
>  	int		v;
>  	int		v1;
> +	int		fd;
>  
>  	/* get an existing path for the source of the rename */
>  	init_pathname(&f);
> @@ -4260,7 +4290,21 @@ rename_f(int opno, long r)
>  		free_pathname(&f);
>  		return;
>  	}
> -	e = rename_path(&f, &newf) < 0 ? errno : 0;
> +	/* Both pathnames must exist for the RENAME_EXCHANGE */
> +	if (mode == RENAME_EXCHANGE) {
> +		fd = creat_path(&newf, 0666);
> +		e = fd < 0 ? errno : 0;
> +		check_cwd();
> +		if (fd < 0) {
> +			if (v)
> +				printf("%d/%d: renameat2 - creat %s failed %d\n",
> +					procid, opno, newf.path, e);
> +			free_pathname(&newf);
> +			free_pathname(&f);
> +			return;
> +		}
> +	}
> +	e = rename_path(&f, &newf, mode) < 0 ? errno : 0;

It looks like the existing code looks up a source entry and a target
directory and generates a new name to serve as the rename target entry.
Here we create the entry so we can perform an exchange. Can we check
mode earlier and instead look up another existing entry if this is an
exchange, else run the existing code to generate a name?

Other than those couple things this looks pretty good to me.

Brian


>  	check_cwd();
>  	if (e == 0) {
>  		int xattr_counter = fep->xattr_counter;
> @@ -4273,12 +4317,13 @@ rename_f(int opno, long r)
>  		add_to_flist(flp - flist, id, parid, xattr_counter);
>  	}
>  	if (v) {
> -		printf("%d/%d: rename %s to %s %d\n", procid, opno, f.path,
> +		printf("%d/%d: rename(%s) %s to %s %d\n", procid,
> +			opno, translate_renameat2_flags(mode), f.path,
>  			newf.path, e);
>  		if (e == 0) {
> -			printf("%d/%d: rename del entry: id=%d,parent=%d\n",
> +			printf("%d/%d: rename source entry: id=%d,parent=%d\n",
>  				procid, opno, fep->id, fep->parent);
> -			printf("%d/%d: rename add entry: id=%d,parent=%d\n",
> +			printf("%d/%d: rename target entry: id=%d,parent=%d\n",
>  				procid, opno, id, parid);
>  		}
>  	}
> @@ -4287,6 +4332,29 @@ rename_f(int opno, long r)
>  }
>  
>  void
> +rename_f(int opno, long r)
> +{
> +	do_renameat2(opno, r, 0);
> +}
> +void
> +rnoreplace_f(int opno, long r)
> +{
> +	do_renameat2(opno, r, RENAME_NOREPLACE);
> +}
> +
> +void
> +rexchange_f(int opno, long r)
> +{
> +	do_renameat2(opno, r, RENAME_EXCHANGE);
> +}
> +
> +void
> +rwhiteout_f(int opno, long r)
> +{
> +	do_renameat2(opno, r, RENAME_WHITEOUT);
> +}
> +
> +void
>  resvsp_f(int opno, long r)
>  {
>  	int		e;
> -- 
> 1.8.3.1
> 
> -- 
> kaixuxia