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

[kaixuxia <xiakaixu1987@gmail.com>]

On 2019/10/30 20:40, Brian Foster wrote:
> On Wed, Oct 30, 2019 at 11:17:04AM +0800, kaixuxia wrote:
>> On 2019/10/29 21:40, Brian Foster wrote:
>>> On Sat, Oct 26, 2019 at 07:18:37PM +0800, kaixuxia wrote:
>>>> Support the EXCHANGE renameat2 syscall in fsstress.
>>>>
>>>> In order to maintain filelist/filename integrity, we restrict
>>>> rexchange to files of the same type.
>>>>
>>>> Signed-off-by: kaixuxia <kaixuxia@tencent.com>
>>>> ---
>>>
>>> While this looks pretty good to me at this point, I do notice instances
>>> of the following in a quick test:
>>>
>>> 0/29: rename(EXCHANGE) d3/d9/dc/dd to d3/d9/dc/dd/df 22
>>> ...
>>> 0/43: rename(EXCHANGE) d3 to d3/d9/dc/d18 22
>>> ...
>>>
>>> It looks like we're getting an EINVAL error on rexchange of directories.
>>> That same operation seems to work fine via the ./src/renameat2 tool. Any
>>> idea what's going on there?
>>
>> Hmm.. I am not sure if I understand what your mean. Seems like
>> this is because the special source and target parameters setting.
>> There are parameters check for RENAME_EXCHANGE in renameat2() call,
>>
>>  static int do_renameat2(int olddfd, const char __user *oldname, int newdfd,
>>                          const char __user *newname, unsigned int flags)
>>  {
>>   ...
>>          /* source should not be ancestor of target */
>>          error = -EINVAL;
>>          if (old_dentry == trap)
>>                  goto exit5;
>>          /* target should not be an ancestor of source */
>>          if (!(flags & RENAME_EXCHANGE))
>>                  error = -ENOTEMPTY;
>>          if (new_dentry == trap)
>>                  goto exit5;
>>  ...
>>  } 
>>
>> so we get the EINVAL error on rexchange of directories. I also tested it
>> via the ./src/renameat2 tool, and the strace result as below,
>>
> 
> Ah, I see. I wasn't aware of the restriction and didn't catch that quirk
> of these particular requests, so I thought it was failing arbitrary
> directory swaps (which is what I tested with renameat2). This makes
> sense, thanks for the explanation.
> 
>>  # src/renameat2 -x /xfs-bufdeadlock/d3 /xfs-bufdeadlock/d3/d9/dc/d18
>>   Invalid argument
>>
>>  syscall_316(0xffffff9c, 0x7ffe38930813, 0xffffff9c, 0x7ffe38930827, 0x2, 0) = -1 (errno 22)
>>  
>> Exchange looks a bit more tricky here.. Maybe we have two choices,
>> one is just leave the EINVAL there since the fsstress is stress
>> test and the EINVAL possibility is low. The other one is we should
>> do parameters check before invoking the renameat2() call, if the
>> source and target file fents are not suitable we will try more
>> until get the right file fents...
>>
> 
> Hmm.. I think it might be fine to ignore from a functional standpoint if
> the complexity is too involved to detect and skip. That said, I'm
> wondering if the filelist helps us enough here to implement similar
> checks as in the kernel VFS. On a quick look, it appears we walk up the
> dentry chain looking to see if one dentry is a parent of the other. See
> d_ancestor() (called via do_renameat2() -> lock_rename()) for example:
> 
> /*
>  * ...
>  * Returns the ancestor dentry of p2 which is a child of p1, if p1 is
>  * an ancestor of p2, else NULL.
>  */
> struct dentry *d_ancestor(struct dentry *p1, struct dentry *p2)
> {
>         struct dentry *p;
> 
>         for (p = p2; !IS_ROOT(p); p = p->d_parent) {
>                 if (p->d_parent == p1)
>                         return p;
>         }
>         return NULL;
> }
> 
> Any reason we couldn't do a couple similar checks on rexchange of two
> dirs and skip the rename if necessary?
> 
Yeah, sounds more reasonable, will add the couple checks
in next version.

Kaixu

> Brian
> 
>>>
>>> Brian
>>>
>>>>  ltp/fsstress.c | 92 ++++++++++++++++++++++++++++++++++++++++++++--------------
>>>>  1 file changed, 71 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/ltp/fsstress.c b/ltp/fsstress.c
>>>> index ecc1adc..83d6337 100644
>>>> --- a/ltp/fsstress.c
>>>> +++ b/ltp/fsstress.c
>>>> @@ -69,6 +69,9 @@ static int renameat2(int dfd1, const char *path1,
>>>>  #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
>>>> @@ -115,6 +118,7 @@ typedef enum {
>>>>  	OP_REMOVEFATTR,
>>>>  	OP_RENAME,
>>>>  	OP_RNOREPLACE,
>>>> +	OP_REXCHANGE,
>>>>  	OP_RWHITEOUT,
>>>>  	OP_RESVSP,
>>>>  	OP_RMDIR,
>>>> @@ -235,6 +239,7 @@ 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);
>>>> @@ -296,6 +301,7 @@ opdesc_t	ops[] = {
>>>>  	{ 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 },
>>>> @@ -371,7 +377,7 @@ void	del_from_flist(int, int);
>>>>  int	dirid_to_name(char *, int);
>>>>  void	doproc(void);
>>>>  int	fent_to_name(pathname_t *, flist_t *, fent_t *);
>>>> -void	fix_parent(int, int);
>>>> +void	fix_parent(int, int, bool);
>>>>  void	free_pathname(pathname_t *);
>>>>  int	generate_fname(fent_t *, int, pathname_t *, int *, int *);
>>>>  int	generate_xattr_name(int, char *, int);
>>>> @@ -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, bool 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;
>>>> +			else if (swap && fep->parent == newid)
>>>> +				fep->parent = oldid;
>>>>  		}
>>>>  	}
>>>>  }
>>>> @@ -4256,6 +4264,7 @@ out:
>>>>  
>>>>  struct print_flags renameat2_flags [] = {
>>>>  	{ RENAME_NOREPLACE, "NOREPLACE"},
>>>> +	{ RENAME_EXCHANGE, "EXCHANGE"},
>>>>  	{ RENAME_WHITEOUT, "WHITEOUT"},
>>>>  	{ -1, NULL}
>>>>  };
>>>> @@ -4291,41 +4300,76 @@ 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, and in
>>>> +	 * order to maintain filelist/filename integrity, we should
>>>> +	 * restrict exchange operation to files of the same type.
>>>> +	 */
>>>> +	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;
>>>> +		bool swap = (mode == RENAME_EXCHANGE) ? true : false;
>>>>  
>>>>  		oldid = fep->id;
>>>>  		oldparid = fep->parent;
>>>>  
>>>> +		/*
>>>> +		 * Swap the parent ids for RENAME_EXCHANGE, and replace the
>>>> +		 * old parent id for the others.
>>>> +		 */
>>>>  		if (flp - flist == FT_DIR)
>>>> -			fix_parent(oldid, id);
>>>> +			fix_parent(oldid, id, swap);
>>>>  
>>>>  		if (mode == RENAME_WHITEOUT) {
>>>>  			fep->xattr_counter = 0;
>>>>  			add_to_flist(flp - flist, id, parid, xattr_counter);
>>>> +		} 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);
>>>> @@ -4359,6 +4403,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
>>>>
>>>
>>
>> -- 
>> kaixuxia
> 

-- 
kaixuxia