All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH blktests] Add surprise removal block test
@ 2018-04-24 21:41 Keith Busch
  2018-04-24 22:04 ` Johannes Thumshirn
  2018-04-25 15:56 ` Omar Sandoval
  0 siblings, 2 replies; 5+ messages in thread
From: Keith Busch @ 2018-04-24 21:41 UTC (permalink / raw)
  To: Omar Sandoval, linux-block; +Cc: Johannes Thumshirn, Keith Busch

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 common/rc       | 17 +++++++++++++++++
 tests/block/016 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)
 create mode 100755 tests/block/016

diff --git a/common/rc b/common/rc
index 1bd0374..979c8c2 100644
--- a/common/rc
+++ b/common/rc
@@ -171,6 +171,23 @@ _get_pci_dev_from_blkdev() {
 		tail -1
 }
 
+_get_pci_parent_from_blkdev() {
+	readlink -f "$TEST_DEV_SYSFS/device" | \
+		grep -Eo '[0-9a-f]{4,5}:[0-9a-f]{2}:[0-9a-f]{2}\.[0-9a-f]' | \
+		tail -2 | head -1
+}
+
+_test_hotplug_slot() {
+	parent="$(_get_pci_parent_from_blkdev)"
+
+	slt_cap=$(setpci -s ${parent} CAP_EXP+14.w)
+	if [ $((0x${slt_cap} & 0x20)) -eq 0 ]; then
+		SKIP_REASON="$TEST_DEV is not in a hot pluggable slot"
+		return 1
+	fi
+	return 0
+}
+
 # Older versions of xfs_io use pwrite64 and such, so the error messages won't
 # match current versions of xfs_io. See c52086226bc6 ("filter: xfs_io output
 # has dropped "64" from error messages") in xfstests.
diff --git a/tests/block/016 b/tests/block/016
new file mode 100755
index 0000000..4c2e294
--- /dev/null
+++ b/tests/block/016
@@ -0,0 +1,52 @@
+#!/bin/bash
+#
+# Do disable PCI device while doing I/O to it
+#
+# Copyright (C) 2018 Keith Busch <keith.busch@intel.com>
+#
+# 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="break PCI link while doing I/O"
+TIMED=1
+
+requires() {
+	_have_fio
+}
+
+device_requires() {
+	_test_dev_is_pci
+	_test_hotplug_slot
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+
+	parent="$(_get_pci_parent_from_blkdev)"
+
+	if _test_dev_is_rotational; then
+		size="32m"
+	else
+		size="1g"
+	fi
+
+	# start fio job
+	_run_fio_rand_io --filename="$TEST_DEV" --size="$size" \
+			--ignore_error=EIO,ENXIO,ENODEV &
+
+	setpci -s ${parent} CAP_EXP+10.w=10:10
+	sleep 10
+	setpci -s ${parent} CAP_EXP+10.w=00:10
+
+	echo "Test complete"
+}
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH blktests] Add surprise removal block test
  2018-04-24 21:41 [PATCH blktests] Add surprise removal block test Keith Busch
@ 2018-04-24 22:04 ` Johannes Thumshirn
  2018-04-24 22:09   ` Keith Busch
  2018-04-25 15:56 ` Omar Sandoval
  1 sibling, 1 reply; 5+ messages in thread
From: Johannes Thumshirn @ 2018-04-24 22:04 UTC (permalink / raw)
  To: Keith Busch; +Cc: Omar Sandoval, linux-block

On Tue, Apr 24, 2018 at 03:41:46PM -0600, Keith Busch wrote:

[...]

> +DESCRIPTION="break PCI link while doing I/O"
> +TIMED=1

I _think_ we can set QUICK=1 here, Omar?

> +
> +requires() {
> +	_have_fio
> +}

You're using setpci below, so please add it to requires,
i.e. _have_fio && _have_program setpci

> +
> +device_requires() {
> +	_test_dev_is_pci
> +	_test_hotplug_slot
> +}

I think this sould be _test_dev_is_pci && _test_hotplug_slot


-- 
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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH blktests] Add surprise removal block test
  2018-04-24 22:04 ` Johannes Thumshirn
@ 2018-04-24 22:09   ` Keith Busch
  0 siblings, 0 replies; 5+ messages in thread
From: Keith Busch @ 2018-04-24 22:09 UTC (permalink / raw)
  To: Johannes Thumshirn; +Cc: Omar Sandoval, linux-block

On Tue, Apr 24, 2018 at 04:04:22PM -0600, Johannes Thumshirn wrote:
> On Tue, Apr 24, 2018 at 03:41:46PM -0600, Keith Busch wrote:
> 
> [...]
> 
> > +DESCRIPTION="break PCI link while doing I/O"
> > +TIMED=1
> 
> I _think_ we can set QUICK=1 here, Omar?
> 
> > +
> > +requires() {
> > +	_have_fio
> > +}
> 
> You're using setpci below, so please add it to requires,
> i.e. _have_fio && _have_program setpci
> 
> > +
> > +device_requires() {
> > +	_test_dev_is_pci
> > +	_test_hotplug_slot
> > +}
> 
> I think this sould be _test_dev_is_pci && _test_hotplug_slot

Sounds good. Thanks for the feedback!

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH blktests] Add surprise removal block test
  2018-04-24 21:41 [PATCH blktests] Add surprise removal block test Keith Busch
  2018-04-24 22:04 ` Johannes Thumshirn
@ 2018-04-25 15:56 ` Omar Sandoval
  2018-04-25 17:49   ` Keith Busch
  1 sibling, 1 reply; 5+ messages in thread
