[PATCH 0/3] io.latency test for blktests

[PATCH 0/3] io.latency test for blktests

[Josef Bacik]

This patchset is to add a test to verify io.latency is working properly, and to
add all the supporting code to run that test.

First is the cgroup2 infrastructure which is fairly straightforward.  Just
verifies we have cgroup2, and gives us the helpers to check and make sure we
have the right controllers in place for the test.

The second patch brings over some python scripts I put in xfstests for parsing
the fio json output.  I looked at the existing fio performance stuff in
blktests, but we only capture bw stuff, which is wonky with this particular test
because once the fast group is finished the slow group is allowed to go as fast
as it wants.  So I needed this to pull out actual jobtime spent.  This will give
us flexibility to pull out other fio performance data in the future.

The final patch is the test itself.  It simply runs a job by itself to get a
baseline view of the disk performance.  Then it creates 2 cgroups, one fast and
one slow, and runs the same job simultaneously in both groups.  The result
should be that the fast group takes just slightly longer time than the baseline
(I use a 15% threshold to be safe), and that the slow one takes considerably
longer.  Thanks,

Josef

[PATCH 1/3] blktests: add cgroup2 infrastructure

[Josef Bacik]

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(+)

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
+}
-- 
2.14.3

[PATCH 2/3] blktests: add python scripts for parsing fio json output

[Josef Bacik]

I wrote these scripts for xfstests, just copying them over to blktests
as well.  The terse output fio support that blktests doesn't get all of
the various fio performance things that we may want to look at, this
gives us a lot of flexibility around getting specific values out of the
fio results data.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 src/FioResultDecoder.py | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/fio-key-value.py    | 28 ++++++++++++++++++++++
 2 files changed, 92 insertions(+)
 create mode 100644 src/FioResultDecoder.py
 create mode 100644 src/fio-key-value.py

diff --git a/src/FioResultDecoder.py b/src/FioResultDecoder.py
new file mode 100644
index 000000000000..d004140c0fdf
--- /dev/null
+++ b/src/FioResultDecoder.py
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: GPL-2.0
+
+import json
+
+class FioResultDecoder(json.JSONDecoder):
+    """Decoder for decoding fio result json to an object for our database
+
+    This decodes the json output from fio into an object that can be directly
+    inserted into our database.  This just strips out the fields we don't care
+    about and collapses the read/write/trim classes into a flat value structure
+    inside of the jobs object.
+
+    For example
+        "write" : {
+            "io_bytes" : 313360384,
+            "bw" : 1016,
+        }
+
+    Get's collapsed to
+
+        "write_io_bytes" : 313360384,
+        "write_bw": 1016,
+
+    Currently any dict under 'jobs' get's dropped, with the exception of 'read',
+    'write', and 'trim'.  For those sub sections we drop any dict's under those.
+
+    Attempt to keep this as generic as possible, we don't want to break every
+    time fio changes it's json output format.
+    """
+    _ignore_types = ['dict', 'list']
+    _override_keys = ['lat_ns', 'lat']
+    _io_ops = ['read', 'write', 'trim']
+
+    _transform_keys = { 'lat': 'lat_ns' }
+
+    def decode(self, json_string):
+        """This does the dirty work of converting everything"""
+        default_obj = super(FioResultDecoder, self).decode(json_string)
+        obj = {}
+        obj['global'] = {}
+        obj['global']['time'] = default_obj['time']
+        obj['jobs'] = []
+        for job in default_obj['jobs']:
+            new_job = {}
+            for key,value in job.iteritems():
+                if key not in self._io_ops:
+                    if value.__class__.__name__ in self._ignore_types:
+                        continue
+                    new_job[key] = value
+                    continue
+                for k,v in value.iteritems():
+                    if k in self._override_keys:
+                        if k in self._transform_keys:
+                            k = self._transform_keys[k]
+                        for subk,subv in v.iteritems():
+                            collapsed_key = "{}_{}_{}".format(key, k, subk)
+                            new_job[collapsed_key] = subv
+                        continue
+                    if v.__class__.__name__ in self._ignore_types:
+                        continue
+                    collapsed_key = "{}_{}".format(key, k)
+                    new_job[collapsed_key] = v
+            obj['jobs'].append(new_job)
+        return obj
diff --git a/src/fio-key-value.py b/src/fio-key-value.py
new file mode 100644
index 000000000000..208e9a453a19
--- /dev/null
+++ b/src/fio-key-value.py
@@ -0,0 +1,28 @@
+# SPDX-License-Identifier: GPL-2.0
+
+import FioResultDecoder
+import json
+import argparse
+import sys
+import platform
+
+parser = argparse.ArgumentParser()
+parser.add_argument('-j', '--jobname', type=str,
+                    help="The jobname we want our key from.",
+                    required=True)
+parser.add_argument('-k', '--key', type=str,
+                    help="The key we want the value of", required=True)
+parser.add_argument('result', type=str,
+                    help="The result file read")
+args = parser.parse_args()
+
+json_data = open(args.result)
+data = json.load(json_data, cls=FioResultDecoder.FioResultDecoder)
+
+for job in data['jobs']:
+    if job['jobname'] == args.jobname:
+        if args.key not in job:
+            print('')
+        else:
+            print("{}".format(job[args.key]))
+        break
-- 
2.14.3

