| Message ID | 20170623142951.17189-2-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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?
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.
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 >
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.
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.
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.
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