* [LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure
@ 2021-06-28 3:30 Leo Liang
2021-06-28 7:24 ` Richard Palethorpe
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Leo Liang @ 2021-06-28 3:30 UTC (permalink / raw)
To: ltp
From a151d48235629a125d5db57dd76c96fd951d5293 Mon Sep 17 00:00:00 2001
From: Leo Yu-Chi Liang <ycliang@andestech.com>
Date: Mon, 28 Jun 2021 11:05:54 +0800
Subject: [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure
The test sequence
mount -t cgroup -o <controllers> <path>
mkdir <path>/<dir>
rmdir <path>/<dir>
umount <path>
mount -t cgroup -o <controllers> <path>
would easily fail at the last mount with -EBUSY.
The reason is that this test sequence have the chance of
missing a release code path when doing rmdir and umount.
Add sync between every "rmdir, umount" pair to fix the problem.
Fixes: #839
Signed-off-by: Leo Yu-Chi Liang <ycliang@andestech.com>
---
.../kernel/controllers/cgroup/cgroup_regression_test.sh | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh
index 1f7f3820e..9a00df101 100755
--- a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh
+++ b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh
@@ -145,6 +145,7 @@ test2()
fi
rmdir cgroup/0 cgroup/1
+ sync
umount cgroup/
}
@@ -193,6 +194,7 @@ test3()
wait $pid2 2>/dev/null
rmdir $cpu_subsys_path/0 2> /dev/null
+ sync
umount cgroup/ 2> /dev/null
check_kernel_bug
}
@@ -222,6 +224,7 @@ test4()
mount -t cgroup -o none,name=foo cgroup cgroup/
mkdir cgroup/0
rmdir cgroup/0
+ sync
umount cgroup/
if dmesg | grep -q "MAX_LOCKDEP_SUBCLASSES too low"; then
@@ -254,6 +257,7 @@ test5()
mount -t cgroup none cgroup 2> /dev/null
mkdir cgroup/0
rmdir cgroup/0
+ sync
umount cgroup/ 2> /dev/null
check_kernel_bug
}
@@ -290,6 +294,7 @@ test6()
mount -t cgroup -o ns xxx cgroup/ > /dev/null 2>&1
rmdir cgroup/[1-9]* > /dev/null 2>&1
+ sync
umount cgroup/
check_kernel_bug
}
@@ -324,6 +329,7 @@ test_7_1()
mkdir $subsys_path/0
sleep 100 < $subsys_path/0 & # add refcnt to this dir
rmdir $subsys_path/0
+ sync
# remount with new subsystems added
# since 2.6.28, this remount will fail
@@ -349,6 +355,7 @@ test_7_2()
mkdir cgroup/0
sleep 100 < cgroup/0 & # add refcnt to this dir
rmdir cgroup/0
+ sync
# remount with some subsystems removed
# since 2.6.28, this remount will fail
--
2.17.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure
2021-06-28 3:30 [LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure Leo Liang
@ 2021-06-28 7:24 ` Richard Palethorpe
2021-06-28 7:36 ` Joerg Vehlow
2021-06-29 7:01 ` xuyang2018.jy
2 siblings, 0 replies; 9+ messages in thread
From: Richard Palethorpe @ 2021-06-28 7:24 UTC (permalink / raw)
To: ltp
Hello,
Leo Liang <ycliang@andestech.com> writes:
> From a151d48235629a125d5db57dd76c96fd951d5293 Mon Sep 17 00:00:00 2001
> From: Leo Yu-Chi Liang <ycliang@andestech.com>
> Date: Mon, 28 Jun 2021 11:05:54 +0800
> Subject: [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure
>
> The test sequence
> mount -t cgroup -o <controllers> <path>
> mkdir <path>/<dir>
> rmdir <path>/<dir>
> umount <path>
> mount -t cgroup -o <controllers> <path>
> would easily fail at the last mount with -EBUSY.
>
> The reason is that this test sequence have the chance of
> missing a release code path when doing rmdir and umount.
>
> Add sync between every "rmdir, umount" pair to fix the problem.
>
> Fixes: #839
>
> Signed-off-by: Leo Yu-Chi Liang <ycliang@andestech.com>
Acked-by: Richard Palethorpe <rpalethorpe@suse.com>
--
Thank you,
Richard.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure
2021-06-28 3:30 [LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure Leo Liang
2021-06-28 7:24 ` Richard Palethorpe
@ 2021-06-28 7:36 ` Joerg Vehlow
2021-06-28 8:49 ` Petr Vorel
2021-06-28 13:08 ` Cyril Hrubis
2021-06-29 7:01 ` xuyang2018.jy
2 siblings, 2 replies; 9+ messages in thread
From: Joerg Vehlow @ 2021-06-28 7:36 UTC (permalink / raw)
To: ltp
Hi,
On 6/28/2021 5:30 AM, Leo Liang wrote:
> From a151d48235629a125d5db57dd76c96fd951d5293 Mon Sep 17 00:00:00 2001
> From: Leo Yu-Chi Liang <ycliang@andestech.com>
> Date: Mon, 28 Jun 2021 11:05:54 +0800
> Subject: [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure
>
> The test sequence
> mount -t cgroup -o <controllers> <path>
> mkdir <path>/<dir>
> rmdir <path>/<dir>
> umount <path>
> mount -t cgroup -o <controllers> <path>
> would easily fail at the last mount with -EBUSY.
>
> The reason is that this test sequence have the chance of
> missing a release code path when doing rmdir and umount.
>
> Add sync between every "rmdir, umount" pair to fix the problem.
>
> Fixes: #839
>
> Signed-off-by: Leo Yu-Chi Liang <ycliang@andestech.com>
> ---
> .../kernel/controllers/cgroup/cgroup_regression_test.sh | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh
> index 1f7f3820e..9a00df101 100755
> --- a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh
> +++ b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh
> @@ -145,6 +145,7 @@ test2()
> fi
>
> rmdir cgroup/0 cgroup/1
> + sync
> umount cgroup/
> }
>
> @@ -193,6 +194,7 @@ test3()
> wait $pid2 2>/dev/null
>
> rmdir $cpu_subsys_path/0 2> /dev/null
> + sync
> umount cgroup/ 2> /dev/null
> check_kernel_bug
> }
> @@ -222,6 +224,7 @@ test4()
> mount -t cgroup -o none,name=foo cgroup cgroup/
> mkdir cgroup/0
> rmdir cgroup/0
> + sync
> umount cgroup/
>
> if dmesg | grep -q "MAX_LOCKDEP_SUBCLASSES too low"; then
> @@ -254,6 +257,7 @@ test5()
> mount -t cgroup none cgroup 2> /dev/null
> mkdir cgroup/0
> rmdir cgroup/0
> + sync
> umount cgroup/ 2> /dev/null
> check_kernel_bug
> }
> @@ -290,6 +294,7 @@ test6()
>
> mount -t cgroup -o ns xxx cgroup/ > /dev/null 2>&1
> rmdir cgroup/[1-9]* > /dev/null 2>&1
> + sync
> umount cgroup/
> check_kernel_bug
> }
> @@ -324,6 +329,7 @@ test_7_1()
> mkdir $subsys_path/0
> sleep 100 < $subsys_path/0 & # add refcnt to this dir
> rmdir $subsys_path/0
> + sync
>
> # remount with new subsystems added
> # since 2.6.28, this remount will fail
> @@ -349,6 +355,7 @@ test_7_2()
> mkdir cgroup/0
> sleep 100 < cgroup/0 & # add refcnt to this dir
> rmdir cgroup/0
> + sync
>
> # remount with some subsystems removed
> # since 2.6.28, this remount will fail
I would like a short comment close to the syncs. When I converted
cpuset_regression_test.sh, I would have removed the sync in there, if
there wouldn't have been any comment.
Most of the time syncs are not required and just added by paranoid
developers, but if there is a real reason, I think it should be stated
in a comment.
J?rg
^ permalink raw reply [flat|nested] 9+ messages in thread
* [LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure
2021-06-28 7:36 ` Joerg Vehlow
@ 2021-06-28 8:49 ` Petr Vorel
2021-06-28 13:08 ` Cyril Hrubis
1 sibling, 0 replies; 9+ messages in thread
From: Petr Vorel @ 2021-06-28 8:49 UTC (permalink / raw)
To: ltp
Hi all,
[ changed Cyril's address in Cc to the working one ]
> > mkdir cgroup/0
> > sleep 100 < cgroup/0 & # add refcnt to this dir
> > rmdir cgroup/0
> > + sync
> > # remount with some subsystems removed
> > # since 2.6.28, this remount will fail
> I would like a short comment close to the syncs. When I converted
> cpuset_regression_test.sh, I would have removed the sync in there, if there
> wouldn't have been any comment.
> Most of the time syncs are not required and just added by paranoid
> developers, but if there is a real reason, I think it should be stated in a
> comment.
Agree with this. Are all these sync really needed? Or just some?
Kind regards,
Petr
> J?rg
^ permalink raw reply [flat|nested] 9+ messages in thread
* [LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure
2021-06-28 7:36 ` Joerg Vehlow
2021-06-28 8:49 ` Petr Vorel
@ 2021-06-28 13:08 ` Cyril Hrubis
2021-07-06 3:27 ` Leo Liang
1 sibling, 1 reply; 9+ messages in thread
From: Cyril Hrubis @ 2021-06-28 13:08 UTC (permalink / raw)
To: ltp
Hi!
> I would like a short comment close to the syncs. When I converted
> cpuset_regression_test.sh, I would have removed the sync in there, if
> there wouldn't have been any comment.
> Most of the time syncs are not required and just added by paranoid
> developers, but if there is a real reason, I think it should be stated
> in a comment.
Sounds reasonable to me, can we please add a comment there?
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 9+ messages in thread
* [LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure
2021-06-28 13:08 ` Cyril Hrubis
@ 2021-07-06 3:27 ` Leo Liang
2021-07-06 5:45 ` xuyang2018.jy
0 siblings, 1 reply; 9+ messages in thread
From: Leo Liang @ 2021-07-06 3:27 UTC (permalink / raw)
To: ltp
Hi,
Sorry for the late response and thanks for all the replies and suggestions.
I am running on a rather new RISC-V platform (Andes/AE350) and with 5.4.0 off-tree kernel.
The umount in cgroup_regression_test mostly failed at test_2 and test_3,
so it would show FAIL result (mount not successfully executed) at test_3 and test_5 (test_4 shows TCONF on my platform).
On Mon, Jun 28, 2021 at 03:08:39PM +0200, Cyril Hrubis wrote:
> Hi!
> > I would like a short comment close to the syncs. When I converted
> > cpuset_regression_test.sh, I would have removed the sync in there, if
> > there wouldn't have been any comment.
> > Most of the time syncs are not required and just added by paranoid
> > developers, but if there is a real reason, I think it should be stated
> > in a comment.
>
> Sounds reasonable to me, can we please add a comment there?
Hi Cyril and Joerg,
Sounds reasonable to me as well,
will send a v2 patch with comment.
> --
> Cyril Hrubis
> chrubis@suse.cz
> Agree with this. Are all these sync really needed? Or just some?
Hi Petr,
I've written a script containing only the following sequence
" mount 'cgroup mntpoint' "
" mkdir 'under cgroup mntpoint' "
" rmdir 'under cgroup mntpoint' "
" umount 'cgroup mntpoint' "
" mount 'cgroup mntpoint' "
and it could trigger the fail.
But only bumped into the umount fail when doing test_2 and test_3 in the cgroup_regression_test.
I am adding syncs in every sub-tests that execute the above sequence now.
Should them be added only at the places where umount failure did occur ?
> Kind regards,
> Petr
> IMO, Even we call sync, this umount may fail because sync ensures
> nothing. Why not use tst_umount?
Hi Yang,
I think this might be a timing issue and a little delay could fix this problem. (e.g. 'sleep 1')
Using 'sync' here IMHO would be more descriptive and has a semantic meaning.
Speaking of tst_umount, do you mean to convert this test to C code ?
> Best Regards
> Yang Xu
Best regards,
Leo
^ permalink raw reply [flat|nested] 9+ messages in thread
* [LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure
2021-07-06 3:27 ` Leo Liang
@ 2021-07-06 5:45 ` xuyang2018.jy
2021-07-12 7:28 ` Leo Liang
0 siblings, 1 reply; 9+ messages in thread
From: xuyang2018.jy @ 2021-07-06 5:45 UTC (permalink / raw)
To: ltp
Hi Leo
> Hi,
>
> Sorry for the late response and thanks for all the replies and suggestions.
>
> I am running on a rather new RISC-V platform (Andes/AE350) and with 5.4.0 off-tree kernel.
> The umount in cgroup_regression_test mostly failed at test_2 and test_3,
> so it would show FAIL result (mount not successfully executed) at test_3 and test_5 (test_4 shows TCONF on my platform).
>
> On Mon, Jun 28, 2021 at 03:08:39PM +0200, Cyril Hrubis wrote:
>> Hi!
>>> I would like a short comment close to the syncs. When I converted
>>> cpuset_regression_test.sh, I would have removed the sync in there, if
>>> there wouldn't have been any comment.
>>> Most of the time syncs are not required and just added by paranoid
>>> developers, but if there is a real reason, I think it should be stated
>>> in a comment.
>>
>> Sounds reasonable to me, can we please add a comment there?
>
> Hi Cyril and Joerg,
>
> Sounds reasonable to me as well,
> will send a v2 patch with comment.
>
>> --
>> Cyril Hrubis
>> chrubis@suse.cz
>
>
>> Agree with this. Are all these sync really needed? Or just some?
>
> Hi Petr,
>
> I've written a script containing only the following sequence
> " mount 'cgroup mntpoint' "
> " mkdir 'under cgroup mntpoint' "
> " rmdir 'under cgroup mntpoint' "
> " umount 'cgroup mntpoint' "
> " mount 'cgroup mntpoint' "
> and it could trigger the fail.
>
> But only bumped into the umount fail when doing test_2 and test_3 in the cgroup_regression_test.
>
> I am adding syncs in every sub-tests that execute the above sequence now.
> Should them be added only at the places where umount failure did occur ?
>
>> Kind regards,
>> Petr
>
>
>> IMO, Even we call sync, this umount may fail because sync ensures
>> nothing. Why not use tst_umount?
>
> Hi Yang,
>
> I think this might be a timing issue and a little delay could fix this problem. (e.g. 'sleep 1')
> Using 'sync' here IMHO would be more descriptive and has a semantic meaning.
Yes, it is a timing issue.
I also met a similar problem because of sync to lead EBUSY error in
xfstests log time ago.
https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=b536de2a042484bb241cca120ce55c974309513a
So, I don't think using sync is a good idea because sync will make
metadata into disk but no ensure it. So if we have other io work, then
sync may push other's metadata into disk firstly instead of here's data.
Since we have tst_umount api to avoid EBUSY error, why not to use it in
here to avoid your problem?
>
> Speaking of tst_umount, do you mean to convert this test to C code ?
No, we also have shell api for tst_umount.
>
>> Best Regards
>> Yang Xu
>
> Best regards,
> Leo
^ permalink raw reply [flat|nested] 9+ messages in thread
* [LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure
2021-07-06 5:45 ` xuyang2018.jy
@ 2021-07-12 7:28 ` Leo Liang
0 siblings, 0 replies; 9+ messages in thread
From: Leo Liang @ 2021-07-12 7:28 UTC (permalink / raw)
To: ltp
Hi Yang,
On Tue, Jul 06, 2021 at 05:45:32AM +0000, xuyang2018.jy@fujitsu.com wrote:
> Hi Leo
> >
> >> IMO, Even we call sync, this umount may fail because sync ensures
> >> nothing. Why not use tst_umount?
> >
> > Hi Yang,
> >
> > I think this might be a timing issue and a little delay could fix this problem. (e.g. 'sleep 1')
> > Using 'sync' here IMHO would be more descriptive and has a semantic meaning.
> Yes, it is a timing issue.
> I also met a similar problem because of sync to lead EBUSY error in
> xfstests log time ago.
>
> https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=b536de2a042484bb241cca120ce55c974309513a
>
> So, I don't think using sync is a good idea because sync will make
> metadata into disk but no ensure it. So if we have other io work, then
> sync may push other's metadata into disk firstly instead of here's data.
>
> Since we have tst_umount api to avoid EBUSY error, why not to use it in
> here to avoid your problem?
> >
> > Speaking of tst_umount, do you mean to convert this test to C code ?
> No, we also have shell api for tst_umount.
Thanks for the suggestion.
I've tested the cgroup_regression_test with the tst_umount api
and it works fine!
I will convert "add sync" patch to this!
Thanks again.
> >
> >> Best Regards
> >> Yang Xu
> >
> > Best regards,
> > Leo
Best regards,
Leo
^ permalink raw reply [flat|nested] 9+ messages in thread
* [LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure
2021-06-28 3:30 [LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure Leo Liang
2021-06-28 7:24 ` Richard Palethorpe
2021-06-28 7:36 ` Joerg Vehlow
@ 2021-06-29 7:01 ` xuyang2018.jy
2 siblings, 0 replies; 9+ messages in thread
From: xuyang2018.jy @ 2021-06-29 7:01 UTC (permalink / raw)
To: ltp
Hi
IMO, Even we call sync, this umount may fail because sync ensures
nothing. Why not use tst_umount?
Best Regards
Yang Xu
> From a151d48235629a125d5db57dd76c96fd951d5293 Mon Sep 17 00:00:00 2001
> From: Leo Yu-Chi Liang<ycliang@andestech.com>
> Date: Mon, 28 Jun 2021 11:05:54 +0800
> Subject: [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure
>
> The test sequence
> mount -t cgroup -o<controllers> <path>
> mkdir<path>/<dir>
> rmdir<path>/<dir>
> umount<path>
> mount -t cgroup -o<controllers> <path>
> would easily fail at the last mount with -EBUSY.
>
> The reason is that this test sequence have the chance of
> missing a release code path when doing rmdir and umount.
>
> Add sync between every "rmdir, umount" pair to fix the problem.
>
> Fixes: #839
>
> Signed-off-by: Leo Yu-Chi Liang<ycliang@andestech.com>
> ---
> .../kernel/controllers/cgroup/cgroup_regression_test.sh | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh
> index 1f7f3820e..9a00df101 100755
> --- a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh
> +++ b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh
> @@ -145,6 +145,7 @@ test2()
> fi
>
> rmdir cgroup/0 cgroup/1
> + sync
> umount cgroup/
> }
>
> @@ -193,6 +194,7 @@ test3()
> wait $pid2 2>/dev/null
>
> rmdir $cpu_subsys_path/0 2> /dev/null
> + sync
> umount cgroup/ 2> /dev/null
> check_kernel_bug
> }
> @@ -222,6 +224,7 @@ test4()
> mount -t cgroup -o none,name=foo cgroup cgroup/
> mkdir cgroup/0
> rmdir cgroup/0
> + sync
> umount cgroup/
>
> if dmesg | grep -q "MAX_LOCKDEP_SUBCLASSES too low"; then
> @@ -254,6 +257,7 @@ test5()
> mount -t cgroup none cgroup 2> /dev/null
> mkdir cgroup/0
> rmdir cgroup/0
> + sync
> umount cgroup/ 2> /dev/null
> check_kernel_bug
> }
> @@ -290,6 +294,7 @@ test6()
>
> mount -t cgroup -o ns xxx cgroup/> /dev/null 2>&1
> rmdir cgroup/[1-9]*> /dev/null 2>&1
> + sync
> umount cgroup/
> check_kernel_bug
> }
> @@ -324,6 +329,7 @@ test_7_1()
> mkdir $subsys_path/0
> sleep 100< $subsys_path/0& # add refcnt to this dir
> rmdir $subsys_path/0
> + sync
>
> # remount with new subsystems added
> # since 2.6.28, this remount will fail
> @@ -349,6 +355,7 @@ test_7_2()
> mkdir cgroup/0
> sleep 100< cgroup/0& # add refcnt to this dir
> rmdir cgroup/0
> + sync
>
> # remount with some subsystems removed
> # since 2.6.28, this remount will fail
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-07-12 7:28 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-28 3:30 [LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure Leo Liang
2021-06-28 7:24 ` Richard Palethorpe
2021-06-28 7:36 ` Joerg Vehlow
2021-06-28 8:49 ` Petr Vorel
2021-06-28 13:08 ` Cyril Hrubis
2021-07-06 3:27 ` Leo Liang
2021-07-06 5:45 ` xuyang2018.jy
2021-07-12 7:28 ` Leo Liang
2021-06-29 7:01 ` xuyang2018.jy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.