[PATCH 3/3] blktests: block/025: an io.latency test

[Josef Bacik]

This is a test to verify io.latency is working properly.  It does this
by first running a fio job by itself to figure out how fast it runs.
Then we calculate some thresholds, set up 2 cgroups, a fast and a slow
cgroup, and then run the same job in both groups simultaneously.  We
should see the slow group get throttled until the first cgroup is able
to finish, and then the slow cgroup will be allowed to finish.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 tests/block/025     | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/block/025.out |   1 +
 2 files changed, 134 insertions(+)
 create mode 100644 tests/block/025
 create mode 100644 tests/block/025.out

diff --git a/tests/block/025 b/tests/block/025
new file mode 100644
index 000000000000..88ce7cca944e
--- /dev/null
+++ b/tests/block/025
@@ -0,0 +1,133 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+#
+# Test io.latency to make sure it's protecting the higher priority group
+# properly.
+
+. tests/block/rc
+
+DESCRIPTION="test the io.latency interface to make sure it's working right"
+
+requires() {
+	_have_cgroup2_controller_file io io.latency && _have_fio && \
+		_have_program python
+}
+
+_fio_results_key() {
+	_job=$1
+	_key=$2
+	_resultfile=$3
+
+	python src/fio-key-value.py -k $_key -j $_job $_resultfile
+}
+
+test_device() {
+	local fio_config_single fio_config_double fio_results fio_args qd
+
+	fio_config_single=${TMPDIR}/single.fio
+	fio_config_double=${TMPDIR}/double.fio
+	fio_results=${TMPDIR}/025.json
+	fio_args="--output-format=json --output=${fio_results}"
+	qd=$(cat ${TEST_DEV_SYSFS}/queue/nr_requests)
+
+	cat << EOF > "${fio_config_single}"
+	[fast]
+	filename=${TEST_DEV}
+	direct=1
+	allrandrepeat=1
+	readwrite=randrw
+	size=4G
+	ioengine=libaio
+	iodepth=${qd}
+	fallocate=none
+	randseed=12345
+EOF
+
+	cat << EOF > "${fio_config_double}"
+	[global]
+	filename=${TEST_DEV}
+	direct=1
+	allrandrepeat=1
+	readwrite=randrw
+	size=4G
+	ioengine=libaio
+	iodepth=${qd}
+	fallocate=none
+	randseed=12345
+
+	[fast]
+	cgroup=blktests/fast
+
+	[slow]
+	cgroup=blktests/slow
+EOF
+	# We run the test once so we have an idea of how fast this workload will
+	# go with nobody else doing IO on the device.
+	if ! fio ${fio_args} ${fio_config_single}; then
+		echo "fio exited with status $?"
+		return 1
+	fi
+
+	_time_taken=$(_fio_results_key fast job_runtime ${fio_results})
+	if [ "${_time_taken}" = "" ]; then
+		echo "fio doesn't report job_runtime"
+		return 1
+	fi
+
+	echo "normal time taken ${_time_taken}" >> $FULL
+
+	# There's no way to predict how the two workloads are going to affect
+	# each other, so we weant to set thresholds to something reasonable so
+	# we can verify io.latency is doing something.  This means we set 15%
+	# for the fast cgroup, just to give us enough wiggle room as throttling
+	# doesn't happen immediately.  But if we have a super fast disk we could
+	# run both groups really fast and make it under our fast threshold, so
+	# we need to set a threshold for the slow group at 50%.  We assume that
+	# if it was faster than 50% of the fast threshold then we probably
+	# didn't throttle and we can assume io.latency is broken.
+	_fast_thresh=$((${_time_taken} + ${_time_taken} * 15 / 100))
+	_slow_thresh=$((${_time_taken} + ${_time_taken} * 50 / 100))
+	echo "fast threshold time is ${_fast_thresh}" >> $FULL
+	echo "slow threshold time is ${_slow_thresh}" >> $FULL
+
+	# Create teh cgroup files
+	_dir=$(_cgroup2_base_dir)/blktests
+	echo "+io" > ${_dir}/cgroup.subtree_control
+	mkdir ${_dir}/fast
+	mkdir ${_dir}/slow
+
+	# We set teh target to 1usec because we could have a fast device that is
+	# capable of remarkable IO latencies that would skew the test.  It needs
+	# to be low enough that we do actually throttle the slow group,
+	# otherwise the test will fail when there's nothing wrong.
+	_major=$((0x$(stat -c "%t" ${TEST_DEV})))
+	_minor=$((0x$(stat -c "%T" ${TEST_DEV})))
+	echo "${_major}:${_minor} is our device" >> $FULL
+	if ! echo "${_major}:${_minor} target=1" > ${_dir}/fast/io.latency; then
+		echo "Failed to set our latency target"
+		return 1
+	fi
+
+	if ! fio ${fio_args} ${fio_config_double}; then
+		echo "fio exited with status $?"
+		return 1
+	fi
+
+	_fast_time=$(_fio_results_key fast job_runtime ${fio_results})
+	echo "Fast time ${_fast_time}" >> $FULL
+	_slow_time=$(_fio_results_key slow job_runtime ${fio_results})
+	echo "Slow time ${_slow_time}" >> $FULL
+	
+	if [ ${_fast_thresh} -lt ${_fast_time} ]; then
+		echo "Too much of a performance drop for the protected workload"
+		return 1
+	fi
+
+	if [ ${_slow_thresh} -gt ${_slow_time} ]; then
+		echo "The slow group does not appear to have been throttled"
+		return 1
+	fi
+
+	echo "Silence is golden"
+	return 0
+}
diff --git a/tests/block/025.out b/tests/block/025.out
new file mode 100644
index 000000000000..c19ff631b9b5
--- /dev/null
+++ b/tests/block/025.out
@@ -0,0 +1 @@
+Silence is golden
-- 
2.14.3

