[LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure

[LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure

[Leo Liang]

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

[LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure

[Richard Palethorpe]

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.

[LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure

[Joerg Vehlow]

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

[LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure

[Petr Vorel]

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

[LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure

[Cyril Hrubis]

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

[LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure

[xuyang2018.jy]

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

[LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure

[Leo Liang]

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

[LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure

[xuyang2018.jy]

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

[LTP] [PATCH 1/1] cgroup/cgroup_regression_test: Fix umount failure

[Leo Liang]

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