| Message ID | 20231227000430.30224-1-wegao@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] move_mount03: check allow to mount beneath top mount | expand |
Hi Wei, I suppose there was no v1, right? -i2 fails, please fix it. # move_mount03 -i2 move_mount03.c:79: TINFO: Mounting none to /tmp/LTP_movcovMfK/LTP_DIR_A fstyp=tmpfs flags=0 move_mount03.c:80: TINFO: Mounting none to /tmp/LTP_movcovMfK/LTP_DIR_B fstyp=tmpfs flags=0 move_mount03.c:92: TPASS: move_mount(fda, "", fdb, "", MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH | MOVE_MOUNT_T_EMPTY_PATH) passed move_mount03.c:98: TPASS: access(DIRB "/B", F_OK) passed move_mount03.c:99: TINFO: Umounting /tmp/LTP_movcovMfK/LTP_DIR_B move_mount03.c:100: TPASS: access(DIRB "/A", F_OK) passed move_mount03.c:102: TINFO: Umounting /tmp/LTP_movcovMfK/LTP_DIR_B move_mount03.c:103: TINFO: Umounting /tmp/LTP_movcovMfK/LTP_DIR_A move_mount03.c:77: TBROK: mkdir(LTP_DIR_A, 0777) failed: EEXIST (17) NOTE: you can speed up the review process, if you check list of common errors before sending patch to ML https://github.com/linux-test-project/ltp/wiki/Maintainer-Patch-Review-Checklist#how-to-find-clear-errors make check-move_mount03 CHECK testcases/kernel/syscalls/move_mount/move_mount03.c move_mount03.c:92:9: error: undefined identifier 'MOVE_MOUNT_BENEATH' I see false positive from checkpatch.pl, it would be interesting to check if it can be fixed. > diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c > new file mode 100644 > index 000000000..dadb19178 > --- /dev/null > +++ b/testcases/kernel/syscalls/move_mount/move_mount03.c > @@ -0,0 +1,111 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2023 Christian Brauner <brauner@kernel.org> > + * Copyright (c) 2023 Wei Gao <wegao@suse.com> > + */ > + > +/*\ > + * [Description] > + * > + * Verify allow to mount beneath top mount base following commit: This will be very badly formatted in docs. > + * commit 6ac392815628f317fcfdca1a39df00b9cc4ebc8b > + * Author: Christian Brauner <brauner@kernel.org> > + * Date: Wed May 3 13:18:42 2023 +0200 > + * fs: allow to mount beneath top mount > + * > + * Above commit has heavily commented but i found following commit > + * contain simple summary of this feature for easy understanding: > + * > + * commit c0a572d9d32fe1e95672f24e860776dba0750a38 > + * Author: Linus Torvalds <torvalds@linux-foundation.org> > + * TL;DR: > + * Please generate metadata and have look at result. The output below looks ugly. Either you find a way to preformat it as generated output (put extra space after * in the comment - equivalent of <pre>... </pre> HTML output) or put it into /* * */, so that it's not in docs (good for some info useful in C source but more related to the test implementation so that it does not have to be in generated docs). > + * > mount -t ext4 /dev/sda /mnt > + * | > + * --/mnt /dev/sda ext4 > + * > + * > mount --beneath -t xfs /dev/sdb /mnt > + * | > + * --/mnt /dev/sdb xfs > + * --/mnt /dev/sda ext4 > + * > + * > umount /mnt > + * | > + * --/mnt /dev/sdb xfs > + * > + * So base above scenario design following scenario for LTP check: > + * > + * > mount -t tmpfs /DIRA > + * | > + * --/DIRA(create A file within DIRA) > + * > + * > mount -t tmpfs /DIRB > + * | > + * --/DIRA(create B file within DIRB) > + * > + * > move_mount --beneath /DIRA /DIRB > + * | > + * --/mnt /DIRA /DIRB > + * --/mnt /DIRB > + * > + * If you check content of /DIRB, you can see file B > + * > + * > umount /DIRB > + * | > + * --/mnt /DIRA /DIRB > + * Check content of /DIRB, you can see file A exist since > + * current /DIRB mount source is already become /DIRA > + * > + * More detail can be found in following link: > + * Link: https://lwn.net/Articles/930591/ > + * Link: https://github.com/brauner/move-mount-beneath Link is bogus (we use it only as tag in the git commit message, not in docs). * See also: * https://lwn.net/Articles/930591/ * https://github.com/brauner/move-mount-beneath */ > + */ > + > +#include <stdio.h> > + > +#include "tst_test.h" > +#include "lapi/fsmount.h" > +#include "lapi/sched.h" > + > +#define DIRA "LTP_DIR_A" > +#define DIRB "LTP_DIR_B" > + > +static void run(void) > +{ > + int fda, fdb; > + > + SAFE_MKDIR(DIRA, 0777); > + SAFE_MKDIR(DIRB, 0777); > + SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0); > + SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0); > + SAFE_TOUCH(DIRA "/A", 0, NULL); > + SAFE_TOUCH(DIRB "/B", 0, NULL); Maybe whole code in above could be in setup() function, it can be created only once even we run test more times with -iN, right? Setup function is for test speedup (imagine somebody run test with -i10000, why to create files all the time? > + > + TEST(fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE)); > + > + if (fda == -1) { > + tst_res(TFAIL | TTERRNO, "open_tree() failed"); > + return; > + } Maybe this should also be in the setup function. also not using TEST() for it? fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE); if (fda > 0) tst_brk(TBROK | TERRNO, "open_tree() failed"); > + > + fdb = SAFE_OPEN(DIRB, O_PATH | O_NOFOLLOW, 0666); > + TST_EXP_PASS(move_mount(fda, "", fdb, "", > + MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH | > + MOVE_MOUNT_T_EMPTY_PATH)); I wonder what happen if any of the following SAFE_*() functions fail. There will be unrelated errors and left mount directories. IMHO at least SAFE_UMOUNT() should be in cleanup functions and guarded by flags (e.g. not trying to umount() when nothing was mounted because tst_brk() earlier). Kind regards, Petr > + SAFE_CLOSE(fda); > + SAFE_CLOSE(fdb); > + > + TST_EXP_PASS(access(DIRB "/B", F_OK)); > + SAFE_UMOUNT(DIRB); > + TST_EXP_PASS(access(DIRB "/A", F_OK)); > + > + SAFE_UMOUNT(DIRB); > + SAFE_UMOUNT(DIRA); > +} > + > +static struct tst_test test = { > + .test_all = run, > + .needs_root = 1, > + .min_kver = "6.5.0", > + .needs_tmpdir = 1, > +};
Hi Petr Very strange I have not found this email in my mutt, so I have to reply it in my outlook, sorry for inconvenience. Regards Gao Wei I suppose there was no v1, right? [GW] V1: https://patchwork.ozlabs.org/project/ltp/patch/20230913101542.18550-1-wegao@suse.com/ NOTE: you can speed up the review process, if you check list of common errors before sending patch to ML https://github.com/linux-test-project/ltp/wiki/Maintainer-Patch-Review-Checklist#how-to-find-clear-errors [GW]: patch v2 send with following check: make check, m32, run with -i3 I see false positive from checkpatch.pl, it would be interesting to check if it can be fixed. [GW]: v1 has no this issue, I wrongly remove some change from v1. > +static void run(void) > +{ > + int fda, fdb; > + > + SAFE_MKDIR(DIRA, 0777); > + SAFE_MKDIR(DIRB, 0777); > + SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0); > + SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0); > + SAFE_TOUCH(DIRA "/A", 0, NULL); > + SAFE_TOUCH(DIRB "/B", 0, NULL); Maybe whole code in above could be in setup() function, it can be created only once even we run test more times with -iN, right? Setup function is for test speedup (imagine somebody run test with -i10000, why to create files all the time? [GW]: MKDIR should put into setup otherwise issue will happen, but mount & touch should keep within run function since move_mount will change mount point later and we need rebuild mount env again when it rerun.
diff --git a/runtest/syscalls b/runtest/syscalls index b1125dd75..04b758fd9 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -824,6 +824,7 @@ mount_setattr01 mount_setattr01 move_mount01 move_mount01 move_mount02 move_mount02 +move_mount03 move_mount03 move_pages01 move_pages01 move_pages02 move_pages02 diff --git a/testcases/kernel/syscalls/move_mount/.gitignore b/testcases/kernel/syscalls/move_mount/.gitignore index 83ae40145..ddfe10128 100644 --- a/testcases/kernel/syscalls/move_mount/.gitignore +++ b/testcases/kernel/syscalls/move_mount/.gitignore @@ -1,2 +1,3 @@ /move_mount01 /move_mount02 +/move_mount03 diff --git a/testcases/kernel/syscalls/move_mount/move_mount03.c b/testcases/kernel/syscalls/move_mount/move_mount03.c new file mode 100644 index 000000000..dadb19178 --- /dev/null +++ b/testcases/kernel/syscalls/move_mount/move_mount03.c @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2023 Christian Brauner <brauner@kernel.org> + * Copyright (c) 2023 Wei Gao <wegao@suse.com> + */ + +/*\ + * [Description] + * + * Verify allow to mount beneath top mount base following commit: + * commit 6ac392815628f317fcfdca1a39df00b9cc4ebc8b + * Author: Christian Brauner <brauner@kernel.org> + * Date: Wed May 3 13:18:42 2023 +0200 + * fs: allow to mount beneath top mount + * + * Above commit has heavily commented but i found following commit + * contain simple summary of this feature for easy understanding: + * + * commit c0a572d9d32fe1e95672f24e860776dba0750a38 + * Author: Linus Torvalds <torvalds@linux-foundation.org> + * TL;DR: + * + * > mount -t ext4 /dev/sda /mnt + * | + * --/mnt /dev/sda ext4 + * + * > mount --beneath -t xfs /dev/sdb /mnt + * | + * --/mnt /dev/sdb xfs + * --/mnt /dev/sda ext4 + * + * > umount /mnt + * | + * --/mnt /dev/sdb xfs + * + * So base above scenario design following scenario for LTP check: + * + * > mount -t tmpfs /DIRA + * | + * --/DIRA(create A file within DIRA) + * + * > mount -t tmpfs /DIRB + * | + * --/DIRA(create B file within DIRB) + * + * > move_mount --beneath /DIRA /DIRB + * | + * --/mnt /DIRA /DIRB + * --/mnt /DIRB + * + * If you check content of /DIRB, you can see file B + * + * > umount /DIRB + * | + * --/mnt /DIRA /DIRB + * Check content of /DIRB, you can see file A exist since + * current /DIRB mount source is already become /DIRA + * + * More detail can be found in following link: + * Link: https://lwn.net/Articles/930591/ + * Link: https://github.com/brauner/move-mount-beneath + */ + +#include <stdio.h> + +#include "tst_test.h" +#include "lapi/fsmount.h" +#include "lapi/sched.h" + +#define DIRA "LTP_DIR_A" +#define DIRB "LTP_DIR_B" + +static void run(void) +{ + int fda, fdb; + + SAFE_MKDIR(DIRA, 0777); + SAFE_MKDIR(DIRB, 0777); + SAFE_MOUNT("none", DIRA, "tmpfs", 0, 0); + SAFE_MOUNT("none", DIRB, "tmpfs", 0, 0); + SAFE_TOUCH(DIRA "/A", 0, NULL); + SAFE_TOUCH(DIRB "/B", 0, NULL); + + TEST(fda = open_tree(AT_FDCWD, DIRA, OPEN_TREE_CLOEXEC | OPEN_TREE_CLONE)); + + if (fda == -1) { + tst_res(TFAIL | TTERRNO, "open_tree() failed"); + return; + } + + fdb = SAFE_OPEN(DIRB, O_PATH | O_NOFOLLOW, 0666); + TST_EXP_PASS(move_mount(fda, "", fdb, "", + MOVE_MOUNT_BENEATH | MOVE_MOUNT_F_EMPTY_PATH | + MOVE_MOUNT_T_EMPTY_PATH)); + SAFE_CLOSE(fda); + SAFE_CLOSE(fdb); + + TST_EXP_PASS(access(DIRB "/B", F_OK)); + SAFE_UMOUNT(DIRB); + TST_EXP_PASS(access(DIRB "/A", F_OK)); + + SAFE_UMOUNT(DIRB); + SAFE_UMOUNT(DIRA); +} + +static struct tst_test test = { + .test_all = run, + .needs_root = 1, + .min_kver = "6.5.0", + .needs_tmpdir = 1, +};
Signed-off-by: Wei Gao <wegao@suse.com> --- runtest/syscalls | 1 + .../kernel/syscalls/move_mount/.gitignore | 1 + .../kernel/syscalls/move_mount/move_mount03.c | 111 ++++++++++++++++++ 3 files changed, 113 insertions(+) create mode 100644 testcases/kernel/syscalls/move_mount/move_mount03.c