* [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs
@ 2016-05-27 15:21 Zorro Lang
2016-05-27 15:21 ` [PATCH 2/2] xfs/006: new case to test xfs fail_at_unmount error handling Zorro Lang
2016-05-27 18:05 ` [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs Eric Sandeen
0 siblings, 2 replies; 10+ messages in thread
From: Zorro Lang @ 2016-05-27 15:21 UTC (permalink / raw)
To: fstests; +Cc: cem, Zorro Lang
XFS add more configurations in /sys/fs/xfs recently. For use
them, this patch add some common functions for:
1. "require" a file/dir in /sys/fs.
2. write a file in /sys/fs.
For common use, these functions can be used by other filesystems.
Signed-off-by: Zorro Lang <zlang@redhat.com>
---
common/rc | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 81 insertions(+)
diff --git a/common/rc b/common/rc
index 51092a0..9935f08 100644
--- a/common/rc
+++ b/common/rc
@@ -3556,6 +3556,87 @@ run_fsx()
fi
}
+_require_sys_fs()
+{
+ local dev=$1
+ local target=$2
+ local dname=""
+
+ if [ ! -b "$dev" -o -z "$target" ];then
+ echo "Usage: _require_sys_fs <device> <target_need_exist>"
+ exit 1
+ fi
+
+ dname=$(basename $(readlink -f $dev))
+ _mkfs_dev $dev > /dev/null 2>&1
+ _mount -t $FSTYP `_common_dev_mount_options` $dev $SCRATCH_MNT
+ if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
+ umount $SCRATCH_MNT
+ _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
+ fi
+ umount $SCRATCH_MNT
+}
+
+_require_scratch_sys_fs()
+{
+ local target=$1
+ local dname=""
+
+ if [ ! -b "$SCRATCH_DEV" -o -z "$target" ];then
+ echo "Usage: _require_scratch_sys_fs <target_need_exist>"
+ exit 1
+ fi
+
+ dname=$(basename $(readlink -f $SCRATCH_DEV))
+ _scratch_mkfs > /dev/null 2>&1
+ _scratch_mount
+ if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
+ _scratch_unmount
+ _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
+ fi
+ _scratch_unmount
+}
+
+_set_sys_fs_param()
+{
+ local dev=$1
+ shift
+ local target=$1
+ shift
+ local content="$*"
+
+ if [ ! -b "$dev" -o -z "$target" -o -z "$content" ];then
+ echo "Usage: _set_sys_fs_param <mounted_device> <target> <content>"
+ exit 1
+ fi
+ local dname=$(basename $(readlink -f $dev))
+ echo "$content" > /sys/fs/${FSTYP}/${dname}/$target
+}
+
+_enable_xfs_fail_at_unmount()
+{
+ local dev=$1
+
+ if [ ! -b "$dev" -o "$FSTYP" != "xfs" ];then
+ echo "Usage: _enable_xfs_fail_at_unmount <mounted_XFS_device>"
+ exit 1
+ fi
+
+ _set_sys_fs_param $dev error/fail_at_unmount 1
+}
+
+_disable_xfs_fail_at_unmount()
+{
+ local dev=$1
+
+ if [ ! -b "$dev" -o "$FSTYP" != "xfs" ];then
+ echo "Usage: _enable_xfs_fail_at_unmount <mounted_XFS_device>"
+ exit 1
+ fi
+
+ _set_sys_fs_param $dev error/fail_at_unmount 0
+}
+
init_rc
################################################################################
--
2.5.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] xfs/006: new case to test xfs fail_at_unmount error handling
2016-05-27 15:21 [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs Zorro Lang
@ 2016-05-27 15:21 ` Zorro Lang
2016-05-27 18:26 ` Eric Sandeen
2016-05-27 18:05 ` [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs Eric Sandeen
1 sibling, 1 reply; 10+ messages in thread
From: Zorro Lang @ 2016-05-27 15:21 UTC (permalink / raw)
To: fstests; +Cc: cem, Zorro Lang
XFS bring in a new configuration under /sys/fs/xfs/error, named
fail_at_unmount. It's used to stop unmount retrying forever when
it hit IO error.
This case try to unmount an faulty dm device, and to sure unmount
won't retry forever if fail_at_unmount=1.
Signed-off-by: Zorro Lang <zlang@redhat.com>
---
Hi,
Thanks Carlos told this easy way(use dm error) to test this feature, or
I will try to use dm thinp to reproduce unmount hit an ENOSPC error. That's
more difficult to do.
I haven't gotten some good ideas to test those /sys/fs/xfs/dm-X/error/
metadata/... configurations. If someone have good ideas, please let me
know:)
Thanks,
Zorro
tests/xfs/006 | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
tests/xfs/006.out | 2 ++
tests/xfs/group | 1 +
3 files changed, 83 insertions(+)
create mode 100755 tests/xfs/006
create mode 100644 tests/xfs/006.out
diff --git a/tests/xfs/006 b/tests/xfs/006
new file mode 100755
index 0000000..dafab50
--- /dev/null
+++ b/tests/xfs/006
@@ -0,0 +1,80 @@
+#! /bin/bash
+# FS QA Test 006
+#
+# Test xfs' "fail at unmount" error handling configuration. Stop
+# XFS retrying to writeback forever when unmount.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2016 YOUR NAME HERE. All Rights Reserved.
+#
+# 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.
+#
+# This program is distributed in the hope that it would 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, write the Free Software Foundation,
+# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1 # failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+ cd /
+ rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/dmerror
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+_supported_fs xfs
+_supported_os Linux
+_require_dm_target error
+_require_scratch
+_require_scratch_sys_fs error/fail_at_unmount
+
+# Format and mount
+# The device is still a linear device at here
+_scratch_mkfs > $seqres.full 2>&1
+_dmerror_init
+_dmerror_mount
+
+# Enable fail_at_unmount
+_enable_xfs_fail_at_unmount $DMERROR_DEV
+
+# make some IO at here. If these data still not writeback from
+# journal to data, it will hit writeback error after load error
+# tabel in next step.
+_pwrite_byte 0x61 0 100M $SCRATCH_MNT/file >/dev/null
+_dmerror_load_error_table
+
+# Even above data already all writeback, umount will cause XFS
+# try to writeback something to root inode. It still can trigger
+# umount fail.
+_dmerror_unmount
+
+echo "Silence is golden"
+_dmerror_cleanup
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/006.out b/tests/xfs/006.out
new file mode 100644
index 0000000..675c1b7
--- /dev/null
+++ b/tests/xfs/006.out
@@ -0,0 +1,2 @@
+QA output created by 006
+Silence is golden
diff --git a/tests/xfs/group b/tests/xfs/group
index f4c6816..39169ea 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -4,6 +4,7 @@
004 db auto quick
005 auto quick
007 auto quota quick
+006 auto quick unmount
008 rw ioctl auto quick
009 rw ioctl auto prealloc quick
010 auto quick repair
--
2.5.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs
2016-05-27 15:21 [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs Zorro Lang
2016-05-27 15:21 ` [PATCH 2/2] xfs/006: new case to test xfs fail_at_unmount error handling Zorro Lang
@ 2016-05-27 18:05 ` Eric Sandeen
2016-05-27 18:27 ` Eric Sandeen
2016-05-28 16:36 ` Zorro Lang
1 sibling, 2 replies; 10+ messages in thread
From: Eric Sandeen @ 2016-05-27 18:05 UTC (permalink / raw)
To: Zorro Lang, fstests; +Cc: cem
On 5/27/16 10:21 AM, Zorro Lang wrote:
> XFS add more configurations in /sys/fs/xfs recently. For use
> them, this patch add some common functions for:
> 1. "require" a file/dir in /sys/fs.
> 2. write a file in /sys/fs.
More specifically, in /sys/fs/${FSTYP}
> For common use, these functions can be used by other filesystems.
>
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
> common/rc | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 81 insertions(+)
>
> diff --git a/common/rc b/common/rc
> index 51092a0..9935f08 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3556,6 +3556,87 @@ run_fsx()
> fi
> }
>
> +_require_sys_fs()
> +{
> + local dev=$1
> + local target=$2
> + local dname=""
> +
> + if [ ! -b "$dev" -o -z "$target" ];then
> + echo "Usage: _require_sys_fs <device> <target_need_exist>"
> + exit 1
> + fi
> +
> + dname=$(basename $(readlink -f $dev))
> + _mkfs_dev $dev > /dev/null 2>&1
we don't want to mkfs the test dev just for a _require test; that
device is supposed to age, and this could possibly take a very long
time, as well. And in general a mkfs should not be required to test
the existence of a sysfs tunable, I think?
> + _mount -t $FSTYP `_common_dev_mount_options` $dev $SCRATCH_MNT
> + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
> + umount $SCRATCH_MNT
> + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
> + fi
> + umount $SCRATCH_MNT
> +}
> +
> +_require_scratch_sys_fs()
> +{
> + local target=$1
> + local dname=""
> +
> + if [ ! -b "$SCRATCH_DEV" -o -z "$target" ];then
> + echo "Usage: _require_scratch_sys_fs <target_need_exist>"
> + exit 1
> + fi
> +
> + dname=$(basename $(readlink -f $SCRATCH_DEV))
> + _scratch_mkfs > /dev/null 2>&1
> + _scratch_mount
> + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
> + _scratch_unmount
> + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
> + fi
> + _scratch_unmount
> +}
This seems overly complex; do you need both of these functions? We are only
testing one filesystem type at a time, and $TEST_DEV is always required.
What is the reason for the two differing functions?
Isn't _require_sys_fs $SCRATCH_DEV exactly the same as
_require_scratch_sys_fs ?
Also, because $SCRATCH_DEV is not required, $SCRATCH_MNT
isn't either, so I don't think you can use $SCRATCH_MNT.
Also :) We may one day want to require sysfs files which
are not in the fs/ namespace, so it should probably
be named according to the namespace.
So maybe something like this (not tested)
+ /* Test for the existence of a sysfs entry at /sys/fs/$FSTYP/$DEV/$ENTRY */
+_require_fs_sys_fs()
+{
+ local dev=$1
+ local target=$2
+ local dname=""
+ local tmp_mnt=`mktemp -d`
+
+ if [ ! -b "$dev" -o -z "$target" ];then
+ echo "Usage: _require_fs_sys_fs <device> <sysfs_path>"
+ exit 1
+ fi
+
+ dname=$(basename $(readlink -f $dev))
+ _mount -t $FSTYP `_common_dev_mount_options` $dev $tmp_mnt
+ if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
+ umount $tmp_mnt
+ rm -f $tmp_mnt
+ _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
+ fi
+ umount $SCRATCH_MNT
+ rm -f $tmp_mnt
+}
and it would work for either SCRATCH or TEST?
> +_set_sys_fs_param()
> +{
> + local dev=$1
> + shift
> + local target=$1
> + shift
> + local content="$*"
> +
> + if [ ! -b "$dev" -o -z "$target" -o -z "$content" ];then
> + echo "Usage: _set_sys_fs_param <mounted_device> <target> <content>"
> + exit 1
> + fi
> + local dname=$(basename $(readlink -f $dev))
> + echo "$content" > /sys/fs/${FSTYP}/${dname}/$target
> +}
This seems fine. Normally I'd say this could just be open coded
in the test, but the dname=$(basename $(readlink -f $dev)) is worth
encapsulating in the helper.
> +_enable_xfs_fail_at_unmount()
> +{
> + local dev=$1
> +
> + if [ ! -b "$dev" -o "$FSTYP" != "xfs" ];then
> + echo "Usage: _enable_xfs_fail_at_unmount <mounted_XFS_device>"
> + exit 1
> + fi
> +
> + _set_sys_fs_param $dev error/fail_at_unmount 1
> +}
> +
> +_disable_xfs_fail_at_unmount()
> +{
> + local dev=$1
> +
> + if [ ! -b "$dev" -o "$FSTYP" != "xfs" ];then
> + echo "Usage: _enable_xfs_fail_at_unmount <mounted_XFS_device>"
> + exit 1
> + fi
> +
> + _set_sys_fs_param $dev error/fail_at_unmount 0
> +}
I don't think this is needed; it's easy enough to just call:
_set_fs_sys_fs_param $dev error/fail_at_unmount 0
_set_fs_sys_fs_param $dev error/fail_at_unmount 1
directly from tests. There are a lot of sysfs tunables, writing wrappers
for each one isn't really that helpful.
Thanks,
-Eric
> init_rc
>
> ################################################################################
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] xfs/006: new case to test xfs fail_at_unmount error handling
2016-05-27 15:21 ` [PATCH 2/2] xfs/006: new case to test xfs fail_at_unmount error handling Zorro Lang
@ 2016-05-27 18:26 ` Eric Sandeen
2016-05-28 16:56 ` Zorro Lang
0 siblings, 1 reply; 10+ messages in thread
From: Eric Sandeen @ 2016-05-27 18:26 UTC (permalink / raw)
To: Zorro Lang, fstests; +Cc: cem
On 5/27/16 10:21 AM, Zorro Lang wrote:
> XFS bring in a new configuration under /sys/fs/xfs/error, named
> fail_at_unmount. It's used to stop unmount retrying forever when
> it hit IO error.
>
> This case try to unmount an faulty dm device, and to sure unmount
> won't retry forever if fail_at_unmount=1.
>
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
>
> Hi,
>
> Thanks Carlos told this easy way(use dm error) to test this feature, or
> I will try to use dm thinp to reproduce unmount hit an ENOSPC error. That's
> more difficult to do.
right, different errors from dm_thinp.
But I think my simple dm_thinp test and the infrastructure it uses
should make this easy? It was merged recently.
> I haven't gotten some good ideas to test those /sys/fs/xfs/dm-X/error/
> metadata/... configurations. If someone have good ideas, please let me
> know:)
>
> Thanks,
> Zorro
>
> tests/xfs/006 | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> tests/xfs/006.out | 2 ++
> tests/xfs/group | 1 +
> 3 files changed, 83 insertions(+)
> create mode 100755 tests/xfs/006
> create mode 100644 tests/xfs/006.out
>
> diff --git a/tests/xfs/006 b/tests/xfs/006
> new file mode 100755
> index 0000000..dafab50
> --- /dev/null
> +++ b/tests/xfs/006
> @@ -0,0 +1,80 @@
> +#! /bin/bash
> +# FS QA Test 006
> +#
> +# Test xfs' "fail at unmount" error handling configuration. Stop
> +# XFS retrying to writeback forever when unmount.
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2016 YOUR NAME HERE. All Rights Reserved.
> +#
> +# 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.
> +#
> +# This program is distributed in the hope that it would 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, write the Free Software Foundation,
> +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1 # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> + cd /
> + rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/dmerror
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +_supported_fs xfs
> +_supported_os Linux
> +_require_dm_target error
> +_require_scratch
> +_require_scratch_sys_fs error/fail_at_unmount
> +
> +# Format and mount
> +# The device is still a linear device at here
> +_scratch_mkfs > $seqres.full 2>&1
I guess my earlier suggestion was to not mkfs inside the
_require function for sysfs, so maybe move the test here:
_require_fs_sys_fs $SCRATCH_DEV error/fail_at_unmount
> +_dmerror_init
> +_dmerror_mount
> +
> +# Enable fail_at_unmount
> +_enable_xfs_fail_at_unmount $DMERROR_DEV
_set_fs_sys_fs_param $DMERROR_DEV error/fail_at_unmount 1
> +
> +# make some IO at here. If these data still not writeback from
> +# journal to data, it will hit writeback error after load error
> +# tabel in next step.
> +_pwrite_byte 0x61 0 100M $SCRATCH_MNT/file >/dev/null
> +_dmerror_load_error_table
> +
> +# Even above data already all writeback, umount will cause XFS
> +# try to writeback something to root inode. It still can trigger
> +# umount fail.
> +_dmerror_unmount
> +
> +echo "Silence is golden"
> +_dmerror_cleanup
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/006.out b/tests/xfs/006.out
> new file mode 100644
> index 0000000..675c1b7
> --- /dev/null
> +++ b/tests/xfs/006.out
> @@ -0,0 +1,2 @@
> +QA output created by 006
> +Silence is golden
> diff --git a/tests/xfs/group b/tests/xfs/group
> index f4c6816..39169ea 100644
> --- a/tests/xfs/group
> +++ b/tests/xfs/group
> @@ -4,6 +4,7 @@
> 004 db auto quick
> 005 auto quick
> 007 auto quota quick
> +006 auto quick unmount
> 008 rw ioctl auto quick
> 009 rw ioctl auto prealloc quick
> 010 auto quick repair
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs
2016-05-27 18:05 ` [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs Eric Sandeen
@ 2016-05-27 18:27 ` Eric Sandeen
2016-05-28 16:42 ` Zorro Lang
2016-05-28 16:36 ` Zorro Lang
1 sibling, 1 reply; 10+ messages in thread
From: Eric Sandeen @ 2016-05-27 18:27 UTC (permalink / raw)
To: Zorro Lang, fstests; +Cc: cem
On 5/27/16 1:05 PM, Eric Sandeen wrote:
> So maybe something like this (not tested)
>
> + /* Test for the existence of a sysfs entry at /sys/fs/$FSTYP/$DEV/$ENTRY */
> +_require_fs_sys_fs()
> +{
> + local dev=$1
> + local target=$2
> + local dname=""
> + local tmp_mnt=`mktemp -d`
> +
> + if [ ! -b "$dev" -o -z "$target" ];then
> + echo "Usage: _require_fs_sys_fs <device> <sysfs_path>"
> + exit 1
> + fi
> +
> + dname=$(basename $(readlink -f $dev))
> + _mount -t $FSTYP `_common_dev_mount_options` $dev $tmp_mnt
> + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
> + umount $tmp_mnt
> + rm -f $tmp_mnt
> + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
> + fi
> + umount $SCRATCH_MNT
> + rm -f $tmp_mnt
> +}
>
> and it would work for either SCRATCH or TEST?
I guess nothing guarantees that the device is already mkfs'd, at least
for scratch, so maybe it also needs to catch a mount failure,
with a message about "could not mount; mkfs first in your test?"
or something like that.
-Eric
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs
2016-05-27 18:05 ` [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs Eric Sandeen
2016-05-27 18:27 ` Eric Sandeen
@ 2016-05-28 16:36 ` Zorro Lang
2016-05-28 18:06 ` Eric Sandeen
1 sibling, 1 reply; 10+ messages in thread
From: Zorro Lang @ 2016-05-28 16:36 UTC (permalink / raw)
To: Eric Sandeen; +Cc: fstests
Hi Eric,
Thanks for you can review this, as I know you're very busy recently:)
On Fri, May 27, 2016 at 01:05:15PM -0500, Eric Sandeen wrote:
> On 5/27/16 10:21 AM, Zorro Lang wrote:
> > XFS add more configurations in /sys/fs/xfs recently. For use
> > them, this patch add some common functions for:
> > 1. "require" a file/dir in /sys/fs.
> > 2. write a file in /sys/fs.
>
> More specifically, in /sys/fs/${FSTYP}
Sure, I should specify this:)
>
> > For common use, these functions can be used by other filesystems.
> >
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > ---
> > common/rc | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 81 insertions(+)
> >
> > diff --git a/common/rc b/common/rc
> > index 51092a0..9935f08 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -3556,6 +3556,87 @@ run_fsx()
> > fi
> > }
> >
> > +_require_sys_fs()
> > +{
> > + local dev=$1
> > + local target=$2
> > + local dname=""
> > +
> > + if [ ! -b "$dev" -o -z "$target" ];then
> > + echo "Usage: _require_sys_fs <device> <target_need_exist>"
> > + exit 1
> > + fi
> > +
> > + dname=$(basename $(readlink -f $dev))
> > + _mkfs_dev $dev > /dev/null 2>&1
>
> we don't want to mkfs the test dev just for a _require test; that
> device is supposed to age, and this could possibly take a very long
> time, as well. And in general a mkfs should not be required to test
> the existence of a sysfs tunable, I think?
see below
>
> > + _mount -t $FSTYP `_common_dev_mount_options` $dev $SCRATCH_MNT
> > + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
> > + umount $SCRATCH_MNT
> > + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
> > + fi
> > + umount $SCRATCH_MNT
> > +}
> > +
> > +_require_scratch_sys_fs()
> > +{
> > + local target=$1
> > + local dname=""
> > +
> > + if [ ! -b "$SCRATCH_DEV" -o -z "$target" ];then
> > + echo "Usage: _require_scratch_sys_fs <target_need_exist>"
> > + exit 1
> > + fi
> > +
> > + dname=$(basename $(readlink -f $SCRATCH_DEV))
> > + _scratch_mkfs > /dev/null 2>&1
> > + _scratch_mount
> > + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
> > + _scratch_unmount
> > + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
> > + fi
> > + _scratch_unmount
> > +}
>
> This seems overly complex; do you need both of these functions? We are only
> testing one filesystem type at a time, and $TEST_DEV is always required.
> What is the reason for the two differing functions?
>
> Isn't _require_sys_fs $SCRATCH_DEV exactly the same as
> _require_scratch_sys_fs ?
>
Sorry for the "complex", let me explain:
1. At first, I write a function _require_sys_fs(), which used to check
all kinds of device's /sys/fs/fstype configurations. Include dmerror,
dmthinp and what ever other devices(not only TEST_DEV, SCRATCH_DEV).
2. Then I want to add a _require_scratch_sys_fs() function, which only
used for SCRATCH_DEV. Because I feel generally if SCRATCH_DEV support
a /sys/fs/fstype configurations, that means the working kernel support
it. And _require_sys_fs() can be used by other devices write their
_require_XXX_sys_fs() function(if needed... Looks like I thought too
much....).
3. When I wrote _require_scratch_sys_fs(), I want to use _require_sys_fs
$SCRATCH_DEV directly. But I saw _scratch_mkfs() use different options
with common mkfs_dev() function. So I use _scratch_mkfs and _scratch_mount
to write this _require_scratch_sys_fs().
4. About why I need mkfs before in this _require function, at first those
/sys/fs/fstype configurations only can be used after mount. The second
is I hope _require_sys_fs() can be used by other devices, which maybe
not mkfs... So....
> Also, because $SCRATCH_DEV is not required, $SCRATCH_MNT
> isn't either, so I don't think you can use $SCRATCH_MNT.
I did this according to _require_scratch_xfs_crc() function.
>
> Also :) We may one day want to require sysfs files which
> are not in the fs/ namespace, so it should probably
> be named according to the namespace.
>
> So maybe something like this (not tested)
>
> + /* Test for the existence of a sysfs entry at /sys/fs/$FSTYP/$DEV/$ENTRY */
> +_require_fs_sys_fs()
> +{
> + local dev=$1
> + local target=$2
> + local dname=""
> + local tmp_mnt=`mktemp -d`
> +
> + if [ ! -b "$dev" -o -z "$target" ];then
> + echo "Usage: _require_fs_sys_fs <device> <sysfs_path>"
> + exit 1
> + fi
> +
> + dname=$(basename $(readlink -f $dev))
> + _mount -t $FSTYP `_common_dev_mount_options` $dev $tmp_mnt
> + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
> + umount $tmp_mnt
> + rm -f $tmp_mnt
> + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
> + fi
> + umount $SCRATCH_MNT
I think you mean "umount $tmp_mnt" at here
> + rm -f $tmp_mnt
> +}
>
> and it would work for either SCRATCH or TEST?
hmm.. That's OK. I can't sure if mkfs before mount is better or not. If we
don't mkfs in this function, we ask the caller keep sure the $dev has been
mkfs. I think both are OK for me, I will follow your idea:)
>
> > +_set_sys_fs_param()
> > +{
> > + local dev=$1
> > + shift
> > + local target=$1
> > + shift
> > + local content="$*"
> > +
> > + if [ ! -b "$dev" -o -z "$target" -o -z "$content" ];then
> > + echo "Usage: _set_sys_fs_param <mounted_device> <target> <content>"
> > + exit 1
> > + fi
> > + local dname=$(basename $(readlink -f $dev))
> > + echo "$content" > /sys/fs/${FSTYP}/${dname}/$target
> > +}
>
> This seems fine. Normally I'd say this could just be open coded
> in the test, but the dname=$(basename $(readlink -f $dev)) is worth
> encapsulating in the helper.
Do you mean I should do?:
local dname=$(readlink -f $dev)
dname=`basename $dname`
>
> > +_enable_xfs_fail_at_unmount()
> > +{
> > + local dev=$1
> > +
> > + if [ ! -b "$dev" -o "$FSTYP" != "xfs" ];then
> > + echo "Usage: _enable_xfs_fail_at_unmount <mounted_XFS_device>"
> > + exit 1
> > + fi
> > +
> > + _set_sys_fs_param $dev error/fail_at_unmount 1
> > +}
> > +
> > +_disable_xfs_fail_at_unmount()
> > +{
> > + local dev=$1
> > +
> > + if [ ! -b "$dev" -o "$FSTYP" != "xfs" ];then
> > + echo "Usage: _enable_xfs_fail_at_unmount <mounted_XFS_device>"
> > + exit 1
> > + fi
> > +
> > + _set_sys_fs_param $dev error/fail_at_unmount 0
> > +}
>
> I don't think this is needed; it's easy enough to just call:
>
> _set_fs_sys_fs_param $dev error/fail_at_unmount 0
> _set_fs_sys_fs_param $dev error/fail_at_unmount 1
>
> directly from tests. There are a lot of sysfs tunables, writing wrappers
> for each one isn't really that helpful.
Yeah, except they can check if "$FSTYP" = "xfs", they don't do
more than use _set_fs_sys_fs_param directly.
Thanks,
Zorro
>
> Thanks,
> -Eric
>
> > init_rc
>
> >
> > ################################################################################
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs
2016-05-27 18:27 ` Eric Sandeen
@ 2016-05-28 16:42 ` Zorro Lang
2016-05-28 17:54 ` Eric Sandeen
0 siblings, 1 reply; 10+ messages in thread
From: Zorro Lang @ 2016-05-28 16:42 UTC (permalink / raw)
To: Eric Sandeen; +Cc: fstests
On Fri, May 27, 2016 at 01:27:56PM -0500, Eric Sandeen wrote:
> On 5/27/16 1:05 PM, Eric Sandeen wrote:
>
> > So maybe something like this (not tested)
> >
> > + /* Test for the existence of a sysfs entry at /sys/fs/$FSTYP/$DEV/$ENTRY */
> > +_require_fs_sys_fs()
> > +{
> > + local dev=$1
> > + local target=$2
> > + local dname=""
> > + local tmp_mnt=`mktemp -d`
> > +
> > + if [ ! -b "$dev" -o -z "$target" ];then
> > + echo "Usage: _require_fs_sys_fs <device> <sysfs_path>"
> > + exit 1
> > + fi
> > +
> > + dname=$(basename $(readlink -f $dev))
> > + _mount -t $FSTYP `_common_dev_mount_options` $dev $tmp_mnt
> > + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
> > + umount $tmp_mnt
> > + rm -f $tmp_mnt
> > + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
> > + fi
> > + umount $SCRATCH_MNT
> > + rm -f $tmp_mnt
> > +}
> >
> > and it would work for either SCRATCH or TEST?
>
> I guess nothing guarantees that the device is already mkfs'd, at least
> for scratch, so maybe it also needs to catch a mount failure,
> with a message about "could not mount; mkfs first in your test?"
> or something like that.
Yes, if we don't mkfs in _require_fs_sys_fs(), that means we need the
caller make sure they do that. I will check the mount error likes:
_mount -t $FSTYP `_common_dev_mount_options` $dev $tmp_mnt
if [ $? -ne 0 ];then
rm -f $tmp_mnt
_notrun "could not mount; mkfs first in your test?"
elif [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
....
fi
Thanks,
Zorro
>
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] xfs/006: new case to test xfs fail_at_unmount error handling
2016-05-27 18:26 ` Eric Sandeen
@ 2016-05-28 16:56 ` Zorro Lang
0 siblings, 0 replies; 10+ messages in thread
From: Zorro Lang @ 2016-05-28 16:56 UTC (permalink / raw)
To: Eric Sandeen; +Cc: fstests
On Fri, May 27, 2016 at 01:26:35PM -0500, Eric Sandeen wrote:
> On 5/27/16 10:21 AM, Zorro Lang wrote:
> > XFS bring in a new configuration under /sys/fs/xfs/error, named
> > fail_at_unmount. It's used to stop unmount retrying forever when
> > it hit IO error.
> >
> > This case try to unmount an faulty dm device, and to sure unmount
> > won't retry forever if fail_at_unmount=1.
> >
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > ---
> >
> > Hi,
> >
> > Thanks Carlos told this easy way(use dm error) to test this feature, or
> > I will try to use dm thinp to reproduce unmount hit an ENOSPC error. That's
> > more difficult to do.
>
> right, different errors from dm_thinp.
>
> But I think my simple dm_thinp test and the infrastructure it uses
> should make this easy? It was merged recently.
Yeah, those dmthin functions looks fine for me, I haven't use them(maybe
use them soon).
I said use dmthin is "more difficult", not means use your functions is difficult,
I mean use dmthin to test this "retry forever" issue is difficult. Because
if use dmthinp, I should make sure when I do unmount, there're some journal have
been recorded, but the data haven't be writeback. So when unmount try to flush
these data, it will hit ENOSPC. right?
I think it's too difficult to create this situation.
But if use dmerror, it's easy to make unmount hit EIO error.
Thanks,
Zorro
>
> > I haven't gotten some good ideas to test those /sys/fs/xfs/dm-X/error/
> > metadata/... configurations. If someone have good ideas, please let me
> > know:)
> >
> > Thanks,
> > Zorro
> >
> > tests/xfs/006 | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > tests/xfs/006.out | 2 ++
> > tests/xfs/group | 1 +
> > 3 files changed, 83 insertions(+)
> > create mode 100755 tests/xfs/006
> > create mode 100644 tests/xfs/006.out
> >
> > diff --git a/tests/xfs/006 b/tests/xfs/006
> > new file mode 100755
> > index 0000000..dafab50
> > --- /dev/null
> > +++ b/tests/xfs/006
> > @@ -0,0 +1,80 @@
> > +#! /bin/bash
> > +# FS QA Test 006
> > +#
> > +# Test xfs' "fail at unmount" error handling configuration. Stop
> > +# XFS retrying to writeback forever when unmount.
> > +#
> > +#-----------------------------------------------------------------------
> > +# Copyright (c) 2016 YOUR NAME HERE. All Rights Reserved.
> > +#
> > +# 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.
> > +#
> > +# This program is distributed in the hope that it would 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, write the Free Software Foundation,
> > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> > +#-----------------------------------------------------------------------
> > +#
> > +
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +status=1 # failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > + cd /
> > + rm -f $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +. ./common/dmerror
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +# real QA test starts here
> > +_supported_fs xfs
> > +_supported_os Linux
> > +_require_dm_target error
> > +_require_scratch
> > +_require_scratch_sys_fs error/fail_at_unmount
> > +
> > +# Format and mount
> > +# The device is still a linear device at here
> > +_scratch_mkfs > $seqres.full 2>&1
>
> I guess my earlier suggestion was to not mkfs inside the
> _require function for sysfs, so maybe move the test here:
>
> _require_fs_sys_fs $SCRATCH_DEV error/fail_at_unmount
hmm... I think generally we do _require_* test before do real test.
But do _require_* test after mkfs is still better than after mount:)
>
> > +_dmerror_init
> > +_dmerror_mount
> > +
> > +# Enable fail_at_unmount
> > +_enable_xfs_fail_at_unmount $DMERROR_DEV
>
> _set_fs_sys_fs_param $DMERROR_DEV error/fail_at_unmount 1
Sure
Thanks again for your review,
Zorro
>
> > +
> > +# make some IO at here. If these data still not writeback from
> > +# journal to data, it will hit writeback error after load error
> > +# tabel in next step.
> > +_pwrite_byte 0x61 0 100M $SCRATCH_MNT/file >/dev/null
> > +_dmerror_load_error_table
> > +
> > +# Even above data already all writeback, umount will cause XFS
> > +# try to writeback something to root inode. It still can trigger
> > +# umount fail.
> > +_dmerror_unmount
> > +
> > +echo "Silence is golden"
> > +_dmerror_cleanup
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/xfs/006.out b/tests/xfs/006.out
> > new file mode 100644
> > index 0000000..675c1b7
> > --- /dev/null
> > +++ b/tests/xfs/006.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 006
> > +Silence is golden
> > diff --git a/tests/xfs/group b/tests/xfs/group
> > index f4c6816..39169ea 100644
> > --- a/tests/xfs/group
> > +++ b/tests/xfs/group
> > @@ -4,6 +4,7 @@
> > 004 db auto quick
> > 005 auto quick
> > 007 auto quota quick
> > +006 auto quick unmount
> > 008 rw ioctl auto quick
> > 009 rw ioctl auto prealloc quick
> > 010 auto quick repair
> >
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs
2016-05-28 16:42 ` Zorro Lang
@ 2016-05-28 17:54 ` Eric Sandeen
0 siblings, 0 replies; 10+ messages in thread
From: Eric Sandeen @ 2016-05-28 17:54 UTC (permalink / raw)
To: Zorro Lang; +Cc: fstests
On 5/28/16 11:42 AM, Zorro Lang wrote:
> On Fri, May 27, 2016 at 01:27:56PM -0500, Eric Sandeen wrote:
>> On 5/27/16 1:05 PM, Eric Sandeen wrote:
>>
>>> So maybe something like this (not tested)
>>>
>>> + /* Test for the existence of a sysfs entry at /sys/fs/$FSTYP/$DEV/$ENTRY */
>>> +_require_fs_sys_fs()
>>> +{
>>> + local dev=$1
>>> + local target=$2
>>> + local dname=""
>>> + local tmp_mnt=`mktemp -d`
>>> +
>>> + if [ ! -b "$dev" -o -z "$target" ];then
>>> + echo "Usage: _require_fs_sys_fs <device> <sysfs_path>"
>>> + exit 1
>>> + fi
>>> +
>>> + dname=$(basename $(readlink -f $dev))
>>> + _mount -t $FSTYP `_common_dev_mount_options` $dev $tmp_mnt
>>> + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
>>> + umount $tmp_mnt
>>> + rm -f $tmp_mnt
>>> + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
>>> + fi
>>> + umount $SCRATCH_MNT
>>> + rm -f $tmp_mnt
>>> +}
>>>
>>> and it would work for either SCRATCH or TEST?
>>
>> I guess nothing guarantees that the device is already mkfs'd, at least
>> for scratch, so maybe it also needs to catch a mount failure,
>> with a message about "could not mount; mkfs first in your test?"
>> or something like that.
>
> Yes, if we don't mkfs in _require_fs_sys_fs(), that means we need the
> caller make sure they do that. I will check the mount error likes:
>
> _mount -t $FSTYP `_common_dev_mount_options` $dev $tmp_mnt
> if [ $? -ne 0 ];then
> rm -f $tmp_mnt
> _notrun "could not mount; mkfs first in your test?"
> elif [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
> ....
> fi
Yes, I think this is better. Any test using scratch should mkfs it
anyway, and this way the test will not have to perform mkfs twice -
once for the _require, and again for the test.
Thanks,
-Eric
> Thanks,
> Zorro
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs
2016-05-28 16:36 ` Zorro Lang
@ 2016-05-28 18:06 ` Eric Sandeen
0 siblings, 0 replies; 10+ messages in thread
From: Eric Sandeen @ 2016-05-28 18:06 UTC (permalink / raw)
To: Zorro Lang; +Cc: fstests
On 5/28/16 11:36 AM, Zorro Lang wrote:
> Hi Eric,
>
> Thanks for you can review this, as I know you're very busy recently:)
>
> On Fri, May 27, 2016 at 01:05:15PM -0500, Eric Sandeen wrote:
>> On 5/27/16 10:21 AM, Zorro Lang wrote:
>>> XFS add more configurations in /sys/fs/xfs recently. For use
>>> them, this patch add some common functions for:
>>> 1. "require" a file/dir in /sys/fs.
>>> 2. write a file in /sys/fs.
>>
>> More specifically, in /sys/fs/${FSTYP}
>
> Sure, I should specify this:)
>
>>
>>> For common use, these functions can be used by other filesystems.
>>>
>>> Signed-off-by: Zorro Lang <zlang@redhat.com>
>>> ---
>>> common/rc | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 81 insertions(+)
>>>
>>> diff --git a/common/rc b/common/rc
>>> index 51092a0..9935f08 100644
>>> --- a/common/rc
>>> +++ b/common/rc
>>> @@ -3556,6 +3556,87 @@ run_fsx()
>>> fi
>>> }
>>>
>>> +_require_sys_fs()
>>> +{
>>> + local dev=$1
>>> + local target=$2
>>> + local dname=""
>>> +
>>> + if [ ! -b "$dev" -o -z "$target" ];then
>>> + echo "Usage: _require_sys_fs <device> <target_need_exist>"
>>> + exit 1
>>> + fi
>>> +
>>> + dname=$(basename $(readlink -f $dev))
>>> + _mkfs_dev $dev > /dev/null 2>&1
>>
>> we don't want to mkfs the test dev just for a _require test; that
>> device is supposed to age, and this could possibly take a very long
>> time, as well. And in general a mkfs should not be required to test
>> the existence of a sysfs tunable, I think?
>
> see below
>
>>
>>> + _mount -t $FSTYP `_common_dev_mount_options` $dev $SCRATCH_MNT
>>> + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
>>> + umount $SCRATCH_MNT
>>> + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
>>> + fi
>>> + umount $SCRATCH_MNT
>>> +}
>>> +
>>> +_require_scratch_sys_fs()
>>> +{
>>> + local target=$1
>>> + local dname=""
>>> +
>>> + if [ ! -b "$SCRATCH_DEV" -o -z "$target" ];then
>>> + echo "Usage: _require_scratch_sys_fs <target_need_exist>"
>>> + exit 1
>>> + fi
>>> +
>>> + dname=$(basename $(readlink -f $SCRATCH_DEV))
>>> + _scratch_mkfs > /dev/null 2>&1
>>> + _scratch_mount
>>> + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
>>> + _scratch_unmount
>>> + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
>>> + fi
>>> + _scratch_unmount
>>> +}
>>
>> This seems overly complex; do you need both of these functions? We are only
>> testing one filesystem type at a time, and $TEST_DEV is always required.
>> What is the reason for the two differing functions?
>>
>> Isn't _require_sys_fs $SCRATCH_DEV exactly the same as
>> _require_scratch_sys_fs ?
>>
>
> Sorry for the "complex", let me explain:
> 1. At first, I write a function _require_sys_fs(), which used to check
> all kinds of device's /sys/fs/fstype configurations. Include dmerror,
> dmthinp and what ever other devices(not only TEST_DEV, SCRATCH_DEV).
Right, that one is generic.
> 2. Then I want to add a _require_scratch_sys_fs() function, which only
> used for SCRATCH_DEV. Because I feel generally if SCRATCH_DEV support
> a /sys/fs/fstype configurations, that means the working kernel support
> it. And _require_sys_fs() can be used by other devices write their
> _require_XXX_sys_fs() function(if needed... Looks like I thought too
> much....).
;)
> 3. When I wrote _require_scratch_sys_fs(), I want to use _require_sys_fs
> $SCRATCH_DEV directly. But I saw _scratch_mkfs() use different options
> with common mkfs_dev() function. So I use _scratch_mkfs and _scratch_mount
> to write this _require_scratch_sys_fs().
If available sysfs options depens on mkfs options, that could be an issue.
But if we make the _require function check for a previous mkfs from the main
test itself, then the test can control the mkfs options too.
> 4. About why I need mkfs before in this _require function, at first those
> /sys/fs/fstype configurations only can be used after mount. The second
> is I hope _require_sys_fs() can be used by other devices, which maybe
> not mkfs... So....
>
>> Also, because $SCRATCH_DEV is not required, $SCRATCH_MNT
>> isn't either, so I don't think you can use $SCRATCH_MNT.
>
> I did this according to _require_scratch_xfs_crc() function.
right, but that is specific to "scratch." My point was that
you were using $SCRATCH_MNT in the first generic function, which
is not specific to scratch, and can operate on any device.
>>
>> Also :) We may one day want to require sysfs files which
>> are not in the fs/ namespace, so it should probably
>> be named according to the namespace.
>>
>> So maybe something like this (not tested)
>>
>> + /* Test for the existence of a sysfs entry at /sys/fs/$FSTYP/$DEV/$ENTRY */
>> +_require_fs_sys_fs()
>> +{
>> + local dev=$1
>> + local target=$2
>> + local dname=""
>> + local tmp_mnt=`mktemp -d`
>> +
>> + if [ ! -b "$dev" -o -z "$target" ];then
>> + echo "Usage: _require_fs_sys_fs <device> <sysfs_path>"
>> + exit 1
>> + fi
>> +
>> + dname=$(basename $(readlink -f $dev))
>> + _mount -t $FSTYP `_common_dev_mount_options` $dev $tmp_mnt
>> + if [ ! -e /sys/fs/${FSTYP}/${dname}/$target ];then
>> + umount $tmp_mnt
>> + rm -f $tmp_mnt
>> + _notrun "/sys/fs/${FSTYP}/${dname}/$target: No such file or directory"
>> + fi
>> + umount $SCRATCH_MNT
>
> I think you mean "umount $tmp_mnt" at here
yep!
>
>> + rm -f $tmp_mnt
>> +}
>>
>> and it would work for either SCRATCH or TEST?
>
> hmm.. That's OK. I can't sure if mkfs before mount is better or not. If we
> don't mkfs in this function, we ask the caller keep sure the $dev has been
> mkfs. I think both are OK for me, I will follow your idea:)
It does seem a little strange to require the mkfs I guess, but it seems better
than performing mkfs twice in the test.
I suppose one other way to do it might e to make a tiny loopback filesystem
image in a tempfile, and test that. That way you could control the
mkfs time? But I guess it's possible that sysfs options *could* even depend
on the size of the fs, though I can't think of any today that do.
>>
>>> +_set_sys_fs_param()
>>> +{
>>> + local dev=$1
>>> + shift
>>> + local target=$1
>>> + shift
>>> + local content="$*"
>>> +
>>> + if [ ! -b "$dev" -o -z "$target" -o -z "$content" ];then
>>> + echo "Usage: _set_sys_fs_param <mounted_device> <target> <content>"
>>> + exit 1
>>> + fi
>>> + local dname=$(basename $(readlink -f $dev))
>>> + echo "$content" > /sys/fs/${FSTYP}/${dname}/$target
>>> +}
>>
>> This seems fine. Normally I'd say this could just be open coded
>> in the test, but the dname=$(basename $(readlink -f $dev)) is worth
>> encapsulating in the helper.
>
> Do you mean I should do?:
> local dname=$(readlink -f $dev)
> dname=`basename $dname`
No ... I mean, what you have is just fine, no change needed.
>>
>>> +_enable_xfs_fail_at_unmount()
>>> +{
>>> + local dev=$1
>>> +
>>> + if [ ! -b "$dev" -o "$FSTYP" != "xfs" ];then
>>> + echo "Usage: _enable_xfs_fail_at_unmount <mounted_XFS_device>"
>>> + exit 1
>>> + fi
>>> +
>>> + _set_sys_fs_param $dev error/fail_at_unmount 1
>>> +}
>>> +
>>> +_disable_xfs_fail_at_unmount()
>>> +{
>>> + local dev=$1
>>> +
>>> + if [ ! -b "$dev" -o "$FSTYP" != "xfs" ];then
>>> + echo "Usage: _enable_xfs_fail_at_unmount <mounted_XFS_device>"
>>> + exit 1
>>> + fi
>>> +
>>> + _set_sys_fs_param $dev error/fail_at_unmount 0
>>> +}
>>
>> I don't think this is needed; it's easy enough to just call:
>>
>> _set_fs_sys_fs_param $dev error/fail_at_unmount 0
>> _set_fs_sys_fs_param $dev error/fail_at_unmount 1
>>
>> directly from tests. There are a lot of sysfs tunables, writing wrappers
>> for each one isn't really that helpful.
>
> Yeah, except they can check if "$FSTYP" = "xfs", they don't do
> more than use _set_fs_sys_fs_param directly.
Well, it would be strange to call an xfs sysfs interface on a
non-xfs test, but ... I guess it could happen.
You could add something to _set_fs_sys_fs_param to also
_fail with a warning if /sys/fs/${FSTYP}/${dname}/$target does not exist.
-Eric
> Thanks,
> Zorro
>
>>
>> Thanks,
>> -Eric
>>
>>> init_rc
>>
>>>
>>> ################################################################################
>>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-05-28 18:06 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-27 15:21 [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs Zorro Lang
2016-05-27 15:21 ` [PATCH 2/2] xfs/006: new case to test xfs fail_at_unmount error handling Zorro Lang
2016-05-27 18:26 ` Eric Sandeen
2016-05-28 16:56 ` Zorro Lang
2016-05-27 18:05 ` [PATCH 1/2] common/rc: add functions to check or write objects under /sys/fs Eric Sandeen
2016-05-27 18:27 ` Eric Sandeen
2016-05-28 16:42 ` Zorro Lang
2016-05-28 17:54 ` Eric Sandeen
2016-05-28 16:36 ` Zorro Lang
2016-05-28 18:06 ` Eric Sandeen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox