[PATCH blktests 1/2] rc: add helpers to handle PCI test devices

[PATCH blktests 1/2] rc: add helpers to handle PCI test devices

[Johannes Thumshirn]

Add two helpers to check whether a device is attached via PCI and to get the
PCI device from a TEST_DEV

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 common/rc | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/common/rc b/common/rc
index b01f936b878b..497cf81ec475 100644
--- a/common/rc
+++ b/common/rc
@@ -120,3 +120,18 @@ _test_dev_queue_set() {
 	fi
 	echo "$2" >"${TEST_DEV_SYSFS}/queue/$1"
 }
+
+_test_dev_is_pci() {
+	if ! readlink -f "$TEST_DEV_SYSFS/device" | grep -q pci; then
+		SKIP_REASON="$TEST_DEV is not a PCI device"
+		return 1
+	fi
+	return 0
+}
+
+_get_pci_dev_from_blkdev() {
+	pdev="$(readlink -f "$TEST_DEV_SYSFS/device" | \
+		grep -Eo '[0-9a-f]{4}:[0-9a-f]{2}:[0-9a-f]{2}\.[0-9a-f]')"
+
+	echo "$pdev"
+}
-- 
2.12.3

[PATCH blktests 2/2] block/011: Perform PCI reset while doing IO

[Johannes Thumshirn]

From: Omar Sandoval <osandov@fb.com>

This test-case performs I/O with fio while doing PCI disable/enable
cycles.

In the results we don't care for I/O errors but for hiccups in dmesg only.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 tests/block/011     | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/block/011.out |  2 ++
 2 files changed, 56 insertions(+)
 create mode 100755 tests/block/011
 create mode 100644 tests/block/011.out

diff --git a/tests/block/011 b/tests/block/011
new file mode 100755
index 000000000000..b0de35816d48
--- /dev/null
+++ b/tests/block/011
@@ -0,0 +1,54 @@
+#!/bin/bash
+#
+# Do disable PCI device while doing I/O to it
+#
+# Copyright (C) 2017 Johannes Thumshirn <jthumshirn@suse.de>
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+DESCRIPTION="disable PCI device while doing I/O"
+TIMED=1
+
+requires() {
+	_have_fio
+}
+
+device_requires() {
+	_test_dev_is_pci
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+
+	pdev=$(_get_pci_dev_from_blkdev)
+
+	if _test_dev_is_rotational; then
+		size="32m"
+	else
+		size="1g"
+	fi
+
+	# start fio job
+	_run_fio --bs=4k --rw=randread --norandommap \
+		--name=reads --filename="$TEST_DEV" --size="$size" \
+		--numjobs=8 --direct=1 2>/dev/null &
+
+	while kill -0 $! 2>/dev/null; do
+		echo 0 > "/sys/bus/pci/devices/${pdev}/enable"
+		sleep .2
+		echo 1 > "/sys/bus/pci/devices/${pdev}/enable"
+	done
+
+	echo "Test complete"
+}
diff --git a/tests/block/011.out b/tests/block/011.out
new file mode 100644
index 000000000000..8e067df63097
--- /dev/null
+++ b/tests/block/011.out
@@ -0,0 +1,2 @@
+Running block/011
+Test complete
-- 
2.12.3

Re: [PATCH blktests 2/2] block/011: Perform PCI reset while doing IO

[Jens Axboe]

On 06/23/2017 08:29 AM, Johannes Thumshirn wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This test-case performs I/O with fio while doing PCI disable/enable
> cycles.
> 
> In the results we don't care for I/O errors but for hiccups in dmesg only.

Let's get this in, that would be a very useful test. A few comments -
not necessarily on this patch in particular, but for future cleanups
and improvements.

> +	if _test_dev_is_rotational; then
> +		size="32m"
> +	else
> +		size="1g"
> +	fi

I introduced this idea in one of my previous patches. I wonder if we
should turn that into a helper. Pass in the dev, get returned a
suitable fio size, instead of hard coding this in each job that
needs it.

> +	# start fio job
> +	_run_fio --bs=4k --rw=randread --norandommap \
> +		--name=reads --filename="$TEST_DEV" --size="$size" \
> +		--numjobs=8 --direct=1 2>/dev/null &

