| Message ID | 20170903093325.GA16272@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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