[v2] xfstests: add a option to run xfstests under a cgroup
diff mbox series

Message ID 20200218152712.3750130-1-josef@toxicpanda.com
State New
Headers show
Series
  • [v2] xfstests: add a option to run xfstests under a cgroup
Related show

Commit Message

Josef Bacik Feb. 18, 2020, 3:27 p.m. UTC
I want to add some extended statistic gathering for xfstests, but it's
tricky to isolate xfstests from the rest of the host applications.  The
most straightforward way to do this is to run every test inside of it's
own cgroup.  From there we can monitor the activity of tasks in the
specific cgroup using BPF.

The support for this is pretty simple, allow users to pass -C <cgroup
name>.  We will create the path if it doesn't already exist, and
validate we can add things to cgroup.procs.  If we cannot it'll be
disabled, otherwise we will use this when we do _run_seq by echo'ing the
bash pid into cgroup.procs, which will cause any children to run under
that cgroup.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
v1->v2:
- Changed it from a local.config option to a command line option.
- Export CGROUP2_PATH for everything, utilize that path when generating our
  cgroup for the scripts to run in.

 check          | 24 +++++++++++++++++++++++-
 common/cgroup2 |  2 --
 common/config  |  1 +
 3 files changed, 24 insertions(+), 3 deletions(-)

Comments

Brian Foster Feb. 19, 2020, 3:48 p.m. UTC | #1
On Tue, Feb 18, 2020 at 10:27:12AM -0500, Josef Bacik wrote:
> I want to add some extended statistic gathering for xfstests, but it's
> tricky to isolate xfstests from the rest of the host applications.  The
> most straightforward way to do this is to run every test inside of it's
> own cgroup.  From there we can monitor the activity of tasks in the
> specific cgroup using BPF.
> 
> The support for this is pretty simple, allow users to pass -C <cgroup
> name>.  We will create the path if it doesn't already exist, and
> validate we can add things to cgroup.procs.  If we cannot it'll be
> disabled, otherwise we will use this when we do _run_seq by echo'ing the
> bash pid into cgroup.procs, which will cause any children to run under
> that cgroup.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> v1->v2:
> - Changed it from a local.config option to a command line option.
> - Export CGROUP2_PATH for everything, utilize that path when generating our
>   cgroup for the scripts to run in.
> 
>  check          | 24 +++++++++++++++++++++++-
>  common/cgroup2 |  2 --
>  common/config  |  1 +
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/check b/check
> index 2e148e57..df33628e 100755
> --- a/check
> +++ b/check
> @@ -72,6 +72,7 @@ check options
>      --large-fs		optimise scratch device for large filesystems
>      -s section		run only specified section from config file
>      -S section		exclude the specified section from the config file
> +    -C cgroup_name	run all the tests in the specified cgroup name
>  
>  testlist options
>      -g group[,group...]	include tests from these groups
> @@ -101,6 +102,10 @@ excluded from the list of tests to run from that test dir.
>  external_file argument is a path to a single file containing a list of tests
>  to exclude in the form of <test dir>/<test name>.
>  
> +cgroup_name is just a plain name, or a path relative to the root cgroup path.
> +If CGROUP2_PATH does not point at where cgroup2 is mounted then adjust it
> +accordingly.
> +
>  examples:
>   check xfs/001
>   check -g quick
> @@ -307,6 +312,7 @@ while [ $# -gt 0 ]; do
>  		;;
>  	--large-fs) export LARGE_SCRATCH_DEV=yes ;;
>  	--extra-space=*) export SCRATCH_DEV_EMPTY_SPACE=${r#*=} ;;
> +	-C)	CGROUP=$2 ; shift ;;
>  
>  	-*)	usage ;;
>  	*)	# not an argument, we've got tests now.
> @@ -509,11 +515,24 @@ _expunge_test()
>  OOM_SCORE_ADJ="/proc/self/oom_score_adj"
>  test -w ${OOM_SCORE_ADJ} && echo -1000 > ${OOM_SCORE_ADJ}
>  
> +# Initialize the cgroup path if it doesn't already exist
> +if [ ! -z "$CGROUP" ]; then
> +	CGROUP=${CGROUP2_PATH}/${CGROUP}
> +	mkdir -p ${CGROUP}
> +
> +	# If we can't write to cgroup.procs then unset cgroup
> +	test -w ${CGROUP}/cgroup.procs || unset CGROUP
> +fi
> +

