Re: [PATCH][v2] xfstests: add a option to run xfstests under a cgroup

[Brian Foster <bfoster@redhat.com>]

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
>