Re: [PATCH] fsstress: add renameat2 support

Brian Foster
2019-10-15T14:55:44Z

On Fri, Oct 11, 2019 at 03:56:01PM +0800, kaixuxia wrote:
> Support the renameat2 syscall in fsstress.
>
> Signed-off-by: kaixuxia <kaixuxia@tencent.com>
> ---
> 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);

Perhaps we should preserve the rename() call with an ifdef or something
(similar to src/renameat2.c) in the event that renameat2 is not
available. In fact, I suppose we'd want to compile out the new renameat2
based ops as well so the user doesn't have to disable them where they
aren't supported.

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

It looks like the existing code looks up a source entry and a target
directory and generates a new name to serve as the rename target entry.
Here we create the entry so we can perform an exchange. Can we check
mode earlier and instead look up another existing entry if this is an
exchange, else run the existing code to generate a name?

Other than those couple things this looks pretty good to me.

Brian

> 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;
> --
> 1.8.3.1
>
> --
> kaixuxia