Re: [PATCH 3/3] blktests: block/025: an io.latency test

[Federico Motta]

[-- Attachment #1.1: Type: text/plain, Size: 5638 bytes --]

On 12/4/18 6:47 PM, Josef Bacik wrote:
> This is a test to verify io.latency is working properly.  It does this
> by first running a fio job by itself to figure out how fast it runs.
> Then we calculate some thresholds, set up 2 cgroups, a fast and a slow
> cgroup, and then run the same job in both groups simultaneously.  We
> should see the slow group get throttled until the first cgroup is able
> to finish, and then the slow cgroup will be allowed to finish.
> 
Hi,
two small typos below.

> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  tests/block/025     | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/block/025.out |   1 +
>  2 files changed, 134 insertions(+)
>  create mode 100644 tests/block/025
>  create mode 100644 tests/block/025.out
> 
> diff --git a/tests/block/025 b/tests/block/025
> new file mode 100644
> index 000000000000..88ce7cca944e
> --- /dev/null
> +++ b/tests/block/025
> @@ -0,0 +1,133 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +#
> +# Test io.latency to make sure it's protecting the higher priority group
> +# properly.
> +
> +. tests/block/rc
> +
> +DESCRIPTION="test the io.latency interface to make sure it's working right"
> +
> +requires() {
> +	_have_cgroup2_controller_file io io.latency && _have_fio && \
> +		_have_program python
> +}
> +
> +_fio_results_key() {
> +	_job=$1
> +	_key=$2
> +	_resultfile=$3
> +
> +	python src/fio-key-value.py -k $_key -j $_job $_resultfile
> +}
> +
> +test_device() {
> +	local fio_config_single fio_config_double fio_results fio_args qd
> +
> +	fio_config_single=${TMPDIR}/single.fio
> +	fio_config_double=${TMPDIR}/double.fio
> +	fio_results=${TMPDIR}/025.json
> +	fio_args="--output-format=json --output=${fio_results}"
> +	qd=$(cat ${TEST_DEV_SYSFS}/queue/nr_requests)
> +
> +	cat << EOF > "${fio_config_single}"
> +	[fast]
> +	filename=${TEST_DEV}
> +	direct=1
> +	allrandrepeat=1
> +	readwrite=randrw
> +	size=4G
> +	ioengine=libaio
> +	iodepth=${qd}
> +	fallocate=none
> +	randseed=12345
> +EOF
> +
> +	cat << EOF > "${fio_config_double}"
> +	[global]
> +	filename=${TEST_DEV}
> +	direct=1
> +	allrandrepeat=1
> +	readwrite=randrw
> +	size=4G
> +	ioengine=libaio
> +	iodepth=${qd}
> +	fallocate=none
> +	randseed=12345
> +
> +	[fast]
> +	cgroup=blktests/fast
> +
> +	[slow]
> +	cgroup=blktests/slow
> +EOF
> +	# We run the test once so we have an idea of how fast this workload will
> +	# go with nobody else doing IO on the device.
> +	if ! fio ${fio_args} ${fio_config_single}; then
> +		echo "fio exited with status $?"
> +		return 1
> +	fi
> +
> +	_time_taken=$(_fio_results_key fast job_runtime ${fio_results})
> +	if [ "${_time_taken}" = "" ]; then
> +		echo "fio doesn't report job_runtime"
> +		return 1
> +	fi
> +
> +	echo "normal time taken ${_time_taken}" >> $FULL
> +
> +	# There's no way to predict how the two workloads are going to affect
> +	# each other, so we weant to set thresholds to something reasonable so
> +	# we can verify io.latency is doing something.  This means we set 15%
> +	# for the fast cgroup, just to give us enough wiggle room as throttling
> +	# doesn't happen immediately.  But if we have a super fast disk we could
> +	# run both groups really fast and make it under our fast threshold, so
> +	# we need to set a threshold for the slow group at 50%.  We assume that
> +	# if it was faster than 50% of the fast threshold then we probably
> +	# didn't throttle and we can assume io.latency is broken.
> +	_fast_thresh=$((${_time_taken} + ${_time_taken} * 15 / 100))
> +	_slow_thresh=$((${_time_taken} + ${_time_taken} * 50 / 100))
> +	echo "fast threshold time is ${_fast_thresh}" >> $FULL
> +	echo "slow threshold time is ${_slow_thresh}" >> $FULL
> +
> +	# Create teh cgroup files
Typo, "the".      ^

> +	_dir=$(_cgroup2_base_dir)/blktests
> +	echo "+io" > ${_dir}/cgroup.subtree_control
> +	mkdir ${_dir}/fast
> +	mkdir ${_dir}/slow
> +
> +	# We set teh target to 1usec because we could have a fast device that is
Also here, "the". ^

> +	# capable of remarkable IO latencies that would skew the test.  It needs
> +	# to be low enough that we do actually throttle the slow group,
> +	# otherwise the test will fail when there's nothing wrong.
> +	_major=$((0x$(stat -c "%t" ${TEST_DEV})))
> +	_minor=$((0x$(stat -c "%T" ${TEST_DEV})))
> +	echo "${_major}:${_minor} is our device" >> $FULL
> +	if ! echo "${_major}:${_minor} target=1" > ${_dir}/fast/io.latency; then
> +		echo "Failed to set our latency target"
> +		return 1
> +	fi
> +
> +	if ! fio ${fio_args} ${fio_config_double}; then
> +		echo "fio exited with status $?"
> +		return 1
> +	fi
> +
> +	_fast_time=$(_fio_results_key fast job_runtime ${fio_results})
> +	echo "Fast time ${_fast_time}" >> $FULL
> +	_slow_time=$(_fio_results_key slow job_runtime ${fio_results})
> +	echo "Slow time ${_slow_time}" >> $FULL
> +	
There is a trailing whitespace in the above line.

> +	if [ ${_fast_thresh} -lt ${_fast_time} ]; then
> +		echo "Too much of a performance drop for the protected workload"
> +		return 1
> +	fi
> +
> +	if [ ${_slow_thresh} -gt ${_slow_time} ]; then
> +		echo "The slow group does not appear to have been throttled"
> +		return 1
> +	fi
> +
> +	echo "Silence is golden"
> +	return 0
> +}
> diff --git a/tests/block/025.out b/tests/block/025.out
> new file mode 100644
> index 000000000000..c19ff631b9b5
> --- /dev/null
> +++ b/tests/block/025.out
> @@ -0,0 +1 @@
> +Silence is golden
> 

Thank you,
Federico


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] blktests: add python scripts for parsing fio json output