Do we need to fix up generic/563 to use this new $CGROUP value, when
set? That test explicitly moves tasks to new/temporary groups, but looks
like it resets back to the top-level CGROUP2_PATH group. I'm not sure
how much that really matters since presumably the next test should move
back into $CGROUP. Otherwise this looks reasonable enough to me.

Brian

>  # ...and make the tests themselves somewhat more attractive to it, so that if
>  # the system runs out of memory it'll be the test that gets killed and not the
>  # test framework.
>  _run_seq() {
> -	bash -c "test -w ${OOM_SCORE_ADJ} && echo 250 > ${OOM_SCORE_ADJ}; exec ./$seq"
> +	_extra="test -w ${OOM_SCORE_ADJ} && echo 250 > ${OOM_SCORE_ADJ};"
> +	if [ ! -z "$CGROUP" ]; then
> +		_extra+="echo $$ > ${CGROUP}/cgroup.procs;"
> +	fi
> +	bash -c "${_extra} exec ./$seq"
>  }
>  
>  _detect_kmemleak
> @@ -615,6 +634,9 @@ for section in $HOST_OPTIONS_SECTIONS; do
>  	  echo "MKFS_OPTIONS  -- `_scratch_mkfs_options`"
>  	  echo "MOUNT_OPTIONS -- `_scratch_mount_options`"
>  	fi
> +	if [ ! -z "$CGROUP" ]; then
> +	  echo "CGROUP        -- ${CGROUP}"
> +	fi
>  	echo
>  	needwrap=true
>  
> diff --git a/common/cgroup2 b/common/cgroup2
> index 8833c9c8..554bd238 100644
> --- a/common/cgroup2
> +++ b/common/cgroup2
> @@ -1,7 +1,5 @@
>  # cgroup2 specific common functions
>  
> -export CGROUP2_PATH="${CGROUP2_PATH:-/sys/fs/cgroup}"
> -
>  _require_cgroup2()
>  {
>  	if [ `findmnt -d backward -n -o FSTYPE -f ${CGROUP2_PATH}` != "cgroup2" ]; then
> diff --git a/common/config b/common/config
> index 9a9c7760..0eaf35c3 100644
> --- a/common/config
> +++ b/common/config
> @@ -259,6 +259,7 @@ case "$HOSTOS" in
>  	export E2FSCK_PROG=$(type -P e2fsck)
>  	export TUNE2FS_PROG=$(type -P tune2fs)
>  	export FSCK_OVERLAY_PROG=$(type -P fsck.overlay)
> +	export CGROUP2_PATH="${CGROUP2_PATH:-/sys/fs/cgroup}"
>          ;;
>  esac
>  
> -- 
> 2.24.1
>
Josef Bacik Feb. 19, 2020, 3:52 p.m. UTC | #2
On 2/19/20 10:48 AM, Brian Foster wrote:
> On Tue, Feb 18, 2020 at 10:27:12AM -0500, Josef Bacik wrote:
>> I want to add some extended statistic gathering for xfstests, but it's
>> tricky to isolate xfstests from the rest of the host applications.  The
>> most straightforward way to do this is to run every test inside of it's
>> own cgroup.  From there we can monitor the activity of tasks in the
>> specific cgroup using BPF.
>>
>> The support for this is pretty simple, allow users to pass -C <cgroup
>> name>.  We will create the path if it doesn't already exist, and
>> validate we can add things to cgroup.procs.  If we cannot it'll be
>> disabled, otherwise we will use this when we do _run_seq by echo'ing the
>> bash pid into cgroup.procs, which will cause any children to run under
>> that cgroup.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>> v1->v2:
>> - Changed it from a local.config option to a command line option.
>> - Export CGROUP2_PATH for everything, utilize that path when generating our
>>    cgroup for the scripts to run in.
>>
>>   check          | 24 +++++++++++++++++++++++-
>>   common/cgroup2 |  2 --
>>   common/config  |  1 +
>>   3 files changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/check b/check
>> index 2e148e57..df33628e 100755
>> --- a/check
>> +++ b/check
>> @@ -72,6 +72,7 @@ check options
>>       --large-fs		optimise scratch device for large filesystems
>>       -s section		run only specified section from config file
>>       -S section		exclude the specified section from the config file
>> +    -C cgroup_name	run all the tests in the specified cgroup name
>>   
>>   testlist options
>>       -g group[,group...]	include tests from these groups
>> @@ -101,6 +102,10 @@ excluded from the list of tests to run from that test dir.
>>   external_file argument is a path to a single file containing a list of tests
>>   to exclude in the form of <test dir>/<test name>.
>>   
>> +cgroup_name is just a plain name, or a path relative to the root cgroup path.
>> +If CGROUP2_PATH does not point at where cgroup2 is mounted then adjust it
>> +accordingly.
>> +
>>   examples:
>>    check xfs/001
>>    check -g quick
>> @@ -307,6 +312,7 @@ while [ $# -gt 0 ]; do
>>   		;;
>>   	--large-fs) export LARGE_SCRATCH_DEV=yes ;;
>>   	--extra-space=*) export SCRATCH_DEV_EMPTY_SPACE=${r#*=} ;;
>> +	-C)	CGROUP=$2 ; shift ;;
>>   
>>   	-*)	usage ;;
>>   	*)	# not an argument, we've got tests now.
>> @@ -509,11 +515,24 @@ _expunge_test()
>>   OOM_SCORE_ADJ="/proc/self/oom_score_adj"
>>   test -w ${OOM_SCORE_ADJ} && echo -1000 > ${OOM_SCORE_ADJ}
>>   
>> +# Initialize the cgroup path if it doesn't already exist
>> +if [ ! -z "$CGROUP" ]; then
>> +	CGROUP=${CGROUP2_PATH}/${CGROUP}
>> +	mkdir -p ${CGROUP}
>> +
>> +	# If we can't write to cgroup.procs then unset cgroup
>> +	test -w ${CGROUP}/cgroup.procs || unset CGROUP
>> +fi
>> +
> 
> Do we need to fix up generic/563 to use this new $CGROUP value, when
> set? That test explicitly moves tasks to new/temporary groups, but looks
> like it resets back to the top-level CGROUP2_PATH group. I'm not sure
> how much that really matters since presumably the next test should move
> back into $CGROUP. Otherwise this looks reasonable enough to me.
> 

