| Message ID | 20220617172641.122296-1-cristian.marussi@arm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix mountns01/02/03/04 removing unneeded final umounts | expand |
Hi Cristian > Running LTP20220527 release it appears that the recently re-written tests > mountns02/03/04 now throw a warning on their final umount attempt: > > <<<test_output>>> > tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s > mountns04.c:38: TPASS: unbindable mount passed > tst_device.c:395: TWARN: umount('A') failed with EINVAL > mountns.h:36: TWARN: umount(A) failed: EINVAL (22) > tst_device.c:434: TINFO: No device is mounted at B I guess this failure because /proc/mounts still has "A", so tst_is_mounted returns true and umount failed. we can reproduce it by using a /mnt/A mntpoint in /proc/mounts. > > Moreover, the underlying safe_umount() then upgrades the TWARN emitted > from tst_umount to a TBROK, so causing the test to completely fail: > > Summary: > passed 1 > failed 0 > broken 0 > skipped 0 > warnings 2 > <<<execution_status>>> > initiation_status="ok" > duration=0 termination_type=exited termination_id=4 corefile=no > > In fact, the final umounts on DIRA seem not needed in mountns02/03/04 > looking at the previous chain of umounts calls and the tests logic and, > in any case, the .cleanup functions of all these tests take care to > finally unmount both DIRA/DIRB after having checked if they were still > mounted at all. > > Remove all the final SAFE_UMOUNT calls (even for mountns01) since all the > possibly needed umounts are already eventually performed by .cleanup. Yes, it can fix this problem but these case still will fail when /proc/mounts has a B mntpoint, ie mount /dev/sda11 /mnt/B ./mountns04 tst_test.c:1528: TINFO: Timeout per run is 0h 00m 30s mountns04.c:38: TPASS: unbindable mount passed tst_device.c:395: TWARN: umount('B') failed with EINVAL mountns.h:39: TWARN: umount(B) failed: EINVAL (22) So I think a right fix should fix the tst_is_mounted function or umount_folders instead of removing final SAFE_UMOUNT. Best Regards Yang Xu > > Cc: Andrea Cervesato<andrea.cervesato@suse.de> > Cc: Cyril Hrubis<chrubis@suse.cz> > Signed-off-by: Cristian Marussi<cristian.marussi@arm.com> > --- > testcases/kernel/containers/mountns/mountns01.c | 2 -- > testcases/kernel/containers/mountns/mountns02.c | 2 -- > testcases/kernel/containers/mountns/mountns03.c | 2 -- > testcases/kernel/containers/mountns/mountns04.c | 2 -- > 4 files changed, 8 deletions(-) > > diff --git a/testcases/kernel/containers/mountns/mountns01.c b/testcases/kernel/containers/mountns/mountns01.c > index 452fe1d10..7c9461e4d 100644 > --- a/testcases/kernel/containers/mountns/mountns01.c > +++ b/testcases/kernel/containers/mountns/mountns01.c > @@ -85,8 +85,6 @@ static void run(void) > tst_res(TFAIL, "shared mount in child failed"); > > TST_CHECKPOINT_WAKE(0); > - > - SAFE_UMOUNT(DIRA); > } > > static void setup(void) > diff --git a/testcases/kernel/containers/mountns/mountns02.c b/testcases/kernel/containers/mountns/mountns02.c > index cbd435958..0693bb9f6 100644 > --- a/testcases/kernel/containers/mountns/mountns02.c > +++ b/testcases/kernel/containers/mountns/mountns02.c > @@ -86,8 +86,6 @@ static void run(void) > tst_res(TPASS, "private mount in child passed"); > > TST_CHECKPOINT_WAKE(0); > - > - SAFE_UMOUNT(DIRA); > } > > static void setup(void) > diff --git a/testcases/kernel/containers/mountns/mountns03.c b/testcases/kernel/containers/mountns/mountns03.c > index 5c19a96a9..aceab32ae 100644 > --- a/testcases/kernel/containers/mountns/mountns03.c > +++ b/testcases/kernel/containers/mountns/mountns03.c > @@ -96,8 +96,6 @@ static void run(void) > tst_res(TFAIL, "propagation form slave mount failed"); > > TST_CHECKPOINT_WAKE(0); > - > - SAFE_UMOUNT(DIRA); > } > > static void setup(void) > diff --git a/testcases/kernel/containers/mountns/mountns04.c b/testcases/kernel/containers/mountns/mountns04.c > index cc63a03d9..d0ecf7667 100644 > --- a/testcases/kernel/containers/mountns/mountns04.c > +++ b/testcases/kernel/containers/mountns/mountns04.c > @@ -40,8 +40,6 @@ static void run(void) > SAFE_UMOUNT(DIRB); > tst_res(TFAIL, "unbindable mount faled"); > } > - > - SAFE_UMOUNT(DIRA); > } > > static void setup(void)
On Mon, Jun 20, 2022 at 03:55:53AM +0000, xuyang2018.jy@fujitsu.com wrote: > Hi Cristian > Hi Yang Xu thanks for the feedback. > > Running LTP20220527 release it appears that the recently re-written tests > > mountns02/03/04 now throw a warning on their final umount attempt: > > > > <<<test_output>>> > > tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s > > mountns04.c:38: TPASS: unbindable mount passed > > tst_device.c:395: TWARN: umount('A') failed with EINVAL > > mountns.h:36: TWARN: umount(A) failed: EINVAL (22) > > tst_device.c:434: TINFO: No device is mounted at B > > I guess this failure because /proc/mounts still has "A", so > tst_is_mounted returns true and umount failed. > > we can reproduce it by using a /mnt/A mntpoint in /proc/mounts. Yes indeed, reasoning about your feedback on this I think the issue really is different from what I thought (my bad): it is the final SAFE_UMOUNT in the cleanup function that fails really because the tst_is_mounted() that protects it is based on a simnple strstr() on /proc/mounts/ and the directory names used are too much simple ("A") so it is sufficient to have some unrelated mountpoint like "/mnt/this_is_Another_mnt" to fool tst_is_mounted and make it think our "A" still needs to be removed. This also would explin why this bug has not been noticed elsewhere... ...it depends on your final running environment. (i.e. mountpoints) Using a bit more peculiar names like to __DIR_A __DIR_B solves for me indeed: root@debian-arm64-bullseye:~# ./mountns01 tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s mountns01.c:42: TPASS: shared mount in parent passed mountns01.c:83: TPASS: shared mount in child passed tst_device.c:434: TINFO: No device is mounted at __DIR_B Summary: passed 2 failed 0 broken 0 skipped 0 warnings 0 root@debian-arm64-bullseye:~# ./mountns02 tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s mountns02.c:45: TPASS: private mount in parent passed mountns02.c:86: TPASS: private mount in child passed tst_device.c:434: TINFO: No device is mounted at __DIR_A tst_device.c:434: TINFO: No device is mounted at __DIR_B Summary: passed 2 failed 0 broken 0 skipped 0 warnings 0 root@debian-arm64-bullseye:~# ./mountns03 tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s mountns03.c:51: TPASS: propagation to slave mount passed mountns03.c:94: TPASS: propagation from slave mount passed tst_device.c:434: TINFO: No device is mounted at __DIR_A tst_device.c:434: TINFO: No device is mounted at __DIR_B Summary: passed 2 failed 0 broken 0 skipped 0 warnings 0 root@debian-arm64-bullseye:~# ./mountns04 tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s mountns04.c:38: TPASS: unbindable mount passed tst_device.c:434: TINFO: No device is mounted at __DIR_A tst_device.c:434: TINFO: No device is mounted at __DIR_B > > > > Moreover, the underlying safe_umount() then upgrades the TWARN emitted > > from tst_umount to a TBROK, so causing the test to completely fail: > > > > Summary: > > passed 1 > > failed 0 > > broken 0 > > skipped 0 > > warnings 2 > > <<<execution_status>>> > > initiation_status="ok" > > duration=0 termination_type=exited termination_id=4 corefile=no > > > > In fact, the final umounts on DIRA seem not needed in mountns02/03/04 > > looking at the previous chain of umounts calls and the tests logic and, > > in any case, the .cleanup functions of all these tests take care to > > finally unmount both DIRA/DIRB after having checked if they were still > > mounted at all. > > > > Remove all the final SAFE_UMOUNT calls (even for mountns01) since all the > > possibly needed umounts are already eventually performed by .cleanup. > > Yes, it can fix this problem but these case still will fail when > /proc/mounts has a B mntpoint, > > ie > mount /dev/sda11 /mnt/B > ./mountns04 > tst_test.c:1528: TINFO: Timeout per run is 0h 00m 30s > mountns04.c:38: TPASS: unbindable mount passed > tst_device.c:395: TWARN: umount('B') failed with EINVAL > mountns.h:39: TWARN: umount(B) failed: EINVAL (22) > > So I think a right fix should fix the tst_is_mounted function or > umount_folders instead of removing final SAFE_UMOUNT. > Agreed that the failure was elsewhere as above mentioned, so I will keep the final SAFE_UMOUNT and just change the dir names and let the cleanups remove what remains to be removed (e.g. in case of error) Thanks, Cristian
diff --git a/testcases/kernel/containers/mountns/mountns01.c b/testcases/kernel/containers/mountns/mountns01.c index 452fe1d10..7c9461e4d 100644 --- a/testcases/kernel/containers/mountns/mountns01.c +++ b/testcases/kernel/containers/mountns/mountns01.c @@ -85,8 +85,6 @@ static void run(void) tst_res(TFAIL, "shared mount in child failed"); TST_CHECKPOINT_WAKE(0); - - SAFE_UMOUNT(DIRA); } static void setup(void) diff --git a/testcases/kernel/containers/mountns/mountns02.c b/testcases/kernel/containers/mountns/mountns02.c index cbd435958..0693bb9f6 100644 --- a/testcases/kernel/containers/mountns/mountns02.c +++ b/testcases/kernel/containers/mountns/mountns02.c @@ -86,8 +86,6 @@ static void run(void) tst_res(TPASS, "private mount in child passed"); TST_CHECKPOINT_WAKE(0); - - SAFE_UMOUNT(DIRA); } static void setup(void) diff --git a/testcases/kernel/containers/mountns/mountns03.c b/testcases/kernel/containers/mountns/mountns03.c index 5c19a96a9..aceab32ae 100644 --- a/testcases/kernel/containers/mountns/mountns03.c +++ b/testcases/kernel/containers/mountns/mountns03.c @@ -96,8 +96,6 @@ static void run(void) tst_res(TFAIL, "propagation form slave mount failed"); TST_CHECKPOINT_WAKE(0); - - SAFE_UMOUNT(DIRA); } static void setup(void) diff --git a/testcases/kernel/containers/mountns/mountns04.c b/testcases/kernel/containers/mountns/mountns04.c index cc63a03d9..d0ecf7667 100644 --- a/testcases/kernel/containers/mountns/mountns04.c +++ b/testcases/kernel/containers/mountns/mountns04.c @@ -40,8 +40,6 @@ static void run(void) SAFE_UMOUNT(DIRB); tst_res(TFAIL, "unbindable mount faled"); } - - SAFE_UMOUNT(DIRA); } static void setup(void)
Running LTP20220527 release it appears that the recently re-written tests mountns02/03/04 now throw a warning on their final umount attempt: <<<test_output>>> tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s mountns04.c:38: TPASS: unbindable mount passed tst_device.c:395: TWARN: umount('A') failed with EINVAL mountns.h:36: TWARN: umount(A) failed: EINVAL (22) tst_device.c:434: TINFO: No device is mounted at B Moreover, the underlying safe_umount() then upgrades the TWARN emitted from tst_umount to a TBROK, so causing the test to completely fail: Summary: passed 1 failed 0 broken 0 skipped 0 warnings 2 <<<execution_status>>> initiation_status="ok" duration=0 termination_type=exited termination_id=4 corefile=no In fact, the final umounts on DIRA seem not needed in mountns02/03/04 looking at the previous chain of umounts calls and the tests logic and, in any case, the .cleanup functions of all these tests take care to finally unmount both DIRA/DIRB after having checked if they were still mounted at all. Remove all the final SAFE_UMOUNT calls (even for mountns01) since all the possibly needed umounts are already eventually performed by .cleanup. Cc: Andrea Cervesato <andrea.cervesato@suse.de> Cc: Cyril Hrubis <chrubis@suse.cz> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- testcases/kernel/containers/mountns/mountns01.c | 2 -- testcases/kernel/containers/mountns/mountns02.c | 2 -- testcases/kernel/containers/mountns/mountns03.c | 2 -- testcases/kernel/containers/mountns/mountns04.c | 2 -- 4 files changed, 8 deletions(-)