| Message ID | 20210719092239.GA1475@atcfdc88 |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | cgroup/cgroup_regression_test: Fix umount failure | expand |
Hi Leo, Reviewed-by: Petr Vorel <pvorel@suse.cz> > rmdir cgroup/0 cgroup/1 > - umount cgroup/ > + tst_umount cgroup/ # Avoid possible EBUSY error > } > #--------------------------------------------------------------------------- > @@ -193,7 +193,7 @@ test3() > wait $pid2 2>/dev/null > rmdir $cpu_subsys_path/0 2> /dev/null > - umount cgroup/ 2> /dev/null > + tst_umount cgroup/ 2> /dev/null # Avoid possible EBUSY error I'd prefer: # keep "/" to avoid possible EBUSY error But that can be changed before merge. More I'm interested if other maintainers agree with me about this approach. (keep / here instead of in tst_umount()) > check_kernel_bug > } > @@ -222,7 +222,7 @@ test4() > mount -t cgroup -o none,name=foo cgroup cgroup/ > mkdir cgroup/0 > rmdir cgroup/0 > - umount cgroup/ > + tst_umount cgroup/ # Avoid possible EBUSY error > if dmesg | grep -q "MAX_LOCKDEP_SUBCLASSES too low"; then > tst_res TFAIL "lockdep BUG was found" > @@ -254,7 +254,7 @@ test5() > mount -t cgroup none cgroup 2> /dev/null > mkdir cgroup/0 > rmdir cgroup/0 > - umount cgroup/ 2> /dev/null > + tst_umount cgroup/ 2> /dev/null # Avoid possible EBUSY error I'd drop stderr redirection here. It was here originally, but I suppose it's not needed when using tst_umount. But that can be done during merge. ... Kind regards, Petr
Hi, On 7/21/2021 4:37 PM, Petr Vorel wrote: > Hi Leo, > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > >> rmdir cgroup/0 cgroup/1 >> - umount cgroup/ >> + tst_umount cgroup/ # Avoid possible EBUSY error >> } >> #--------------------------------------------------------------------------- >> @@ -193,7 +193,7 @@ test3() >> wait $pid2 2>/dev/null >> rmdir $cpu_subsys_path/0 2> /dev/null >> - umount cgroup/ 2> /dev/null >> + tst_umount cgroup/ 2> /dev/null # Avoid possible EBUSY error > I'd prefer: # keep "/" to avoid possible EBUSY error > But that can be changed before merge. > > More I'm interested if other maintainers agree with me about this approach. > (keep / here instead of in tst_umount()) I had a first look at this patches and was curious, what the reasoning behind the "/" is. The comment you suggest is wrong. The / was introduced to prevent unmounting some other mountpoint, where the device was cgroup. Imho the approach of adding a / to the end was wrong and intransparent. I would rather use "./cgroup" or "$PWD/cgroup". If possible, I'd actually change tst_umount, to always unmount the mountpoint and not the device, i.e. if the given path is not an absolute path, make it absolute (e.g. by prepending $PWD"). This way the check if the mountpoint exist wouldn't be the fuzzy thing it is right now. As for the comment ("# Avoid possible EBUSY error"): Honestly I'd drop it and like in the c-api make using tst_umount instead of plain umount the default, for the same reasons. >> if dmesg | grep -q "MAX_LOCKDEP_SUBCLASSES too low"; then >> tst_res TFAIL "lockdep BUG was found" >> @@ -254,7 +254,7 @@ test5() >> mount -t cgroup none cgroup 2> /dev/null >> mkdir cgroup/0 >> rmdir cgroup/0 >> - umount cgroup/ 2> /dev/null >> + tst_umount cgroup/ 2> /dev/null # Avoid possible EBUSY error > I'd drop stderr redirection here. It was here originally, but I suppose it's not > needed when using tst_umount. But that can be done during merge. +1 Joerg
Hi Petr, On Wed, Jul 21, 2021 at 10:37:27PM +0800, Petr Vorel wrote: > Hi Leo, > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > > rmdir cgroup/0 cgroup/1 > > - umount cgroup/ > > + tst_umount cgroup/ # Avoid possible EBUSY error > > } > > > #--------------------------------------------------------------------------- > > @@ -193,7 +193,7 @@ test3() > > wait $pid2 2>/dev/null > > > rmdir $cpu_subsys_path/0 2> /dev/null > > - umount cgroup/ 2> /dev/null > > + tst_umount cgroup/ 2> /dev/null # Avoid possible EBUSY error > I'd prefer: # keep "/" to avoid possible EBUSY error > But that can be changed before merge. > > More I'm interested if other maintainers agree with me about this approach. > (keep / here instead of in tst_umount()) > > > check_kernel_bug > > } > > > @@ -222,7 +222,7 @@ test4() > > mount -t cgroup -o none,name=foo cgroup cgroup/ > > mkdir cgroup/0 > > rmdir cgroup/0 > > - umount cgroup/ > > + tst_umount cgroup/ # Avoid possible EBUSY error > > > if dmesg | grep -q "MAX_LOCKDEP_SUBCLASSES too low"; then > > tst_res TFAIL "lockdep BUG was found" > > @@ -254,7 +254,7 @@ test5() > > mount -t cgroup none cgroup 2> /dev/null > > mkdir cgroup/0 > > rmdir cgroup/0 > > - umount cgroup/ 2> /dev/null > > + tst_umount cgroup/ 2> /dev/null # Avoid possible EBUSY error > I'd drop stderr redirection here. It was here originally, but I suppose it's not > needed when using tst_umount. But that can be done during merge. > OK got it. Thanks for the review! Let's wait for other maintainer's comment on the aaproach for keeping '/' here or not. Best regards, Leo > ... > > Kind regards, > Petr
Hi Joerg, On Thu, Jul 22, 2021 at 12:35:59PM +0800, Joerg Vehlow wrote: > Hi, > > On 7/21/2021 4:37 PM, Petr Vorel wrote: > > Hi Leo, > > > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > > >> rmdir cgroup/0 cgroup/1 > >> - umount cgroup/ > >> + tst_umount cgroup/ # Avoid possible EBUSY error > >> } > >> #--------------------------------------------------------------------------- > >> @@ -193,7 +193,7 @@ test3() > >> wait $pid2 2>/dev/null > >> rmdir $cpu_subsys_path/0 2> /dev/null > >> - umount cgroup/ 2> /dev/null > >> + tst_umount cgroup/ 2> /dev/null # Avoid possible EBUSY error > > I'd prefer: # keep "/" to avoid possible EBUSY error > > But that can be changed before merge. > > > > More I'm interested if other maintainers agree with me about this approach. > > (keep / here instead of in tst_umount()) > I had a first look at this patches and was curious, what the reasoning > behind the "/" is. > The comment you suggest is wrong. The / was introduced to prevent > unmounting some other mountpoint, > where the device was cgroup. > Imho the approach of adding a / to the end was wrong and intransparent. > I would rather use "./cgroup" or "$PWD/cgroup". > If possible, I'd actually change tst_umount, to always unmount the > mountpoint and not the device, i.e. if the given path is not an absolute > path, make it absolute (e.g. by prepending $PWD"). > This way the check if the mountpoint exist wouldn't be the fuzzy thing > it is right now. > > As for the comment ("# Avoid possible EBUSY error"): Honestly I'd drop > it and like in the c-api make using tst_umount instead of plain umount > the default, for the same reasons. Got it! Thanks for the detailed explanation! I will send a v5 patch for this modification. (making the path absolute inside tst_umount) > > >> if dmesg | grep -q "MAX_LOCKDEP_SUBCLASSES too low"; then > >> tst_res TFAIL "lockdep BUG was found" > >> @@ -254,7 +254,7 @@ test5() > >> mount -t cgroup none cgroup 2> /dev/null > >> mkdir cgroup/0 > >> rmdir cgroup/0 > >> - umount cgroup/ 2> /dev/null > >> + tst_umount cgroup/ 2> /dev/null # Avoid possible EBUSY error > > I'd drop stderr redirection here. It was here originally, but I suppose it's not > > needed when using tst_umount. But that can be done during merge. > +1 This change will be included in the next patch as well. Thanks again! Best regards, Leo > Joerg
Hi Leo, On 7/22/2021 8:32 AM, Leo Liang wrote: > Hi Joerg, > On Thu, Jul 22, 2021 at 12:35:59PM +0800, Joerg Vehlow wrote: >> Hi, >> >> On 7/21/2021 4:37 PM, Petr Vorel wrote: >>> Hi Leo, >>> >>> Reviewed-by: Petr Vorel <pvorel@suse.cz> >>> >>>> rmdir cgroup/0 cgroup/1 >>>> - umount cgroup/ >>>> + tst_umount cgroup/ # Avoid possible EBUSY error >>>> } >>>> #--------------------------------------------------------------------------- >>>> @@ -193,7 +193,7 @@ test3() >>>> wait $pid2 2>/dev/null >>>> rmdir $cpu_subsys_path/0 2> /dev/null >>>> - umount cgroup/ 2> /dev/null >>>> + tst_umount cgroup/ 2> /dev/null # Avoid possible EBUSY error >>> I'd prefer: # keep "/" to avoid possible EBUSY error >>> But that can be changed before merge. >>> >>> More I'm interested if other maintainers agree with me about this approach. >>> (keep / here instead of in tst_umount()) >> I had a first look at this patches and was curious, what the reasoning >> behind the "/" is. >> The comment you suggest is wrong. The / was introduced to prevent >> unmounting some other mountpoint, >> where the device was cgroup. >> Imho the approach of adding a / to the end was wrong and intransparent. >> I would rather use "./cgroup" or "$PWD/cgroup". >> If possible, I'd actually change tst_umount, to always unmount the >> mountpoint and not the device, i.e. if the given path is not an absolute >> path, make it absolute (e.g. by prepending $PWD"). >> This way the check if the mountpoint exist wouldn't be the fuzzy thing >> it is right now. >> >> As for the comment ("# Avoid possible EBUSY error"): Honestly I'd drop >> it and like in the c-api make using tst_umount instead of plain umount >> the default, for the same reasons. > Got it! > Thanks for the detailed explanation! > I will send a v5 patch for this modification. > (making the path absolute inside tst_umount) This was just my opinon. I am not in the place to say how it should be done. Maybe wait for replies from the maintainers. Additionally, all usages of tst_umount have to be checked, to ensure they are passing a mountpoint and not a device, otherwise my proposal cannot be implemented in tst_umount. Joerg
Hi Joerg, On Thu, Jul 22, 2021 at 02:37:23PM +0800, Joerg Vehlow wrote: > Hi Leo, > > > On 7/22/2021 8:32 AM, Leo Liang wrote: > > Hi Joerg, > > On Thu, Jul 22, 2021 at 12:35:59PM +0800, Joerg Vehlow wrote: > >> Hi, > >> > >> On 7/21/2021 4:37 PM, Petr Vorel wrote: > >>> Hi Leo, > >>> > >>> Reviewed-by: Petr Vorel <pvorel@suse.cz> > >>> > >>>> rmdir cgroup/0 cgroup/1 > >>>> - umount cgroup/ > >>>> + tst_umount cgroup/ # Avoid possible EBUSY error > >>>> } > >>>> #--------------------------------------------------------------------------- > >>>> @@ -193,7 +193,7 @@ test3() > >>>> wait $pid2 2>/dev/null > >>>> rmdir $cpu_subsys_path/0 2> /dev/null > >>>> - umount cgroup/ 2> /dev/null > >>>> + tst_umount cgroup/ 2> /dev/null # Avoid possible EBUSY error > >>> I'd prefer: # keep "/" to avoid possible EBUSY error > >>> But that can be changed before merge. > >>> > >>> More I'm interested if other maintainers agree with me about this approach. > >>> (keep / here instead of in tst_umount()) > >> I had a first look at this patches and was curious, what the reasoning > >> behind the "/" is. > >> The comment you suggest is wrong. The / was introduced to prevent > >> unmounting some other mountpoint, > >> where the device was cgroup. > >> Imho the approach of adding a / to the end was wrong and intransparent. > >> I would rather use "./cgroup" or "$PWD/cgroup". > >> If possible, I'd actually change tst_umount, to always unmount the > >> mountpoint and not the device, i.e. if the given path is not an absolute > >> path, make it absolute (e.g. by prepending $PWD"). > >> This way the check if the mountpoint exist wouldn't be the fuzzy thing > >> it is right now. > >> > >> As for the comment ("# Avoid possible EBUSY error"): Honestly I'd drop > >> it and like in the c-api make using tst_umount instead of plain umount > >> the default, for the same reasons. > > Got it! > > Thanks for the detailed explanation! > > I will send a v5 patch for this modification. > > (making the path absolute inside tst_umount) > This was just my opinon. I am not in the place to say how it should be done. > Maybe wait for replies from the maintainers. > Additionally, all usages of tst_umount have to be checked, to ensure > they are passing a mountpoint and not a device, otherwise my proposal > cannot be implemented in tst_umount. > Understood! Thanks for the suggestion. I think I will send a v5 that stays with the original change for this patchset. Then send a new RFC patchset to implement your suggestion and check for all uses of this API. Best regards, Leo > Joerg
Hi! > I had a first look at this patches and was curious, what the reasoning > behind the "/" is. > The comment you suggest is wrong. The / was introduced to prevent > unmounting some other mountpoint, > where the device was cgroup. > Imho the approach of adding a / to the end was wrong and intransparent. > I would rather use "./cgroup" or "$PWD/cgroup". Passing full path to the cgroup directory sound much safer to me especially when the directory name is just 'cgroup', try it yourself: device=cgroup/; grep "${device%/}" /proc/mounts On my machine this yields 10 lines and 21 matches. > If possible, I'd actually change tst_umount, to always unmount the > mountpoint and not the device, i.e. if the given path is not an absolute > path, make it absolute (e.g. by prepending $PWD"). > This way the check if the mountpoint exist wouldn't be the fuzzy thing > it is right now. Strongly agree here. I would go even one step further and change the library so that it rejects anything that does not start with '/'.
Hi, FYI this discussion is on v4, there is already v5 (marking it changes requested in patchwork) and obviously v6 will be needed. Leo, I suppose you'll implement everything mentioned here in v6. > Hi! > > I had a first look at this patches and was curious, what the reasoning > > behind the "/" is. +1 I should have ask myself as well :). > > The comment you suggest is wrong. The / was introduced to prevent > > unmounting some other mountpoint, > > where the device was cgroup. > > Imho the approach of adding a / to the end was wrong and intransparent. > > I would rather use "./cgroup" or "$PWD/cgroup". > Passing full path to the cgroup directory sound much safer to me > especially when the directory name is just 'cgroup', try it yourself: > device=cgroup/; grep "${device%/}" /proc/mounts > On my machine this yields 10 lines and 21 matches. > > If possible, I'd actually change tst_umount, to always unmount the > > mountpoint and not the device, i.e. if the given path is not an absolute > > path, make it absolute (e.g. by prepending $PWD"). > > This way the check if the mountpoint exist wouldn't be the fuzzy thing > > it is right now. +1 > Strongly agree here. > I would go even one step further and change the library so that it > rejects anything that does not start with '/'. +1 Kind regards, Petr
Hi, On Tue, Jul 27, 2021 at 09:53:54PM +0800, Petr Vorel wrote: > Hi, > > FYI this discussion is on v4, there is already v5 (marking it changes requested > in patchwork) and obviously v6 will be needed. Leo, I suppose you'll implement > everything mentioned here in v6. > No problem! Thanks for all the advice! Will prepare a v6 soon! > > Hi! > > > I had a first look at this patches and was curious, what the reasoning > > > behind the "/" is. > +1 I should have ask myself as well :). > > > > The comment you suggest is wrong. The / was introduced to prevent > > > unmounting some other mountpoint, > > > where the device was cgroup. > > > Imho the approach of adding a / to the end was wrong and intransparent. > > > I would rather use "./cgroup" or "$PWD/cgroup". > > > Passing full path to the cgroup directory sound much safer to me > > especially when the directory name is just 'cgroup', try it yourself: > > > device=cgroup/; grep "${device%/}" /proc/mounts > > > On my machine this yields 10 lines and 21 matches. > > > > If possible, I'd actually change tst_umount, to always unmount the > > > mountpoint and not the device, i.e. if the given path is not an absolute > > > path, make it absolute (e.g. by prepending $PWD"). > > > This way the check if the mountpoint exist wouldn't be the fuzzy thing > > > it is right now. > +1 > > > Strongly agree here. > > > I would go even one step further and change the library so that it > > rejects anything that does not start with '/'. > +1 > Will include both points in v6 patch! > Kind regards, > Petr Best regards, Leo
diff --git a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh index 1f7f3820e..06379c7ae 100755 --- a/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh +++ b/testcases/kernel/controllers/cgroup/cgroup_regression_test.sh @@ -145,7 +145,7 @@ test2() fi rmdir cgroup/0 cgroup/1 - umount cgroup/ + tst_umount cgroup/ # Avoid possible EBUSY error } #--------------------------------------------------------------------------- @@ -193,7 +193,7 @@ test3() wait $pid2 2>/dev/null rmdir $cpu_subsys_path/0 2> /dev/null - umount cgroup/ 2> /dev/null + tst_umount cgroup/ 2> /dev/null # Avoid possible EBUSY error check_kernel_bug } @@ -222,7 +222,7 @@ test4() mount -t cgroup -o none,name=foo cgroup cgroup/ mkdir cgroup/0 rmdir cgroup/0 - umount cgroup/ + tst_umount cgroup/ # Avoid possible EBUSY error if dmesg | grep -q "MAX_LOCKDEP_SUBCLASSES too low"; then tst_res TFAIL "lockdep BUG was found" @@ -254,7 +254,7 @@ test5() mount -t cgroup none cgroup 2> /dev/null mkdir cgroup/0 rmdir cgroup/0 - umount cgroup/ 2> /dev/null + tst_umount cgroup/ 2> /dev/null # Avoid possible EBUSY error check_kernel_bug } @@ -290,7 +290,7 @@ test6() mount -t cgroup -o ns xxx cgroup/ > /dev/null 2>&1 rmdir cgroup/[1-9]* > /dev/null 2>&1 - umount cgroup/ + tst_umount cgroup/ # Avoid possible EBUSY error check_kernel_bug }
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 on certain platform. The reason is that this test sequence would have the chance of missing a release code path when doing rmdir and umount. Adding a little delay between "rmdir" and "umount" could fix the problem, so use tst_umount API instead of umount in "rmdir, umount" sequence. Fixes: #839 Signed-off-by: Leo Yu-Chi Liang <ycliang@andestech.com> --- .../controllers/cgroup/cgroup_regression_test.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)