Wtf I had an explanation for this in my commit message, I must have exited 
without saving.

I tried to do this actually, and it ended up spewing because you cannot change 
cgroup.subtree_control for a cgroup you are currently inside of.  We can fix 
this by just making sure all controllers are enabled in our test cgroup, and 
then fix 562 to not bother messing subtree and just assume everything is 
enabled.  But since it's just one test and I wasn't sure if it would create more 
wonkiness I just left it as is.  I'm open to either solution, I just opted for 
the less change route.  Thanks,

Josef
Brian Foster Feb. 19, 2020, 4:07 p.m. UTC | #3
On Wed, Feb 19, 2020 at 10:52:43AM -0500, Josef Bacik wrote:
> On 2/19/20 10:48 AM, Brian Foster wrote:
> > On Tue, Feb 18, 2020 at 10:27:12AM -0500, Josef Bacik wrote:
> > > I want to add some extended statistic gathering for xfstests, but it's
> > > tricky to isolate xfstests from the rest of the host applications.  The
> > > most straightforward way to do this is to run every test inside of it's
> > > own cgroup.  From there we can monitor the activity of tasks in the
> > > specific cgroup using BPF.
> > > 
> > > The support for this is pretty simple, allow users to pass -C <cgroup
> > > name>.  We will create the path if it doesn't already exist, and
> > > validate we can add things to cgroup.procs.  If we cannot it'll be
> > > disabled, otherwise we will use this when we do _run_seq by echo'ing the
> > > bash pid into cgroup.procs, which will cause any children to run under
> > > that cgroup.
> > > 
> > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > > ---
> > > v1->v2:
> > > - Changed it from a local.config option to a command line option.
> > > - Export CGROUP2_PATH for everything, utilize that path when generating our
> > >    cgroup for the scripts to run in.
> > > 
> > >   check          | 24 +++++++++++++++++++++++-
> > >   common/cgroup2 |  2 --
> > >   common/config  |  1 +
> > >   3 files changed, 24 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/check b/check
> > > index 2e148e57..df33628e 100755
> > > --- a/check
> > > +++ b/check
> > > @@ -72,6 +72,7 @@ check options
> > >       --large-fs		optimise scratch device for large filesystems
> > >       -s section		run only specified section from config file
> > >       -S section		exclude the specified section from the config file
> > > +    -C cgroup_name	run all the tests in the specified cgroup name
> > >   testlist options
> > >       -g group[,group...]	include tests from these groups
> > > @@ -101,6 +102,10 @@ excluded from the list of tests to run from that test dir.
> > >   external_file argument is a path to a single file containing a list of tests
> > >   to exclude in the form of <test dir>/<test name>.
> > > +cgroup_name is just a plain name, or a path relative to the root cgroup path.
> > > +If CGROUP2_PATH does not point at where cgroup2 is mounted then adjust it
> > > +accordingly.
> > > +
> > >   examples:
> > >    check xfs/001
> > >    check -g quick
> > > @@ -307,6 +312,7 @@ while [ $# -gt 0 ]; do
> > >   		;;
> > >   	--large-fs) export LARGE_SCRATCH_DEV=yes ;;
> > >   	--extra-space=*) export SCRATCH_DEV_EMPTY_SPACE=${r#*=} ;;
> > > +	-C)	CGROUP=$2 ; shift ;;
> > >   	-*)	usage ;;
> > >   	*)	# not an argument, we've got tests now.
> > > @@ -509,11 +515,24 @@ _expunge_test()
> > >   OOM_SCORE_ADJ="/proc/self/oom_score_adj"
> > >   test -w ${OOM_SCORE_ADJ} && echo -1000 > ${OOM_SCORE_ADJ}
> > > +# Initialize the cgroup path if it doesn't already exist
> > > +if [ ! -z "$CGROUP" ]; then
> > > +	CGROUP=${CGROUP2_PATH}/${CGROUP}
> > > +	mkdir -p ${CGROUP}
> > > +
> > > +	# If we can't write to cgroup.procs then unset cgroup
> > > +	test -w ${CGROUP}/cgroup.procs || unset CGROUP
> > > +fi
> > > +
> > 
> > Do we need to fix up generic/563 to use this new $CGROUP value, when
> > set? That test explicitly moves tasks to new/temporary groups, but looks
> > like it resets back to the top-level CGROUP2_PATH group. I'm not sure
> > how much that really matters since presumably the next test should move
> > back into $CGROUP. Otherwise this looks reasonable enough to me.
> > 
> 
> Wtf I had an explanation for this in my commit message, I must have exited
> without saving.
> 
> I tried to do this actually, and it ended up spewing because you cannot
> change cgroup.subtree_control for a cgroup you are currently inside of.  We
> can fix this by just making sure all controllers are enabled in our test
> cgroup, and then fix 562 to not bother messing subtree and just assume
> everything is enabled.  But since it's just one test and I wasn't sure if it
> would create more wonkiness I just left it as is.  I'm open to either
> solution, I just opted for the less change route.  Thanks,
> 