From: Omar Sandoval @ 2018-04-25 15:56 UTC (permalink / raw)
  To: Keith Busch; +Cc: linux-block, Johannes Thumshirn

On Tue, Apr 24, 2018 at 03:41:46PM -0600, Keith Busch wrote:
> Signed-off-by: Keith Busch <keith.busch@intel.com>

Hey, Keith, thanks for the test! Some comments/questions below.

> ---
>  common/rc       | 17 +++++++++++++++++
>  tests/block/016 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 69 insertions(+)
>  create mode 100755 tests/block/016
> 
> diff --git a/common/rc b/common/rc
> index 1bd0374..979c8c2 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -171,6 +171,23 @@ _get_pci_dev_from_blkdev() {
>  		tail -1
>  }
>  
> +_get_pci_parent_from_blkdev() {
> +	readlink -f "$TEST_DEV_SYSFS/device" | \
> +		grep -Eo '[0-9a-f]{4,5}:[0-9a-f]{2}:[0-9a-f]{2}\.[0-9a-f]' | \
> +		tail -2 | head -1
> +}
> +
> +_test_hotplug_slot() {

I'd call this _test_dev_in_hotplug_slot().

> +	parent="$(_get_pci_parent_from_blkdev)"

I haven't been consistent about asking people to do this, but could you
make these variables local? I.e.,

local parent
parent="$(_get_pci_parent_from_blkdev)"
local slt_cap
slt_cap=...

> +
> +	slt_cap=$(setpci -s ${parent} CAP_EXP+14.w)

Missing quotes around "${parent}" here (and elsewhere). `make
shellcheck` will catch this.

> +	if [ $((0x${slt_cap} & 0x20)) -eq 0 ]; then
> +		SKIP_REASON="$TEST_DEV is not in a hot pluggable slot"
> +		return 1
> +	fi
> +	return 0
> +}
> +
>  # Older versions of xfs_io use pwrite64 and such, so the error messages won't
>  # match current versions of xfs_io. See c52086226bc6 ("filter: xfs_io output
>  # has dropped "64" from error messages") in xfstests.
> diff --git a/tests/block/016 b/tests/block/016
> new file mode 100755
> index 0000000..4c2e294
> --- /dev/null
> +++ b/tests/block/016
> @@ -0,0 +1,52 @@
> +#!/bin/bash
> +#
> +# Do disable PCI device while doing I/O to it
> +#
> +# Copyright (C) 2018 Keith Busch <keith.busch@intel.com>
> +#
> +# 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="break PCI link while doing I/O"
> +TIMED=1
> +
> +requires() {
> +	_have_fio
> +}
> +
> +device_requires() {
> +	_test_dev_is_pci
> +	_test_hotplug_slot
> +}
> +
> +test_device() {
> +	echo "Running ${TEST_NAME}"
> +
> +	parent="$(_get_pci_parent_from_blkdev)"
> +
> +	if _test_dev_is_rotational; then
> +		size="32m"
> +	else
> +		size="1g"
> +	fi
> +
> +	# start fio job
> +	_run_fio_rand_io --filename="$TEST_DEV" --size="$size" \
> +			--ignore_error=EIO,ENXIO,ENODEV &
> +
> +	setpci -s ${parent} CAP_EXP+10.w=10:10
> +	sleep 10
> +	setpci -s ${parent} CAP_EXP+10.w=00:10

For the sake of people of me who don't speak PCI, what do each of these
commands do? :) Should we make the fio job --time_based instead of using
--size so that we're sure it runs long enough for the sleep?

> +	echo "Test complete"
> +}
> -- 
> 2.14.3
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH blktests] Add surprise removal block test
  2018-04-25 15:56 ` Omar Sandoval
@ 2018-04-25 17:49   ` Keith Busch
  0 siblings, 0 replies; 5+ messages in thread
From: Keith Busch @ 2018-04-25 17:49 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-block, Johannes Thumshirn

On Wed, Apr 25, 2018 at 08:56:02AM -0700, Omar Sandoval wrote:
> On Tue, Apr 24, 2018 at 03:41:46PM -0600, Keith Busch wrote:
> > +_test_hotplug_slot() {
> 
> I'd call this _test_dev_in_hotplug_slot().

Sounds good.
 
> > +	parent="$(_get_pci_parent_from_blkdev)"
> 
> I haven't been consistent about asking people to do this, but could you
> make these variables local? I.e.,
> 
> local parent
> parent="$(_get_pci_parent_from_blkdev)"
> local slt_cap
> slt_cap=...

No problem.

> > +	setpci -s ${parent} CAP_EXP+10.w=10:10
> > +	sleep 10
> > +	setpci -s ${parent} CAP_EXP+10.w=00:10
> 
> For the sake of people of me who don't speak PCI, what do each of these
> commands do? :)

:) Will add to the change log.

This command finds the PCI Express Capability register of the slot the
device is in, then at offset 0x10 (the Link Control Register) writes a
1 to bit 4 (Link Disable). This is happening unbeknownst to any of the
drivers, just like a surprise removal. If this is a capable slot, the
drivers will find out about this through the pcie hotplug handler.

> Should we make the fio job --time_based instead of using
> --size so that we're sure it runs long enough for the sleep?

Right, that makes sense.

Will resend with the fix-ups, but probably not until tomorrow.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-04-25 17:47 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-24 21:41 [PATCH blktests] Add surprise removal block test Keith Busch
2018-04-24 22:04 ` Johannes Thumshirn
2018-04-24 22:09   ` Keith Busch
2018-04-25 15:56 ` Omar Sandoval
2018-04-25 17:49   ` Keith Busch

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.