I don't believe we check for fio errors right now, but we probably
should in the future. So I think you'd want to add something ala:

--ignore_error=EIO,ENXIO,ENODEV

to your options to make it explicit that you don't care about IO
errors for this test.


-- 
Jens Axboe

Re: [PATCH blktests 2/2] block/011: Perform PCI reset while doing IO

[Johannes Thumshirn]

On Fri, Jun 23, 2017 at 09:36:14AM -0600, Jens Axboe wrote:
> On 06/23/2017 08:29 AM, Johannes Thumshirn wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > This test-case performs I/O with fio while doing PCI disable/enable
> > cycles.
> > 
> > In the results we don't care for I/O errors but for hiccups in dmesg only.
> 
> Let's get this in, that would be a very useful test. A few comments -
> not necessarily on this patch in particular, but for future cleanups
> and improvements.
> 
> > +	if _test_dev_is_rotational; then
> > +		size="32m"
> > +	else
> > +		size="1g"
> > +	fi
> 
> I introduced this idea in one of my previous patches. I wonder if we
> should turn that into a helper. Pass in the dev, get returned a
> suitable fio size, instead of hard coding this in each job that
> needs it.

Sure.

> 
> > +	# start fio job
> > +	_run_fio --bs=4k --rw=randread --norandommap \
> > +		--name=reads --filename="$TEST_DEV" --size="$size" \
> > +		--numjobs=8 --direct=1 2>/dev/null &
> 
> I don't believe we check for fio errors right now, but we probably
> should in the future. So I think you'd want to add something ala:
> 
> --ignore_error=EIO,ENXIO,ENODEV
> 
> to your options to make it explicit that you don't care about IO
> errors for this test.

Oh nice, didn't know about the option. Btw as we're currently all have
arbitrary values for the numjobs parameter, how about a wrapper over getconf
_NPROCESSORS_ONLN?

-- 
Johannes Thumshirn                                          Storage
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 blktests 2/2] block/011: Perform PCI reset while doing IO

[Jens Axboe]

On 06/26/2017 08:06 AM, Johannes Thumshirn wrote:
> On Fri, Jun 23, 2017 at 09:36:14AM -0600, Jens Axboe wrote:
>> On 06/23/2017 08:29 AM, Johannes Thumshirn wrote:
>>> From: Omar Sandoval <osandov@fb.com>
>>>
>>> This test-case performs I/O with fio while doing PCI disable/enable
>>> cycles.
>>>
>>> In the results we don't care for I/O errors but for hiccups in dmesg only.
>>
>> Let's get this in, that would be a very useful test. A few comments -
>> not necessarily on this patch in particular, but for future cleanups
>> and improvements.
>>
>>> +	if _test_dev_is_rotational; then
>>> +		size="32m"
>>> +	else
>>> +		size="1g"
>>> +	fi
>>
>> I introduced this idea in one of my previous patches. I wonder if we
>> should turn that into a helper. Pass in the dev, get returned a
>> suitable fio size, instead of hard coding this in each job that
>> needs it.
> 
> Sure.
> 
>>
>>> +	# start fio job
>>> +	_run_fio --bs=4k --rw=randread --norandommap \
>>> +		--name=reads --filename="$TEST_DEV" --size="$size" \
>>> +		--numjobs=8 --direct=1 2>/dev/null &
>>
>> I don't believe we check for fio errors right now, but we probably
>> should in the future. So I think you'd want to add something ala:
>>
>> --ignore_error=EIO,ENXIO,ENODEV
>>
>> to your options to make it explicit that you don't care about IO
>> errors for this test.
> 
> Oh nice, didn't know about the option. Btw as we're currently all have
> arbitrary values for the numjobs parameter, how about a wrapper over getconf
> _NPROCESSORS_ONLN?

Yes that's a good idea, then we can at least size the jobs based on
how many cores we have.

-- 
Jens Axboe

Re: [PATCH blktests 2/2] block/011: Perform PCI reset while doing IO

[Omar Sandoval]