[Federico Motta]

[-- Attachment #1.1: Type: text/plain, Size: 5286 bytes --]

On 12/4/18 6:47 PM, Josef Bacik wrote:
> I wrote these scripts for xfstests, just copying them over to blktests
> as well.  The terse output fio support that blktests doesn't get all of
> the various fio performance things that we may want to look at, this
> gives us a lot of flexibility around getting specific values out of the
> fio results data.
> 
Hi,
some suggestions below :)

> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  src/FioResultDecoder.py | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>  src/fio-key-value.py    | 28 ++++++++++++++++++++++
>  2 files changed, 92 insertions(+)
>  create mode 100644 src/FioResultDecoder.py
>  create mode 100644 src/fio-key-value.py
> 
> diff --git a/src/FioResultDecoder.py b/src/FioResultDecoder.py
> new file mode 100644
> index 000000000000..d004140c0fdf
> --- /dev/null
> +++ b/src/FioResultDecoder.py
> @@ -0,0 +1,64 @@
I think a shebang could clarify this runs under python2.

> +# SPDX-License-Identifier: GPL-2.0
> +
> +import json
> +
> +class FioResultDecoder(json.JSONDecoder):
> +    """Decoder for decoding fio result json to an object for our database
> +
> +    This decodes the json output from fio into an object that can be directly
> +    inserted into our database.  This just strips out the fields we don't care
> +    about and collapses the read/write/trim classes into a flat value structure
> +    inside of the jobs object.
> +
> +    For example
> +        "write" : {
> +            "io_bytes" : 313360384,
> +            "bw" : 1016,
> +        }
> +
> +    Get's collapsed to
> +
> +        "write_io_bytes" : 313360384,
> +        "write_bw": 1016,
> +
> +    Currently any dict under 'jobs' get's dropped, with the exception of 'read',
> +    'write', and 'trim'.  For those sub sections we drop any dict's under those.
> +
> +    Attempt to keep this as generic as possible, we don't want to break every
> +    time fio changes it's json output format.
> +    """
> +    _ignore_types = ['dict', 'list']
> +    _override_keys = ['lat_ns', 'lat']
> +    _io_ops = ['read', 'write', 'trim']
> +
> +    _transform_keys = { 'lat': 'lat_ns' }
> +
> +    def decode(self, json_string):
> +        """This does the dirty work of converting everything"""
> +        default_obj = super(FioResultDecoder, self).decode(json_string)
> +        obj = {}
> +        obj['global'] = {}
> +        obj['global']['time'] = default_obj['time']
> +        obj['jobs'] = []
> +        for job in default_obj['jobs']:
> +            new_job = {}
> +            for key,value in job.iteritems():
> +                if key not in self._io_ops:
> +                    if value.__class__.__name__ in self._ignore_types:
> +                        continue
> +                    new_job[key] = value
> +                    continue
> +                for k,v in value.iteritems():
> +                    if k in self._override_keys:
> +                        if k in self._transform_keys:
> +                            k = self._transform_keys[k]
> +                        for subk,subv in v.iteritems():
> +                            collapsed_key = "{}_{}_{}".format(key, k, subk)
> +                            new_job[collapsed_key] = subv
> +                        continue
> +                    if v.__class__.__name__ in self._ignore_types:
> +                        continue
> +                    collapsed_key = "{}_{}".format(key, k)
> +                    new_job[collapsed_key] = v
> +            obj['jobs'].append(new_job)
> +        return obj
> diff --git a/src/fio-key-value.py b/src/fio-key-value.py
> new file mode 100644
> index 000000000000..208e9a453a19
> --- /dev/null
> +++ b/src/fio-key-value.py
> @@ -0,0 +1,28 @@
Also here a shebang could be fine.

