Re: [PATCH 3/4] fsstress: add EXCHANGE renameat2 support

[Brian Foster <bfoster@redhat.com>]

On Thu, Oct 24, 2019 at 10:20:50PM +0800, kaixuxia wrote:
> Support the EXCHANGE renameat2 syscall in fsstress.
> 
> Signed-off-by: kaixuxia <kaixuxia@tencent.com>
> ---
>  ltp/fsstress.c | 86 +++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 64 insertions(+), 22 deletions(-)
> 
> diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> index 5059639..958adf9 100644
> --- a/ltp/fsstress.c
> +++ b/ltp/fsstress.c
...
> @@ -1118,7 +1124,7 @@ fent_to_name(pathname_t *name, flist_t *flp, fent_t *fep)
>  }
>  
>  void
> -fix_parent(int oldid, int newid)
> +fix_parent(int oldid, int newid, int swap)
>  {
>  	fent_t	*fep;
>  	flist_t	*flp;
> @@ -1129,6 +1135,8 @@ fix_parent(int oldid, int newid)
>  		for (j = 0, fep = flp->fents; j < flp->nfiles; j++, fep++) {
>  			if (fep->parent == oldid)
>  				fep->parent = newid;
> +			if (swap && fep->parent == newid)
> +				fep->parent = oldid;

We might as well use a bool for swap.

>  		}
>  	}
>  }
> @@ -4256,6 +4264,7 @@ out:
>  
>  struct print_flags renameat2_flags [] = {
>  	{ RENAME_NOREPLACE, "NOREPLACE"},
> +	{ RENAME_EXCHANGE, "EXCHANGE"},
>  	{ RENAME_WHITEOUT, "WHITEOUT"},
>  	{ -1, NULL}
>  };
> @@ -4291,41 +4300,68 @@ do_renameat2(int opno, long r, int mode)
>  		return;
>  	}
>  
> -	/* get an existing directory for the destination parent directory name */
> -	if (!get_fname(FT_DIRm, random(), NULL, NULL, &dfep, &v))
> -		parid = -1;
> -	else
> -		parid = dfep->id;
> -	v |= v1;
> +	/* Both pathnames must exist for the RENAME_EXCHANGE */

This comment should also say that the types must be the same.

> +	if (mode == RENAME_EXCHANGE) {
> +		which = 1 << (flp - flist);
> +		init_pathname(&newf);
> +		if (!get_fname(which, random(), &newf, NULL, &dfep, &v)) {
> +			if (v)
> +				printf("%d/%d: rename - no target filename\n",
> +					procid, opno);
> +			free_pathname(&newf);
> +			free_pathname(&f);
> +			return;
> +		}
> +		v |= v1;
> +		id = dfep->id;
> +		parid = dfep->parent;
> +	} else {
> +		/*
> +		 * get an existing directory for the destination parent
> +		 * directory name.
> +		 */
> +		if (!get_fname(FT_DIRm, random(), NULL, NULL, &dfep, &v))
> +			parid = -1;
> +		else
> +			parid = dfep->id;
> +		v |= v1;
>  
> -	/* generate a new path using an existing parent directory in name */
> -	init_pathname(&newf);
> -	e = generate_fname(dfep, flp - flist, &newf, &id, &v1);
> -	v |= v1;
> -	if (!e) {
> -		if (v) {
> -			(void)fent_to_name(&f, &flist[FT_DIR], dfep);
> -			printf("%d/%d: rename - no filename from %s\n",
> -				procid, opno, f.path);
> +		/*
> +		 * generate a new path using an existing parent directory
> +		 * in name.
> +		 */
> +		init_pathname(&newf);
> +		e = generate_fname(dfep, flp - flist, &newf, &id, &v1);
> +		v |= v1;
> +		if (!e) {
> +			if (v) {
> +				(void)fent_to_name(&f, &flist[FT_DIR], dfep);
> +				printf("%d/%d: rename - no filename from %s\n",
> +					procid, opno, f.path);
> +			}
> +			free_pathname(&newf);
> +			free_pathname(&f);
> +			return;
>  		}
> -		free_pathname(&newf);
> -		free_pathname(&f);
> -		return;
>  	}
>  	e = rename_path(&f, &newf, mode) < 0 ? errno : 0;
>  	check_cwd();
>  	if (e == 0) {
>  		int xattr_counter = fep->xattr_counter;
> +		int swap = (mode == RENAME_EXCHANGE) ? 1 : 0;
>  
>  		oldid = fep->id;
>  		oldparid = fep->parent;
>  
>  		if (flp - flist == FT_DIR)
> -			fix_parent(oldid, id);
> +			fix_parent(oldid, id, swap);

What about the other directory if we exchanged two dirs (also see my
comment on the previous version around the safety of doing two separate
swaps)? BTW however this turns out, it would also be useful to see some
comments in this area of code to explain it along with some content in
the commit log descriptions of both patches to document the limitations
of the various renameat2 modes.

Brian

>  
>  		if (mode == RENAME_WHITEOUT)
>  			add_to_flist(flp - flist, id, parid, xattr_counter);
> -		else {
> +		else if (mode == RENAME_EXCHANGE) {
> +			fep->xattr_counter = dfep->xattr_counter;
> +			dfep->xattr_counter = xattr_counter;
> +		} else {
>  			del_from_flist(flp - flist, fep - flp->fents);
>  			add_to_flist(flp - flist, id, parid, xattr_counter);
>  		}
> @@ -4358,6 +4394,12 @@ rnoreplace_f(int opno, long r)
>  }
>  
>  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);
> -- 
> 1.8.3.1
>