test the per-inode DAX flag
diff mbox

Message ID 20170903093325.GA16272@lst.de
State New
Headers show

Commit Message

Christoph Hellwig Sept. 3, 2017, 9:33 a.m. UTC
This tests checks that the per-inode DAX flag is either reject
or sticks around, and that rapidly setting/clearing it will not
crash the kernel.

Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Comments

Eryu Guan Sept. 4, 2017, 7:24 a.m. UTC | #1
On Sun, Sep 03, 2017 at 11:33:25AM +0200, Christoph Hellwig wrote:
> This tests checks that the per-inode DAX flag is either reject
> or sticks around, and that rapidly setting/clearing it will not
> crash the kernel.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Does this test make sense when filesystem was mounted without dax
option? I saw these failures when testing on normal block device without
dax mount option.

    --- tests/xfs/115.out       2017-09-04 11:45:11.628000000 +0800
    +++ /root/workspace/xfstests/results//xfs_4k_crc/xfs/115.out.bad    2017-09-04 15:05:19.433000000 +0800
    @@ -1,4 +1,1004 @@
     QA output created by 115
     checking for DAX flag:
     --------------x- TEST_DIR/test.115
    +xfs_io: cannot set flags on /mnt/testarea/test/test.115: Input/output error
    +./tests/xfs/115: line 82: 15344 Bus error               $here/ltp/fsx -N 1000 $file > $tmp.log 2>&1
    +xfs_io: cannot set flags on /mnt/testarea/test/test.115: Input/output error
    +xfs_io: cannot set flags on /mnt/testarea/test/test.115: Input/output error
    ...

If not, I think we need a new _require_test_dax rule similar to
_require_scratch_dax.

> 
> diff --git a/tests/xfs/115 b/tests/xfs/115
> new file mode 100755
> index 0000000..4b6472f
> --- /dev/null
> +++ b/tests/xfs/115
> @@ -0,0 +1,92 @@
> +#! /bin/bash
> +# FS QA Test 115
> +#
> +# Test that the DAX xflag works, and rapidly setting/clearing it doesn't
> +# crash the kernel
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017 Christoph Hellwig.  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
> +
> +file=$TEST_DIR/test.$seq
> +
> +_lets_get_pidst()

Better to name local functions without the leading underscore.