> +# SPDX-License-Identifier: GPL-2.0
> +
> +import FioResultDecoder
> +import json
> +import argparse
> +import sys
> +import platform
> +
> +parser = argparse.ArgumentParser()
> +parser.add_argument('-j', '--jobname', type=str,
> +                    help="The jobname we want our key from.",
> +                    required=True)
> +parser.add_argument('-k', '--key', type=str,
> +                    help="The key we want the value of", required=True)
> +parser.add_argument('result', type=str,
If here you set type=open              ^

> +                    help="The result file read")
> +args = parser.parse_args()
> +
> +json_data = open(args.result)
you could avoid this line ^

> +data = json.load(json_data, cls=FioResultDecoder.FioResultDecoder)
and substitute json_data with args.result which would return a file object.

In short the above piece of patch would become something like:

parser.add_argument('result', type=open, help="The result file read")
args = parser.parse_args()
data = json.load(args.result, cls=FioResultDecoder.FioResultDecoder)

Which still raises IOError if bad arguments are passed.

> +
> +for job in data['jobs']:
> +    if job['jobname'] == args.jobname:
> +        if args.key not in job:
> +            print('')
> +        else:
> +            print("{}".format(job[args.key]))
> +        break
> 


Have a nice day,
Federico


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] blktests: add python scripts for parsing fio json output

