| Message ID | 1560250815-2308-1-git-send-email-xuyang2018.jy@cn.fujitsu.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cgroup_regression_test.sh: fix test_5 possible mount failure because of cgroup hierarchy | expand |
Hi Ping. :-) > Currently, if systems doesn't mount subsys1,subsys2 and the hierarchy is not equal to 0, running it > reports the following error: > > mount: xxx is already mounted or /tmp/ltp-wPw08anmTI/LTP_cgroup_regression_test.V4jf0qrS7z/cgroup busy > cgroup_regression_test 5 TFAIL: mount net_prio and pids failed > > It fails because libcgroup doesn't permmit destroy cgroup subsystem hierarchies. > Simple umnout does not destroy the hierarchies. They still live inside kernel! > > When hierarchy is equal to 0 in /proc/cgroups, we can mount them together on > a new mountpoint. > > I add a check for subsystem hierarchy and get subsystem from head. > > Notice: > more information about"Bug 612805 - cgroup: mount: none already mounted or /cgroups busy" > > Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> > --- > .../cgroup/cgroup_regression_test.sh | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh > index e197f5d3f..38cb760c2 100755 > --- a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh > +++ b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh > @@ -252,8 +252,10 @@ test5() > return > fi > > - local subsys1=`tail -n 1 /proc/cgroups | awk '{ print $1 }'` > - local subsys2=`tail -n 2 /proc/cgroups | head -1 | awk '{ print $1 }'` > + local subsys1=`head -2 /proc/cgroups | tail -n 1 | awk '{ print $1 }'` > + local subsys1_hierarchy=`head -2 /proc/cgroups | tail -n 1 | awk '{ print $2 }'` > + local subsys2=`head -3 /proc/cgroups | tail -n 1 | awk '{ print $1 }'` > + local subsys2_hierarchy=`head -3 /proc/cgroups | tail -n 1 | awk '{ print $2 }'` > > # Accounting here for the fact that the chosen subsystems could > # have been already previously mounted at boot time: in such a > @@ -267,10 +269,16 @@ test5() > if [ -z "$mounted" ]; then > mntpoint=cgroup > failing=$subsys1 > - mount -t cgroup -o $subsys1,$subsys2 xxx $mntpoint/ > + mount -t cgroup -o $subsys1,$subsys2 xxx $mntpoint/ 2>/dev/null > + # Even subsystem has not been mounted, it still live in kernel. > + # So we will get EBUSY when both mount subsys1 and subsys2 if > + # hierarchy isn't equal to 0. > if [ $? -ne 0 ]; then > - tst_res TFAIL "mount $subsys1 and $subsys2 failed" > - return > + if [ "$subsys1_hierarchy" = 0 -a "$subsys2_hierarchy" = 0 ]; then > + tst_res TFAIL "mount $subsys1 and $subsys2 failed" > + return > + fi > + failing=$subsys1,$subsys2 > fi > else > # Use the pre-esistent mountpoint as $mntpoint and use a
Hi Ping. :-) > Hi > > Ping. :-) > >> Currently, if systems doesn't mount subsys1,subsys2 and the hierarchy is not equal to 0, running it >> reports the following error: >> >> mount: xxx is already mounted or /tmp/ltp-wPw08anmTI/LTP_cgroup_regression_test.V4jf0qrS7z/cgroup busy >> cgroup_regression_test 5 TFAIL: mount net_prio and pids failed >> >> It fails because libcgroup doesn't permmit destroy cgroup subsystem hierarchies. >> Simple umnout does not destroy the hierarchies. They still live inside kernel! >> >> When hierarchy is equal to 0 in /proc/cgroups, we can mount them together on >> a new mountpoint. >> >> I add a check for subsystem hierarchy and get subsystem from head. >> >> Notice: >> more information about"Bug 612805 - cgroup: mount: none already mounted or /cgroups busy" >> >> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> >> --- >> .../cgroup/cgroup_regression_test.sh | 18 +++++++++++++----- >> 1 file changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh >> index e197f5d3f..38cb760c2 100755 >> --- a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh >> +++ b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh >> @@ -252,8 +252,10 @@ test5() >> return >> fi >> >> - local subsys1=`tail -n 1 /proc/cgroups | awk '{ print $1 }'` >> - local subsys2=`tail -n 2 /proc/cgroups | head -1 | awk '{ print $1 }'` >> + local subsys1=`head -2 /proc/cgroups | tail -n 1 | awk '{ print $1 }'` >> + local subsys1_hierarchy=`head -2 /proc/cgroups | tail -n 1 | awk '{ print $2 }'` >> + local subsys2=`head -3 /proc/cgroups | tail -n 1 | awk '{ print $1 }'` >> + local subsys2_hierarchy=`head -3 /proc/cgroups | tail -n 1 | awk '{ print $2 }'` >> >> # Accounting here for the fact that the chosen subsystems could >> # have been already previously mounted at boot time: in such a >> @@ -267,10 +269,16 @@ test5() >> if [ -z "$mounted" ]; then >> mntpoint=cgroup >> failing=$subsys1 >> - mount -t cgroup -o $subsys1,$subsys2 xxx $mntpoint/ >> + mount -t cgroup -o $subsys1,$subsys2 xxx $mntpoint/ 2>/dev/null >> + # Even subsystem has not been mounted, it still live in kernel. >> + # So we will get EBUSY when both mount subsys1 and subsys2 if >> + # hierarchy isn't equal to 0. >> if [ $? -ne 0 ]; then >> - tst_res TFAIL "mount $subsys1 and $subsys2 failed" >> - return >> + if [ "$subsys1_hierarchy" = 0 -a "$subsys2_hierarchy" = 0 ]; then >> + tst_res TFAIL "mount $subsys1 and $subsys2 failed" >> + return >> + fi >> + failing=$subsys1,$subsys2 >> fi >> else >> # Use the pre-esistent mountpoint as $mntpoint and use a
Hi Have somebody noticed this patch? ping.:-) > Hi > Ping. :-) >> Hi >> >> Ping. :-) >> >>> Currently, if systems doesn't mount subsys1,subsys2 and the hierarchy is not equal to 0, running it >>> reports the following error: >>> >>> mount: xxx is already mounted or /tmp/ltp-wPw08anmTI/LTP_cgroup_regression_test.V4jf0qrS7z/cgroup busy >>> cgroup_regression_test 5 TFAIL: mount net_prio and pids failed >>> >>> It fails because libcgroup doesn't permmit destroy cgroup subsystem hierarchies. >>> Simple umnout does not destroy the hierarchies. They still live inside kernel! >>> >>> When hierarchy is equal to 0 in /proc/cgroups, we can mount them together on >>> a new mountpoint. >>> >>> I add a check for subsystem hierarchy and get subsystem from head. >>> >>> Notice: >>> more information about"Bug 612805 - cgroup: mount: none already mounted or /cgroups busy" >>> >>> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> >>> --- >>> .../cgroup/cgroup_regression_test.sh | 18 +++++++++++++----- >>> 1 file changed, 13 insertions(+), 5 deletions(-) >>> >>> diff --git a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh >>> index e197f5d3f..38cb760c2 100755 >>> --- a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh >>> +++ b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh >>> @@ -252,8 +252,10 @@ test5() >>> return >>> fi >>> >>> - local subsys1=`tail -n 1 /proc/cgroups | awk '{ print $1 }'` >>> - local subsys2=`tail -n 2 /proc/cgroups | head -1 | awk '{ print $1 }'` >>> + local subsys1=`head -2 /proc/cgroups | tail -n 1 | awk '{ print $1 }'` >>> + local subsys1_hierarchy=`head -2 /proc/cgroups | tail -n 1 | awk '{ print $2 }'` >>> + local subsys2=`head -3 /proc/cgroups | tail -n 1 | awk '{ print $1 }'` >>> + local subsys2_hierarchy=`head -3 /proc/cgroups | tail -n 1 | awk '{ print $2 }'` >>> >>> # Accounting here for the fact that the chosen subsystems could >>> # have been already previously mounted at boot time: in such a >>> @@ -267,10 +269,16 @@ test5() >>> if [ -z "$mounted" ]; then >>> mntpoint=cgroup >>> failing=$subsys1 >>> - mount -t cgroup -o $subsys1,$subsys2 xxx $mntpoint/ >>> + mount -t cgroup -o $subsys1,$subsys2 xxx $mntpoint/ 2>/dev/null >>> + # Even subsystem has not been mounted, it still live in kernel. >>> + # So we will get EBUSY when both mount subsys1 and subsys2 if >>> + # hierarchy isn't equal to 0. >>> if [ $? -ne 0 ]; then >>> - tst_res TFAIL "mount $subsys1 and $subsys2 failed" >>> - return >>> + if [ "$subsys1_hierarchy" = 0 -a "$subsys2_hierarchy" = 0 ]; then >>> + tst_res TFAIL "mount $subsys1 and $subsys2 failed" >>> + return >>> + fi >>> + failing=$subsys1,$subsys2 >>> fi >>> else >>> # Use the pre-esistent mountpoint as $mntpoint and use a
Hi!
> Have somebody noticed this patch? ping.:-)
Sorry for the long delay.
I'm looking at the original reproducer at:
https://lists.openvz.org/pipermail/devel/2009-January/016345.html
And as far as I can tell the test_5() was never actually doing what it
takes to reproduce the bug, as far as I can tell the test was bogus to
begin with. The main point of the reproducer is that the cgroup is
unmounted while there is task in the group, then remounted again. As we
cannot unmount the cgroup these days I would just go for removing the
test instead of applying band aid over the code.
on 2019/09/11 21:33, Cyril Hrubis wrote: > Hi! >> Have somebody noticed this patch? ping.:-) > Sorry for the long delay. > > I'm looking at the original reproducer at: > > https://lists.openvz.org/pipermail/devel/2009-January/016345.html > > And as far as I can tell the test_5() was never actually doing what it > takes to reproduce the bug, as far as I can tell the test was bogus to > begin with. The main point of the reproducer is that the cgroup is > unmounted while there is task in the group, then remounted again. As we > cannot unmount the cgroup these days I would just go for removing the > test instead of applying band aid over the code. Hi Cyril why we can't unmount the cgroup these days? From kernel commit 839ec545("cgroup: fix root_count when mount fails due to busy subsystem"), it should be reproduced as the following step 1)mount two subsystem(A and B) mntpoint 2)mount one subsystem(A) mntpoint, it will get EBUSY error 3)without kernel commit, kill this process and task is still in cgroup, kernel will call cgroup_kill_sb() to decrement root_count, then kernel crashes. Is it right? Thanks Yang Xu
Hi! > > I'm looking at the original reproducer at: > > > > https://lists.openvz.org/pipermail/devel/2009-January/016345.html > > > > And as far as I can tell the test_5() was never actually doing what it > > takes to reproduce the bug, as far as I can tell the test was bogus to > > begin with. The main point of the reproducer is that the cgroup is > > unmounted while there is task in the group, then remounted again. As we > > cannot unmount the cgroup these days I would just go for removing the > > test instead of applying band aid over the code. > > Hi Cyril > > why we can't unmount the cgroup these days? It's a bit more complicated, you can't decide on which controllers to put into your hierarchy as it's mounted already (by a systemd). You can mount them exactly the way systemd mounts them (a few controllers are put into combined hierarchies but most of them are separated) but to a different mount point. Also once controller is in use in v2 it cannot be used by v1, which is going to be problem soon. As we are in transition period between v1 and v2 doing anything portable with cgroups is going to be nightmare. > From kernel commit 839ec545("cgroup: fix root_count when mount fails due to busy subsystem"), > it should be reproduced as the following step > 1)mount two subsystem(A and B) mntpoint > 2)mount one subsystem(A) mntpoint, it will get EBUSY error > 3)without kernel commit, kill this process and task is still in cgroup, kernel will call cgroup_kill_sb() > to decrement root_count, then kernel crashes. > > Is it right? This does not seem to match the original reproducer but it may be another way how to reproduce the bug. Also I'm not sure that this reproducer makes sense, since the code in kernel has been rewritten completely since the 2.6 days. Generally I would say that we may need completely new tests for cgroups, but I doubt that we should make much effort for the v1 anyways. In the v2 you get all controllers in an unified hierachy and you can't mount them in a different way. If the only point is to get EBUSY on mount, then have a process exit while in the cgroup we should as well simplify the test. There is no point in mounting the subsystems together in the first step, as a matter of fact on modern distributions the test just checks that the two subsystems are mounted already then attempts to mount them combined, which fails. Why can't we mount the two controllers seperatelly in the case that nothing is mounted as well?
on 2019/09/12 20:29, Cyril Hrubis wrote: > Hi! >>> I'm looking at the original reproducer at: >>> >>> https://lists.openvz.org/pipermail/devel/2009-January/016345.html >>> >>> And as far as I can tell the test_5() was never actually doing what it >>> takes to reproduce the bug, as far as I can tell the test was bogus to >>> begin with. The main point of the reproducer is that the cgroup is >>> unmounted while there is task in the group, then remounted again. As we >>> cannot unmount the cgroup these days I would just go for removing the >>> test instead of applying band aid over the code. >> Hi Cyril >> >> why we can't unmount the cgroup these days? > It's a bit more complicated, you can't decide on which controllers to > put into your hierarchy as it's mounted already (by a systemd). You can > mount them exactly the way systemd mounts them (a few controllers are > put into combined hierarchies but most of them are separated) but to a > different mount point. Also once controller is in use in v2 it cannot be > used by v1, which is going to be problem soon. As we are in transition > period between v1 and v2 doing anything portable with cgroups is going > to be nightmare. Thanks. I understand it.. > >> From kernel commit 839ec545("cgroup: fix root_count when mount fails due to busy subsystem"), >> it should be reproduced as the following step >> 1)mount two subsystem(A and B) mntpoint >> 2)mount one subsystem(A) mntpoint, it will get EBUSY error >> 3)without kernel commit, kill this process and task is still in cgroup, kernel will call cgroup_kill_sb() >> to decrement root_count, then kernel crashes. >> >> Is it right? > This does not seem to match the original reproducer but it may be > another way how to reproduce the bug. Also I'm not sure that this > reproducer makes sense, since the code in kernel has been rewritten > completely since the 2.6 days. Generally I would say that we may need > completely new tests for cgroups, but I doubt that we should make much > effort for the v1 anyways. In the v2 you get all controllers in an > unified hierachy and you can't mount them in a different way. Yes. cgroup v2 has only a single hierarchy, not promise to mount them in a different way. And test_5 is a very old regresstion test and kernel code has been rewritten completely since 2.6. No user will use such old kernel code to test. I agree with you that we should remove this test_5. > > If the only point is to get EBUSY on mount, then have a process exit > while in the cgroup we should as well simplify the test. There is no > point in mounting the subsystems together in the first step, as a matter > of fact on modern distributions the test just checks that the two > subsystems are mounted already then attempts to mount them combined, > which fails. Why can't we mount the two controllers seperatelly in the > case that nothing is mounted as well. It sounds reasonable, mount them seperatelly in the case should also be a right way to reproduce this bug. But I run this test_5 and I nerver meet this crash on my machines with cgroup v1 (kernel with 2.6.32 3.10 and 4.18 kernel). I think this test is too old that we shoud remove it. >
diff --git a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh index e197f5d3f..38cb760c2 100755 --- a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh +++ b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh @@ -252,8 +252,10 @@ test5() return fi - local subsys1=`tail -n 1 /proc/cgroups | awk '{ print $1 }'` - local subsys2=`tail -n 2 /proc/cgroups | head -1 | awk '{ print $1 }'` + local subsys1=`head -2 /proc/cgroups | tail -n 1 | awk '{ print $1 }'` + local subsys1_hierarchy=`head -2 /proc/cgroups | tail -n 1 | awk '{ print $2 }'` + local subsys2=`head -3 /proc/cgroups | tail -n 1 | awk '{ print $1 }'` + local subsys2_hierarchy=`head -3 /proc/cgroups | tail -n 1 | awk '{ print $2 }'` # Accounting here for the fact that the chosen subsystems could # have been already previously mounted at boot time: in such a @@ -267,10 +269,16 @@ test5() if [ -z "$mounted" ]; then mntpoint=cgroup failing=$subsys1 - mount -t cgroup -o $subsys1,$subsys2 xxx $mntpoint/ + mount -t cgroup -o $subsys1,$subsys2 xxx $mntpoint/ 2>/dev/null + # Even subsystem has not been mounted, it still live in kernel. + # So we will get EBUSY when both mount subsys1 and subsys2 if + # hierarchy isn't equal to 0. if [ $? -ne 0 ]; then - tst_res TFAIL "mount $subsys1 and $subsys2 failed" - return + if [ "$subsys1_hierarchy" = 0 -a "$subsys2_hierarchy" = 0 ]; then + tst_res TFAIL "mount $subsys1 and $subsys2 failed" + return + fi + failing=$subsys1,$subsys2 fi else # Use the pre-esistent mountpoint as $mntpoint and use a
Currently, if systems doesn't mount subsys1,subsys2 and the hierarchy is not equal to 0, running it reports the following error: mount: xxx is already mounted or /tmp/ltp-wPw08anmTI/LTP_cgroup_regression_test.V4jf0qrS7z/cgroup busy cgroup_regression_test 5 TFAIL: mount net_prio and pids failed It fails because libcgroup doesn't permmit destroy cgroup subsystem hierarchies. Simple umnout does not destroy the hierarchies. They still live inside kernel! When hierarchy is equal to 0 in /proc/cgroups, we can mount them together on a new mountpoint. I add a check for subsystem hierarchy and get subsystem from head. Notice: more information about"Bug 612805 - cgroup: mount: none already mounted or /cgroups busy" Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> --- .../cgroup/cgroup_regression_test.sh | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)