> +{
> +	if [ -n "$pid" ]; then
> +		kill -TERM $pid 2>/dev/null
> +		pid=""
> +		wait 2>/dev/null
> +	fi
> +}
> +
> +_cleanup()
> +{
> +	cd /
> +	_lets_get_pidst
> +	rm -f $tmp.*
> +	rm -f $file*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs xfs
> +_supported_os Linux
> +_require_test
> +
> +# test that the +x flag sticks if we don't get an error
> +touch $file
> +xfs_io -c 'chattr +x' $file > $tmp.log 2>&1
> +if grep -q "Invalid argument" $tmp.log; then
> +    _notrun "file system doesn't support the DAX flag"
> +fi

I think we can add an "chattr" entry in  _require_xfs_io_command, and
actually run chattr command with the given option (-x in this case) to
check the xflag support status. (Then the _require_xfs_io_command usage
in xfs/260 should be fixed.)

> +
> +echo "checking for DAX flag:"
> +xfs_io -c 'lsattr' $file | _filter_test_dir

$XFS_IO_PROG

> +rm -f $file
> +
> +# run fsx racing with setting/clearing the DAX flag
> +$here/ltp/fsx -N 1000 $file > $tmp.log 2>&1 &
> +pid=$!
> +
> +for i in `seq 1 1000`; do
> +    xfs_io -c 'chattr +x' $file
> +    xfs_io -c 'chattr -x' $file
> +done
> +
> +_lets_get_pidst
> +
> +# success, all done
> +echo "*** done"
> +status=0
> +exit
> diff --git a/tests/xfs/115.out b/tests/xfs/115.out
> new file mode 100644
> index 0000000..652276a
> --- /dev/null
> +++ b/tests/xfs/115.out
> @@ -0,0 +1,4 @@
> +QA output created by 115
> +checking for DAX flag:
> +--------------x- TEST_DIR/test.115 
> +*** done
> diff --git a/tests/xfs/group b/tests/xfs/group
> index 5977330..5c02498 100644
> --- a/tests/xfs/group
> +++ b/tests/xfs/group
> @@ -111,6 +111,7 @@
>  112 fuzzers
>  113 fuzzers
>  114 auto quick clone rmap
> +115 auto rw dangerous

Sometimes test passed for me without triggering any crashes, and it
finished within 10 seconds, can be a 'quick' test too.

Thanks,
Eryu

>  116 quota auto quick
>  117 fuzzers
>  118 auto quick fsr dangerous
> --
> 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
--
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
Christoph Hellwig Sept. 4, 2017, 3:44 p.m. UTC | #2
On Mon, Sep 04, 2017 at 03:24:31PM +0800, Eryu Guan wrote:
> On Sun, Sep 03, 2017 at 11:33:25AM +0200, Christoph Hellwig wrote:
> > This tests checks that the per-inode DAX flag is either reject
> > or sticks around, and that rapidly setting/clearing it will not
> > crash the kernel.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Does this test make sense when filesystem was mounted without dax
> option? I saw these failures when testing on normal block device without
> dax mount option.

I think the first part that tries to set it makes sense everywhere,
but we should also _notrun for this case and not just for EINVAL.

That being said: right now I don't understand at all where the
EIO when setting the flag comes from, let me figure out where it is.

And thinking about it - why would we not allow setting the flag,
especially given that right now it doesn't have a meaning either
with or without DAX..
--
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
Theodore Ts'o Sept. 6, 2017, 2:50 p.m. UTC | #3
On Mon, Sep 04, 2017 at 05:44:53PM +0200, Christoph Hellwig wrote:
> > Does this test make sense when filesystem was mounted without dax
> > option? I saw these failures when testing on normal block device without
> > dax mount option.
> 
> And thinking about it - why would we not allow setting the flag,
> especially given that right now it doesn't have a meaning either
> with or without DAX..

At least for the ext4 patches that have been crossing my screen, the
claim from the commit description is that with the dax mount option,
all files are treated as if the DAX flag.  And the point of the DAX
flag is that if the file system is stored on DAX-capable hardware, and
you don't want DAX treatment for all files, that you would set the DAX
flag only for those files where having the file system actually using
the DAX codepaths would make sense --- as opposed to just using the
traditional block I/O paths against the /dev/pmem driver.

So I *think* the answer is that the tax makes sense when the file
system is mounted w/o the dax option, but we do need to check to see
whether the block device is DAX capable.

It also makes an interesting question how we should be testing DAX
capable file systems on /dev/pmem0.  Right now I force the use of
/dev/pmem0 *and* use the DAX mount options so that all of the other
tests will cause file I/O to use the DAX path.  But for the per-inode
DAX flag, we would want to mask out the DAX mount option so we can
test what happens when we toggle the DAX.

But I'm not a DAX expert, so I may have some of this wrong.

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

Patch
diff mbox

diff --git a/tests/xfs/114 b/tests/xfs/114
new file mode 100755
index 0000000..4b6472f
--- /dev/null
+++ b/tests/xfs/114
@@ -0,0 +1,92 @@ 
+#! /bin/bash
+# FS QA Test 114
+#
+# Test that the DAX xflag works, and rapidly setting/clearing it doesn't
+# crash the kernel
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2017 Christoph Hellwig.  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
+
+file=$TEST_DIR/test.$seq
+
+_lets_get_pidst()
+{
+	if [ -n "$pid" ]; then
+		kill -TERM $pid 2>/dev/null
+		pid=""
+		wait 2>/dev/null
+	fi
+}
+
+_cleanup()
+{
+	cd /
+	_lets_get_pidst
+	rm -f $tmp.*
+	rm -f $file*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs
+_supported_os Linux
+_require_test
+
+# test that the +x flag sticks if we don't get an error
+touch $file
+xfs_io -c 'chattr +x' $file > $tmp.log 2>&1
+if grep -q "Invalid argument" $tmp.log; then
+    _notrun "file system doesn't support the DAX flag"
+fi
+
+echo "checking for DAX flag:"
+xfs_io -c 'lsattr' $file | _filter_test_dir
+rm -f $file
+
+# run fsx racing with setting/clearing the DAX flag
+$here/ltp/fsx -N 1000 $file > $tmp.log 2>&1 &
+pid=$!
+
+for i in `seq 1 1000`; do
+    xfs_io -c 'chattr +x' $file
+    xfs_io -c 'chattr -x' $file
+done
+
+_lets_get_pidst
+
+# success, all done
+echo "*** done"
+status=0
+exit
diff --git a/tests/xfs/114.out b/tests/xfs/114.out
new file mode 100644
index 0000000..652276a
--- /dev/null
+++ b/tests/xfs/114.out
@@ -0,0 +1,4 @@ 
+QA output created by 114
+checking for DAX flag:
+--------------x- TEST_DIR/test.114 
+*** done
diff --git a/tests/xfs/group b/tests/xfs/group
index 5977330..5c02498 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -111,6 +111,7 @@ 
 111 ioctl
 112 fuzzers
 113 fuzzers
+114 auto rw dangerous
 116 quota auto quick
 117 fuzzers
 118 auto quick fsr dangerous