[Johannes Thumshirn]

On 04/12/2018 18:47, Josef Bacik wrote:
> +    For example
> +        "write" : {
> +            "io_bytes" : 313360384,
> +            "bw" : 1016,
> +        }
> +
> +    Get's collapsed to
> +
> +        "write_io_bytes" : 313360384,
> +        "write_bw": 1016,

I'd definitively prefer to not pull in python as a dependency. The above
example can be done in shell (with the jq utility) as follows:

jthumshirn@linux-x5ow:~$ cat test.json |\
   jq '. | {write_io_bytes: .write.io_bytes, write_bw: .write.bw}'
{
  "write_io_bytes": 313360384,
  "write_bw": 1016
}

And on a plus side jq is small as well:

jthumshirn@linux-x5ow:~$ zypper info jq
Loading repository data...
Reading installed packages...


Information for package jq:
---------------------------
Repository     : openSUSE:Leap:15.0
Name           : jq
Version        : 1.5-lp150.1.10
Arch           : x86_64
Vendor         : openSUSE
Installed Size : 93.4 KiB
Installed      : Yes
[...]

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 1/3] blktests: add cgroup2 infrastructure

[Bart Van Assche]

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.

Re: [PATCH 1/3] blktests: add cgroup2 infrastructure

[Bart Van Assche]

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.

Re: [PATCH 1/3] blktests: add cgroup2 infrastructure

[Omar Sandoval]

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

Re: [PATCH 1/3] blktests: add cgroup2 infrastructure

[Bart Van Assche]

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.

Re: [PATCH 1/3] blktests: add cgroup2 infrastructure

[Josef Bacik]

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