On Mon, Jun 26, 2017 at 08:25:36AM -0600, Jens Axboe wrote:
> On 06/26/2017 08:06 AM, Johannes Thumshirn wrote:
> > On Fri, Jun 23, 2017 at 09:36:14AM -0600, Jens Axboe wrote:
> >> On 06/23/2017 08:29 AM, Johannes Thumshirn wrote:
> >>> From: Omar Sandoval <osandov@fb.com>
> >>>
> >>> This test-case performs I/O with fio while doing PCI disable/enable
> >>> cycles.
> >>>
> >>> In the results we don't care for I/O errors but for hiccups in dmesg only.
> >>
> >> Let's get this in, that would be a very useful test. A few comments -
> >> not necessarily on this patch in particular, but for future cleanups
> >> and improvements.
> >>
> >>> +	if _test_dev_is_rotational; then
> >>> +		size="32m"
> >>> +	else
> >>> +		size="1g"
> >>> +	fi
> >>
> >> I introduced this idea in one of my previous patches. I wonder if we
> >> should turn that into a helper. Pass in the dev, get returned a
> >> suitable fio size, instead of hard coding this in each job that
> >> needs it.

What I wanted to have here eventually is a helper that you can run when
you just want arbitrary I/O. Haven't gotten around to it.

> > 
> > Sure.
> > 
> >>
> >>> +	# start fio job
> >>> +	_run_fio --bs=4k --rw=randread --norandommap \
> >>> +		--name=reads --filename="$TEST_DEV" --size="$size" \
> >>> +		--numjobs=8 --direct=1 2>/dev/null &
> >>
> >> I don't believe we check for fio errors right now, but we probably
> >> should in the future. So I think you'd want to add something ala:
> >>
> >> --ignore_error=EIO,ENXIO,ENODEV
> >>
> >> to your options to make it explicit that you don't care about IO
> >> errors for this test.

Yup, we redirect fio errors to /dev/null everywhere, we should fix that.

> > Oh nice, didn't know about the option. Btw as we're currently all have
> > arbitrary values for the numjobs parameter, how about a wrapper over getconf
> > _NPROCESSORS_ONLN?
> 
> Yes that's a good idea, then we can at least size the jobs based on
> how many cores we have.

We can just use nproc for this.

Re: [PATCH blktests 2/2] block/011: Perform PCI reset while doing IO

[Jens Axboe]

On 06/26/2017 03:29 PM, Omar Sandoval wrote:
> On Mon, Jun 26, 2017 at 08:25:36AM -0600, Jens Axboe wrote:
>> On 06/26/2017 08:06 AM, Johannes Thumshirn wrote:
>>> On Fri, Jun 23, 2017 at 09:36:14AM -0600, Jens Axboe wrote:
>>>> On 06/23/2017 08:29 AM, Johannes Thumshirn wrote:
>>>>> From: Omar Sandoval <osandov@fb.com>
>>>>>
>>>>> This test-case performs I/O with fio while doing PCI disable/enable
>>>>> cycles.
>>>>>
>>>>> In the results we don't care for I/O errors but for hiccups in dmesg only.
>>>>
>>>> Let's get this in, that would be a very useful test. A few comments -
>>>> not necessarily on this patch in particular, but for future cleanups
>>>> and improvements.
>>>>
>>>>> +	if _test_dev_is_rotational; then
>>>>> +		size="32m"
>>>>> +	else
>>>>> +		size="1g"
>>>>> +	fi
>>>>
>>>> I introduced this idea in one of my previous patches. I wonder if we
>>>> should turn that into a helper. Pass in the dev, get returned a
>>>> suitable fio size, instead of hard coding this in each job that
>>>> needs it.
> 
> What I wanted to have here eventually is a helper that you can run when
> you just want arbitrary I/O. Haven't gotten around to it.

That would be handy. Until that happens, I would not worry about it,
it's not like it's a lot of work to just copy/paste the "do random
reads QD=x with Y jobs" between jobs. It'd be fine as a separate
cleanup at some point.


-- 
Jens Axboe

Re: [PATCH blktests 2/2] block/011: Perform PCI reset while doing IO

[Omar Sandoval]

