FS/XFS testing framework
 help / color / mirror / Atom feed
* [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