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