On Fri, Jun 23, 2017 at 04:29:51PM +0200, Johannes Thumshirn wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This test-case performs I/O with fio while doing PCI disable/enable
> cycles.
> 
> In the results we don't care for I/O errors but for hiccups in dmesg only.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  tests/block/011     | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/block/011.out |  2 ++
>  2 files changed, 56 insertions(+)
>  create mode 100755 tests/block/011
>  create mode 100644 tests/block/011.out
> 
> diff --git a/tests/block/011 b/tests/block/011
> new file mode 100755
> index 000000000000..b0de35816d48
> --- /dev/null
> +++ b/tests/block/011
> @@ -0,0 +1,54 @@
> +#!/bin/bash
> +#
> +# Do disable PCI device while doing I/O to it
> +#
> +# Copyright (C) 2017 Johannes Thumshirn <jthumshirn@suse.de>
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +DESCRIPTION="disable PCI device while doing I/O"
> +TIMED=1
> +
> +requires() {
> +	_have_fio
> +}
> +
> +device_requires() {
> +	_test_dev_is_pci
> +}
> +
> +test_device() {
> +	echo "Running ${TEST_NAME}"
> +
> +	pdev=$(_get_pci_dev_from_blkdev)
> +
> +	if _test_dev_is_rotational; then
> +		size="32m"
> +	else
> +		size="1g"
> +	fi
> +
> +	# start fio job
> +	_run_fio --bs=4k --rw=randread --norandommap \
> +		--name=reads --filename="$TEST_DEV" --size="$size" \
> +		--numjobs=8 --direct=1 2>/dev/null &
> +
> +	while kill -0 $! 2>/dev/null; do
> +		echo 0 > "/sys/bus/pci/devices/${pdev}/enable"
> +		sleep .2
> +		echo 1 > "/sys/bus/pci/devices/${pdev}/enable"

Test looks good, but one question: do you want another sleep .2 here?
Like this, you immediately disable it after enabling it, but maybe
that's what you want :)

> +	done
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/block/011.out b/tests/block/011.out
> new file mode 100644
> index 000000000000..8e067df63097
> --- /dev/null
> +++ b/tests/block/011.out
> @@ -0,0 +1,2 @@
> +Running block/011
> +Test complete
> -- 
> 2.12.3
> 

Re: [PATCH blktests 2/2] block/011: Perform PCI reset while doing IO

[Johannes Thumshirn]

On Mon, Jun 26, 2017 at 02:27:24PM -0700, Omar Sandoval wrote:
[...]
> > +
> > +	while kill -0 $! 2>/dev/null; do
> > +		echo 0 > "/sys/bus/pci/devices/${pdev}/enable"
> > +		sleep .2
> > +		echo 1 > "/sys/bus/pci/devices/${pdev}/enable"
> 
> Test looks good, but one question: do you want another sleep .2 here?
> Like this, you immediately disable it after enabling it, but maybe
> that's what you want :)

Good catch. I'll fix it up.

-- 
Johannes Thumshirn                                          Storage
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 blktests 1/2] rc: add helpers to handle PCI test devices

[Omar Sandoval]

On Fri, Jun 23, 2017 at 04:29:50PM +0200, Johannes Thumshirn wrote:
> Add two helpers to check whether a device is attached via PCI and to get the
> PCI device from a TEST_DEV
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  common/rc | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index b01f936b878b..497cf81ec475 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -120,3 +120,18 @@ _test_dev_queue_set() {
>  	fi
>  	echo "$2" >"${TEST_DEV_SYSFS}/queue/$1"
>  }
> +
> +_test_dev_is_pci() {
> +	if ! readlink -f "$TEST_DEV_SYSFS/device" | grep -q pci; then
> +		SKIP_REASON="$TEST_DEV is not a PCI device"
> +		return 1
> +	fi
> +	return 0
> +}
> +
> +_get_pci_dev_from_blkdev() {
> +	pdev="$(readlink -f "$TEST_DEV_SYSFS/device" | \
> +		grep -Eo '[0-9a-f]{4}:[0-9a-f]{2}:[0-9a-f]{2}\.[0-9a-f]')"
> +
> +	echo "$pdev"
> +}

No need to do pdev=$(blah) and then echo $pdev, you can just do

_get_pci_dev_from_blkdev() {
	readlink -f "$TEST_DEV_SYSFS/device" | grep -Eo '[0-9a-f]{4}:[0-9a-f]{2}:[0-9a-f]{2}\.[0-9a-f]'
}

I'll fix that up when I commit it. Looks good otherwise.