Re: [PATCH v5] fsstress: add renameat2 support

[kaixuxia <xiakaixu1987@gmail.com>]

On 2019/10/23 21:01, Brian Foster wrote:
> 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
> 
I think the source entry id and parentid here are overwritten by
del_from_flist() call above for all kinds of rename operations,
we should show the actually values. 

> ... 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).
>Sounds reasonable, will do this in next version.

> 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.
> 
Sure.

> 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').
> 
Yeah, I have tested this on my vm when restricting RWHITEOUT to device
nodes, the associated fstest still can reproduced the deadlock problem,
and the run time is very short. 

> 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).

Thanks for your comments, will do it in next version. 

> > 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
> 

-- 
kaixuxia