Re: [PATCH v5] fsstress: add renameat2 support

[Brian Foster <bfoster@redhat.com>]

On Tue, Oct 22, 2019 at 08:19:37PM +0800, kaixuxia wrote:
> Support the renameat2 syscall in fsstress.
> 
> Signed-off-by: kaixuxia <kaixuxia@tencent.com>
> ---
> Changes in v5:
>  - Fix the RENAME_EXCHANGE flist fents swap problem.
> 
>  ltp/fsstress.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 169 insertions(+), 33 deletions(-)
> 
> diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> index 51976f5..7c59f2d 100644
> --- a/ltp/fsstress.c
> +++ b/ltp/fsstress.c
...
> @@ -4269,16 +4367,31 @@ rename_f(int opno, long r)
>  			oldid = fep->id;
>  			fix_parent(oldid, id);
>  		}
> -		del_from_flist(flp - flist, fep - flp->fents);
> -		add_to_flist(flp - flist, id, parid, xattr_counter);
> +
> +		if (mode == RENAME_WHITEOUT) {
> +			add_to_flist(FT_DEV, fep->id, fep->parent, 0);
> +			del_from_flist(flp - flist, fep - flp->fents);
> +			add_to_flist(flp - flist, id, parid, xattr_counter);
> +		} else if (mode == RENAME_EXCHANGE) {
> +			if (dflp - flist == FT_DIR) {
> +				oldid = dfep->id;
> +				fix_parent(oldid, fep->id);
> +			}
> +			swap_flist_fents(flp - flist, fep - flp->fents,
> +					 dflp - flist, dfep - dflp->fents);

Hmm.. sorry, but this is still a little confusing. One thing I realized
when running this is that the id correlates with filename and the
filename correlates to type (i.e., fN for files, cN for devs, dN for
dirs, etc.). This means that we can now end up doing something like
this:

0/8: rename(EXCHANGE) c4 to f5 0
0/8: rename source entry: id=5,parent=-1
0/8: rename target entry: id=5,parent=-1

... which leaves an 'f5' device node and 'c4' regular file. Because of
this, I'm wondering if we should just restrict rexchange to files of the
same type and keep this simple. That means we would use the file type of
the source file when looking up a destination to exchange with (instead
of FT_ANY).

With regard to fixing up the flist, this leaves two general cases:

- Between two non-dirs: REXCHANGE f0 <-> d3/f5

The id -> parent relationship actually hasn't changed because both file
entries still exist just as before the call. We've basically just
swapped inodes from the directory tree perspective. This means
xattr_count needs to be swapped between the entries.

- Between two dirs: REXCHANGE d1 <-> d2/d3

I think the same thing applies as above with regard to the parent ids of
the directories themselves. E.g., d3 is still under d2, it just now
needs the xattr_count from the old d1 and vice versa. Additionally, all
of the children of d2/d3 are now under d1 and vice versa, so those
parent ids need to be swapped. That said, we can't just call
fix_parent() to swap all parentid == 1 to 3 and then repeat for 3 -> 1
because that would put everything under 1. Instead, it seems like we
actually need a single fix_parent() sweep to change all 1 -> 3 and 3 ->
1 parent ids in a single pass.

Moving on to RWHITEOUT, the above means that we leave a dev node around
with whatever the name of the source file was. That implies we should
restrict RWHITEOUT to device nodes if we want to maintain
filelist/filename integrity. The immediate question is: would that allow
the associated fstest to still reproduce the deadlock problem? I think
it should, but we should confirm that (i.e., the test now needs to do
'-fmknod=NN' instead of '-fcreat=NN').

Thoughts? Does that all sound reasonable/correct or have I
misinterpreted things?

Finally, given the complexity disparity between the two operations, at
this point I'd suggest to split this into two patches (one for general
renameat2 support + rwhiteout, another for rexchange support on top).

Brian

> +		} else {
> +			del_from_flist(flp - flist, fep - flp->fents);
> +			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 +4400,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