On 2019/10/15 23:05, Darrick J. Wong wrote:
> On Tue, Oct 15, 2019 at 10:34:31AM +0800, kaixuxia wrote:
>> According to the comments, after adding this fsstress renameat2 support,
>> the deadlock between the AGI and AGF with RENAME_WHITEOUT can be reproduced
>> by using customized parameters(limited to rename_whiteout and creates). If
>> this patch is okay, I will send the fsstress test patch.
>
> /me looks forward to that, particularly because I asked weeks ago if the
> xfs_droplink calls in xfs_rename() could try to lock the AGI after we'd
> already locked the AGF for the directory expansion, but nobody sent an
> answer...
Yeah, It's been a long time since we talked this xfs_droplink() deadlock
problem. I already have the simple fix patch, now I'm trying to get a
xfstests test patch to reproduce this deadlock problem with high possibility.
I will send the fix patch ASAP.
>
>> So, Eryu, Brian, comments?
>>
>> On 2019/10/11 15:56, kaixuxia wrote:
>>> Support the renameat2 syscall in fsstress.
>>>
>>> Signed-off-by: kaixuxia <kaixuxia@xxxxxxxxxxx>
>>> ---
>>> ltp/fsstress.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
>>> 1 file changed, 79 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/ltp/fsstress.c b/ltp/fsstress.c
>>> index 51976f5..21529a2 100644
>>> --- a/ltp/fsstress.c
>>> +++ b/ltp/fsstress.c
>>> @@ -44,6 +44,16 @@ io_context_t io_ctx;
>>> #define IOV_MAX 1024
>>> #endif
>>>
>>> +#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
>>> +
>>> #define FILELEN_MAX (32*4096)
>>>
>>> typedef enum {
>>> @@ -85,6 +95,9 @@ typedef enum {
>>> OP_READV,
>>> OP_REMOVEFATTR,
>>> OP_RENAME,
>>> + OP_RNOREPLACE,
>>> + OP_REXCHANGE,
>>> + OP_RWHITEOUT,
>>> OP_RESVSP,
>>> OP_RMDIR,
>>> OP_SETATTR,
>>> @@ -203,6 +216,9 @@ void readlink_f(int, long);
>>> 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);
>>> void setattr_f(int, long);
>>> @@ -262,6 +278,9 @@ opdesc_t ops[] = {
>>> /* remove (delete) extended attribute */
>>> { 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 },
>>> /* set attribute flag (FS_IOC_SETFLAGS ioctl) */
>>> @@ -354,7 +373,7 @@ int open_path(pathname_t *, int);
>>> DIR *opendir_path(pathname_t *);
>>> void process_freq(char *);
>>> int readlink_path(pathname_t *, char *, size_t);
>>> -int rename_path(pathname_t *, pathname_t *);
>>> +int rename_path(pathname_t *, pathname_t *, int);
>>> int rmdir_path(pathname_t *);
>>> void separate_pathname(pathname_t *, char *, pathname_t *);
>>> void show_ops(int, char *);
>>> @@ -1519,7 +1538,7 @@ readlink_path(pathname_t *name, char *lbuf, size_t lbufsiz)
>>> }
>>>
>>> int
>>> -rename_path(pathname_t *name1, pathname_t *name2)
>>> +rename_path(pathname_t *name1, pathname_t *name2, int mode)
>>> {
>>> char buf1[NAME_MAX + 1];
>>> char buf2[NAME_MAX + 1];
>>> @@ -1528,14 +1547,14 @@ rename_path(pathname_t *name1, pathname_t *name2)
>>> pathname_t newname2;
>>> int rval;
>>>
>>> - rval = rename(name1->path, name2->path);
>>> + rval = syscall(__NR_renameat2, AT_FDCWD, name1->path, AT_FDCWD, name2->path, mode);
>
> For the rename(..., 0) case, would we be crippling fsstress if the
> kernel doesn't know about renameat2 and doesn't fall back to renameat()
> or regular rename()? I guess renameat2 showed up in 3.15 which was
> quite a long time ago except in RHEL land. :)
>
Yeah, will preserve the rename() call when it is not available in V2.
> --D
>
>>> if (rval >= 0 || errno != ENAMETOOLONG)
>>> return rval;
>>> separate_pathname(name1, buf1, &newname1);
>>> separate_pathname(name2, buf2, &newname2);
>>> if (strcmp(buf1, buf2) == 0) {
>>> if (chdir(buf1) == 0) {
>>> - rval = rename_path(&newname1, &newname2);
>>> + rval = rename_path(&newname1, &newname2, mode);
>>> assert(chdir("..") == 0);
>>> }
>>> } else {
>>> @@ -1555,7 +1574,7 @@ rename_path(pathname_t *name1, pathname_t *name2)
>>> append_pathname(&newname2, "../");
>>> append_pathname(&newname2, name2->path);
>>> if (chdir(buf1) == 0) {
>>> - rval = rename_path(&newname1, &newname2);
>>> + rval = rename_path(&newname1, &newname2, mode);
>>> assert(chdir("..") == 0);
>>> }
>>> } else {
>>> @@ -1563,7 +1582,7 @@ rename_path(pathname_t *name1, pathname_t *name2)
>>> append_pathname(&newname1, "../");
>>> append_pathname(&newname1, name1->path);
>>> if (chdir(buf2) == 0) {
>>> - rval = rename_path(&newname1, &newname2);
>>> + rval = rename_path(&newname1, &newname2, mode);
>>> assert(chdir("..") == 0);
>>> }
>>> }
>>> @@ -4215,8 +4234,18 @@ out:
>>> free_pathname(&f);
>>> }
>>>
>>> +struct print_flags renameat2_flags [] = {
>>> + { RENAME_NOREPLACE, "NOREPLACE"},
>>> + { RENAME_EXCHANGE, "EXCHANGE"},
>>> + { RENAME_WHITEOUT, "WHITEOUT"},
>>> + { -1, NULL}
>>> +};
>>> +
>>> +#define translate_renameat2_flags(mode) \
>>> + ({translate_flags(mode, "|", renameat2_flags);})
>>> +
>>> void
>>> -rename_f(int opno, long r)
>>> +do_renameat2(int opno, long r, int mode)
>>> {
>>> fent_t *dfep;
>>> int e;
>>> @@ -4229,6 +4258,7 @@ rename_f(int opno, long r)
>>> int parid;
>>> int v;
>>> int v1;
>>> + int fd;
>>>
>>> /* get an existing path for the source of the rename */
>>> init_pathname(&f);
>>> @@ -4260,7 +4290,21 @@ rename_f(int opno, long r)
>>> free_pathname(&f);
>>> return;
>>> }
>>> - e = rename_path(&f, &newf) < 0 ? errno : 0;
>>> + /* Both pathnames must exist for the RENAME_EXCHANGE */
>>> + if (mode == RENAME_EXCHANGE) {
>>> + fd = creat_path(&newf, 0666);
>>> + e = fd < 0 ? errno : 0;
>>> + check_cwd();
>>> + if (fd < 0) {
>>> + if (v)
>>> + printf("%d/%d: renameat2 - creat %s failed %d\n",
>>> + procid, opno, newf.path, e);
>>> + 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;
>>> @@ -4273,12 +4317,13 @@ rename_f(int opno, long r)
>>> 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 +4332,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;
>>>
>>
>> --
>> kaixuxia
--
kaixuxia