Oh, Ok. Eh, it seems the test should still work either way so I'm fine
with leaving it as is as well until there's some better reason to do
otherwise, as long as you can add that explanation back into the commit
log..

Brian

> Josef
>

Patch
diff mbox series

diff --git a/check b/check
index 2e148e57..df33628e 100755
--- a/check
+++ b/check
@@ -72,6 +72,7 @@  check options
     --large-fs		optimise scratch device for large filesystems
     -s section		run only specified section from config file
     -S section		exclude the specified section from the config file
+    -C cgroup_name	run all the tests in the specified cgroup name
 
 testlist options
     -g group[,group...]	include tests from these groups
@@ -101,6 +102,10 @@  excluded from the list of tests to run from that test dir.
 external_file argument is a path to a single file containing a list of tests
 to exclude in the form of <test dir>/<test name>.
 
+cgroup_name is just a plain name, or a path relative to the root cgroup path.
+If CGROUP2_PATH does not point at where cgroup2 is mounted then adjust it
+accordingly.
+
 examples:
  check xfs/001
  check -g quick
@@ -307,6 +312,7 @@  while [ $# -gt 0 ]; do
 		;;
 	--large-fs) export LARGE_SCRATCH_DEV=yes ;;
 	--extra-space=*) export SCRATCH_DEV_EMPTY_SPACE=${r#*=} ;;
+	-C)	CGROUP=$2 ; shift ;;
 
 	-*)	usage ;;
 	*)	# not an argument, we've got tests now.
