| Message ID | 20181204174733.7423-2-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | io.latency test for blktests | expand |
On 12/4/18 9:47 AM, Josef Bacik wrote: > In order to test io.latency and other cgroup related things we need some > supporting helpers to setup and tear down cgroup2. This adds support > for checking that we can even configure cgroup2 things, set them up if > need be, and then add the cleanup stuff to the main cleanup function so > everything is always in a clean state. Is this the patch that went in as commit ae7daae7e35a ("blktests: add cgroup2 infrastructure")? I think that commit introduced a regression. With that patch applied the SRP tests fail as follows: # ./check -q srp/001 srp/001 (Create and remove LUNs) runtime 4.067s ... common/cgroup: line 25: CGROUP2_DIR: unbound variable Is this a known issue? Bart.
On Tue, 2019-01-01 at 19:13 -0800, Bart Van Assche wrote: > On 12/4/18 9:47 AM, Josef Bacik wrote: > > In order to test io.latency and other cgroup related things we need some > > supporting helpers to setup and tear down cgroup2. This adds support > > for checking that we can even configure cgroup2 things, set them up if > > need be, and then add the cleanup stuff to the main cleanup function so > > everything is always in a clean state. > > Is this the patch that went in as commit ae7daae7e35a ("blktests: add > cgroup2 infrastructure")? I think that commit introduced a regression. > With that patch applied the SRP tests fail as follows: > > # ./check -q srp/001 > srp/001 (Create and remove LUNs) > runtime 4.067s ... > common/cgroup: line 25: CGROUP2_DIR: unbound variable > > Is this a known issue? Hi Josef, Had you noticed this e-mail? Thanks, Bart.
On Tue, Jan 15, 2019 at 08:40:41AM -0800, Bart Van Assche wrote: > On Tue, 2019-01-01 at 19:13 -0800, Bart Van Assche wrote: > > On 12/4/18 9:47 AM, Josef Bacik wrote: > > > In order to test io.latency and other cgroup related things we need some > > > supporting helpers to setup and tear down cgroup2. This adds support > > > for checking that we can even configure cgroup2 things, set them up if > > > need be, and then add the cleanup stuff to the main cleanup function so > > > everything is always in a clean state. > > > > Is this the patch that went in as commit ae7daae7e35a ("blktests: add > > cgroup2 infrastructure")? I think that commit introduced a regression. > > With that patch applied the SRP tests fail as follows: > > > > # ./check -q srp/001 > > srp/001 (Create and remove LUNs) > > runtime 4.067s ... > > common/cgroup: line 25: CGROUP2_DIR: unbound variable > > > > Is this a known issue? > > Hi Josef, > > Had you noticed this e-mail? > > Thanks, > > Bart. Hey, Bart, I just pushed a fix for this: commit 8a274578e2895b9f0b66c09f3a8f63b5ff1293b2 Author: Omar Sandoval <osandov@fb.com> Date: Wed Jan 16 17:34:19 2019 -0800 cgroup: test if CGROUP2_DIR is set with -v instead of -n common/multipath-over-rdma does set -u, so -n "$CGROUP2_DIR" fails with an unbound variable error. Instead, use -v to test if the variable was set. Signed-off-by: Omar Sandoval <osandov@fb.com> diff --git a/common/cgroup b/common/cgroup index 48e546f..554ebf7 100644 --- a/common/cgroup +++ b/common/cgroup @@ -22,7 +22,7 @@ _init_cgroup2() _exit_cgroup2() { - if [[ -n $CGROUP2_DIR ]]; then + if [[ -v CGROUP2_DIR ]]; then find "$CGROUP2_DIR" -type d -delete unset CGROUP2_DIR fi
On 1/16/19 5:40 PM, Omar Sandoval wrote: > On Tue, Jan 15, 2019 at 08:40:41AM -0800, Bart Van Assche wrote: >> On Tue, 2019-01-01 at 19:13 -0800, Bart Van Assche wrote: >>> On 12/4/18 9:47 AM, Josef Bacik wrote: >>>> In order to test io.latency and other cgroup related things we need some >>>> supporting helpers to setup and tear down cgroup2. This adds support >>>> for checking that we can even configure cgroup2 things, set them up if >>>> need be, and then add the cleanup stuff to the main cleanup function so >>>> everything is always in a clean state. >>> >>> Is this the patch that went in as commit ae7daae7e35a ("blktests: add >>> cgroup2 infrastructure")? I think that commit introduced a regression. >>> With that patch applied the SRP tests fail as follows: >>> >>> # ./check -q srp/001 >>> srp/001 (Create and remove LUNs) >>> runtime 4.067s ... >>> common/cgroup: line 25: CGROUP2_DIR: unbound variable >>> >>> Is this a known issue? >> >> Hi Josef, >> >> Had you noticed this e-mail? >> >> Thanks, >> >> Bart. > > Hey, Bart, I just pushed a fix for this: > > commit 8a274578e2895b9f0b66c09f3a8f63b5ff1293b2 > Author: Omar Sandoval <osandov@fb.com> > Date: Wed Jan 16 17:34:19 2019 -0800 > > cgroup: test if CGROUP2_DIR is set with -v instead of -n > > common/multipath-over-rdma does set -u, so -n "$CGROUP2_DIR" fails with > an unbound variable error. Instead, use -v to test if the variable was > set. > > Signed-off-by: Omar Sandoval <osandov@fb.com> > > diff --git a/common/cgroup b/common/cgroup > index 48e546f..554ebf7 100644 > --- a/common/cgroup > +++ b/common/cgroup > @@ -22,7 +22,7 @@ _init_cgroup2() > > _exit_cgroup2() > { > - if [[ -n $CGROUP2_DIR ]]; then > + if [[ -v CGROUP2_DIR ]]; then > find "$CGROUP2_DIR" -type d -delete > unset CGROUP2_DIR > fi That change looks good to me. Thanks! Bart.
On Wed, Jan 16, 2019 at 06:31:51PM -0800, Bart Van Assche wrote: > On 1/16/19 5:40 PM, Omar Sandoval wrote: > > On Tue, Jan 15, 2019 at 08:40:41AM -0800, Bart Van Assche wrote: > > > On Tue, 2019-01-01 at 19:13 -0800, Bart Van Assche wrote: > > > > On 12/4/18 9:47 AM, Josef Bacik wrote: > > > > > In order to test io.latency and other cgroup related things we need some > > > > > supporting helpers to setup and tear down cgroup2. This adds support > > > > > for checking that we can even configure cgroup2 things, set them up if > > > > > need be, and then add the cleanup stuff to the main cleanup function so > > > > > everything is always in a clean state. > > > > > > > > Is this the patch that went in as commit ae7daae7e35a ("blktests: add > > > > cgroup2 infrastructure")? I think that commit introduced a regression. > > > > With that patch applied the SRP tests fail as follows: > > > > > > > > # ./check -q srp/001 > > > > srp/001 (Create and remove LUNs) > > > > runtime 4.067s ... > > > > common/cgroup: line 25: CGROUP2_DIR: unbound variable > > > > > > > > Is this a known issue? > > > > > > Hi Josef, > > > > > > Had you noticed this e-mail? > > > > > > Thanks, > > > > > > Bart. > > > > Hey, Bart, I just pushed a fix for this: > > > > commit 8a274578e2895b9f0b66c09f3a8f63b5ff1293b2 > > Author: Omar Sandoval <osandov@fb.com> > > Date: Wed Jan 16 17:34:19 2019 -0800 > > > > cgroup: test if CGROUP2_DIR is set with -v instead of -n > > common/multipath-over-rdma does set -u, so -n "$CGROUP2_DIR" fails with > > an unbound variable error. Instead, use -v to test if the variable was > > set. > > Signed-off-by: Omar Sandoval <osandov@fb.com> > > > > diff --git a/common/cgroup b/common/cgroup > > index 48e546f..554ebf7 100644 > > --- a/common/cgroup > > +++ b/common/cgroup > > @@ -22,7 +22,7 @@ _init_cgroup2() > > _exit_cgroup2() > > { > > - if [[ -n $CGROUP2_DIR ]]; then > > + if [[ -v CGROUP2_DIR ]]; then > > find "$CGROUP2_DIR" -type d -delete > > unset CGROUP2_DIR > > fi > > That change looks good to me. Thanks! > Hmm sorry Bart, I wasn't trying to ignore you, your email ended up in a folder for some reason and I didn't see this until Omar sent his patch. Thanks, Josef
diff --git a/check b/check index ebd87c097e25..1c9dbc518fa1 100755 --- a/check +++ b/check @@ -294,6 +294,8 @@ _cleanup() { done unset RESTORE_CPUS_ONLINE fi + + _cleanup_cgroup2 } _call_test() { diff --git a/common/rc b/common/rc index 8a892bcd5fde..a785f2329687 100644 --- a/common/rc +++ b/common/rc @@ -202,3 +202,51 @@ _test_dev_in_hotplug_slot() { _filter_xfs_io_error() { sed -e 's/^\(.*\)64\(: .*$\)/\1\2/' } + +_cgroup2_base_dir() +{ + grep cgroup2 /proc/mounts | awk '{ print $2 }' +} + +_cleanup_cgroup2() +{ + _dir=$(_cgroup2_base_dir)/blktests + [ -d "${_dir}" ] || return + + for i in $(find ${_dir} -type d | tac) + do + rmdir $i + done +} + +_have_cgroup2() +{ + if ! grep -q 'cgroup2' /proc/mounts; then + SKIP_REASON="This test requires cgroup2" + return 1 + fi + return 0 +} + +_have_cgroup2_controller_file() +{ + _have_cgroup2 || return 1 + + _controller=$1 + _file=$2 + _dir=$(_cgroup2_base_dir) + + if ! grep -q ${_controller} ${_dir}/cgroup.controllers; then + SKIP_REASON="No support for ${_controller} cgroup controller" + return 1 + fi + + mkdir ${_dir}/blktests + echo "+${_controller}" > ${_dir}/cgroup.subtree_control + if [ ! -f ${_dir}/blktests/${_file} ]; then + _cleanup_cgroup2 + SKIP_REASON="Cgroup file ${_file} doesn't exist" + return 1 + fi + return 0 +}
In order to test io.latency and other cgroup related things we need some supporting helpers to setup and tear down cgroup2. This adds support for checking that we can even configure cgroup2 things, set them up if need be, and then add the cleanup stuff to the main cleanup function so everything is always in a clean state. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- check | 2 ++ common/rc | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+)