@@ -509,11 +515,24 @@  _expunge_test()
 OOM_SCORE_ADJ="/proc/self/oom_score_adj"
 test -w ${OOM_SCORE_ADJ} && echo -1000 > ${OOM_SCORE_ADJ}
 
+# Initialize the cgroup path if it doesn't already exist
+if [ ! -z "$CGROUP" ]; then
+	CGROUP=${CGROUP2_PATH}/${CGROUP}
+	mkdir -p ${CGROUP}
+
+	# If we can't write to cgroup.procs then unset cgroup
+	test -w ${CGROUP}/cgroup.procs || unset CGROUP
+fi
+
 # ...and make the tests themselves somewhat more attractive to it, so that if
 # the system runs out of memory it'll be the test that gets killed and not the
 # test framework.
 _run_seq() {
-	bash -c "test -w ${OOM_SCORE_ADJ} && echo 250 > ${OOM_SCORE_ADJ}; exec ./$seq"
+	_extra="test -w ${OOM_SCORE_ADJ} && echo 250 > ${OOM_SCORE_ADJ};"
+	if [ ! -z "$CGROUP" ]; then
+		_extra+="echo $$ > ${CGROUP}/cgroup.procs;"
+	fi
+	bash -c "${_extra} exec ./$seq"
 }
 
 _detect_kmemleak
@@ -615,6 +634,9 @@  for section in $HOST_OPTIONS_SECTIONS; do
 	  echo "MKFS_OPTIONS  -- `_scratch_mkfs_options`"
 	  echo "MOUNT_OPTIONS -- `_scratch_mount_options`"
 	fi
+	if [ ! -z "$CGROUP" ]; then
+	  echo "CGROUP        -- ${CGROUP}"
+	fi
 	echo
 	needwrap=true
 
diff --git a/common/cgroup2 b/common/cgroup2
index 8833c9c8..554bd238 100644
--- a/common/cgroup2
+++ b/common/cgroup2
@@ -1,7 +1,5 @@ 
 # cgroup2 specific common functions
 
-export CGROUP2_PATH="${CGROUP2_PATH:-/sys/fs/cgroup}"
-
 _require_cgroup2()
 {
 	if [ `findmnt -d backward -n -o FSTYPE -f ${CGROUP2_PATH}` != "cgroup2" ]; then
diff --git a/common/config b/common/config
index 9a9c7760..0eaf35c3 100644
--- a/common/config
+++ b/common/config
@@ -259,6 +259,7 @@  case "$HOSTOS" in
 	export E2FSCK_PROG=$(type -P e2fsck)
 	export TUNE2FS_PROG=$(type -P tune2fs)
 	export FSCK_OVERLAY_PROG=$(type -P fsck.overlay)
+	export CGROUP2_PATH="${CGROUP2_PATH:-/sys/fs/cgroup}"
         ;;
 esac