* Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut [not found] <20161117032753.69315-1-jaegeuk@kernel.org> @ 2016-11-17 8:35 ` Eryu Guan 2016-11-17 12:56 ` Brian Foster 2016-11-17 18:31 ` Jaegeuk Kim 2016-11-17 19:20 ` [PATCH v2] " Jaegeuk Kim 1 sibling, 2 replies; 17+ messages in thread From: Eryu Guan @ 2016-11-17 8:35 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: fstests, linux-f2fs-devel, linux-xfs [cc'ed linux-xfs for inputs on a flush log issue] On Wed, Nov 16, 2016 at 07:27:53PM -0800, Jaegeuk Kim wrote: > This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. > > The rule to check is: > 1) fsync should guarantee all the inode metadata after power-cut, > 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. > > Suggested-by: Chao Yu <yuchao0@huawei.com> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > tests/generic/391 | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/generic/391.out | 11 +++++ > tests/generic/group | 1 + > 3 files changed, 137 insertions(+) > create mode 100644 tests/generic/391 > create mode 100644 tests/generic/391.out > > diff --git a/tests/generic/391 b/tests/generic/391 > new file mode 100644 > index 0000000..17c3fd3 > --- /dev/null > +++ b/tests/generic/391 > @@ -0,0 +1,125 @@ > +#! /bin/bash > +# FS QA Test 391 > +# > +# Test inode's metadata after fsync or fdatasync calls. > +# In the case of fsync, filesystem should recover all the inode metadata, while > +# recovering i_blocks and i_size at least for fdatasync. > +# > +#----------------------------------------------------------------------- > +# Copyright (c) 2016 Jaegeuk Kim. 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 "rm -f $tmp.*; exit \$status" 0 1 2 3 15 Better to follow the template and other tests on how to do cleanup, i.e. trap _cleanup on exit and do real cleanups in it. e.g. trap "_cleanup; exit \$status" 0 1 2 3 15 _cleanup() { cd / rm -f $tmp.* } Not a big deal, but it's better to maintain consistency :) > + > +# get standard environment, filters and checks > +. ./common/rc > +. ./common/filter > +. ./common/punch > + > +# real QA test starts here > +_supported_fs generic > +_supported_os Linux > + > +rm -f $seqres.full > +_require_scratch > +_require_scratch_shutdown > +_require_xfs_io_command "fpunch" > + > +_scratch_mkfs >/dev/null 2>&1 > +_scratch_mount > + > +testfile=$SCRATCH_MNT/testfile > + > +# check inode metadata after shutdown > +_check_inode_metadata() No need to name local functions with the leading "_", that's for global helper functions. > +{ > + src/godown $SCRATCH_MNT >> $seqres.full > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.before Shouldn't we call godown *after* xfs_io -c stat? I saw EIO on this xfs_io command and all sub-tests reported stat diff. And perhaps we need to flush the log on godown for XFS? i.e. src/godown -f $SCRATCH_MNT >> $seqres.full Otherwise XFS fails all the "1024" & fsync tests (after I fixed the godown & xfs_io order locally), fdatasync tests are fine. @@ -1,8 +1,16 @@ QA output created by 391 ==== i_size 1024 test with fsync ==== +6c6 +< stat.blocks = 8200 +--- +> stat.blocks = 16256 ==== i_size 4096 test with fsync ==== ==== i_time test with fsync ==== ==== fpunch 1024 test with fsync ==== +6c6 +< stat.blocks = 8208 +--- +> stat.blocks = 24576 ==== fpunch 4096 test with fsync ==== Not sure if this is the expected behavior on XFS. cc'ed xfs list for some inputs. > + _scratch_cycle_mount > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after > + diff $tmp.before $tmp.after >>$tmp.diff I think $tmp.diff should be overwritten on each sub-test. > + > + if [ "$2" = "fdatasync" ]; then > + cat $tmp.diff | grep stat.size > + cat $tmp.diff | grep stat.blocks > + else > + cat $tmp.diff > + fi > + cat $tmp.before >> $seqres.full > + cat $tmp.after >> $seqres.full > +} > + > +# append XX KB with f{data}sync, followed by power-cut > +_test_i_size() > +{ > + echo "==== i_size $2 test with $1 ====" I find it's much easier to read the $seqres.full log if we append these kind of logs to $seqres.full too, e.g. echo "==== i_size $2 test with $1 ====" | tee -a $seqres.full Thanks, Eryu > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > + -c "fsync" \ > + -c "pwrite 4M $2" \ > + -c "$1" \ > + $testfile >/dev/null > + _check_inode_metadata $testfile $1 > +} > + > +# update times with f{data}sync, followed by power-cut > +_test_i_time() > +{ > + echo "==== i_time test with $1 ====" > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > + -c "fsync" \ > + $testfile >/dev/null > + sleep 1 > + touch $testfile > + $XFS_IO_PROG -c "$1" \ > + $testfile >/dev/null > + _check_inode_metadata $testfile $1 > +} > + > +# punch XX KB with f{data}sync, followed by power-cut > +_test_punch() > +{ > + echo "==== fpunch $2 test with $1 ====" > + $XFS_IO_PROG -f -c "pwrite 0 4202496" \ > + -c "fsync" \ > + -c "fpunch 4194304 $2"\ > + -c "$1" \ > + $testfile >/dev/null > + > + _check_inode_metadata $testfile $1 > +} > + > +for i in fsync fdatasync > +do > + _test_i_size $i 1024 > + _test_i_size $i 4096 > + _test_i_time $i > + _test_punch $i 1024 > + _test_punch $i 4096 > +done > + > +rm -f $tmp.* > + > +# success, all done > +status=0 > +exit > diff --git a/tests/generic/391.out b/tests/generic/391.out > new file mode 100644 > index 0000000..7c66776 > --- /dev/null > +++ b/tests/generic/391.out > @@ -0,0 +1,11 @@ > +QA output created by 391 > +==== i_size 1024 test with fsync ==== > +==== i_size 4096 test with fsync ==== > +==== i_time test with fsync ==== > +==== fpunch 1024 test with fsync ==== > +==== fpunch 4096 test with fsync ==== > +==== i_size 1024 test with fdatasync ==== > +==== i_size 4096 test with fdatasync ==== > +==== i_time test with fdatasync ==== > +==== fpunch 1024 test with fdatasync ==== > +==== fpunch 4096 test with fdatasync ==== > diff --git a/tests/generic/group b/tests/generic/group > index 08007d7..9de3415 100644 > --- a/tests/generic/group > +++ b/tests/generic/group > @@ -392,3 +392,4 @@ > 387 auto clone > 388 auto log metadata > 389 auto quick acl > +391 auto quick metadata > -- > 2.8.3 > > -- > 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] 17+ messages in thread
* Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-17 8:35 ` [PATCH] generic/391: check inode metadata on f{data}sync after power-cut Eryu Guan @ 2016-11-17 12:56 ` Brian Foster 2016-11-17 14:00 ` Eryu Guan 2016-11-17 18:31 ` Jaegeuk Kim 1 sibling, 1 reply; 17+ messages in thread From: Brian Foster @ 2016-11-17 12:56 UTC (permalink / raw) To: Eryu Guan; +Cc: Jaegeuk Kim, fstests, linux-f2fs-devel, linux-xfs On Thu, Nov 17, 2016 at 04:35:27PM +0800, Eryu Guan wrote: > [cc'ed linux-xfs for inputs on a flush log issue] > > On Wed, Nov 16, 2016 at 07:27:53PM -0800, Jaegeuk Kim wrote: > > This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. > > > > The rule to check is: > > 1) fsync should guarantee all the inode metadata after power-cut, > > 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. > > > > Suggested-by: Chao Yu <yuchao0@huawei.com> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > tests/generic/391 | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > tests/generic/391.out | 11 +++++ > > tests/generic/group | 1 + > > 3 files changed, 137 insertions(+) > > create mode 100644 tests/generic/391 > > create mode 100644 tests/generic/391.out > > > > diff --git a/tests/generic/391 b/tests/generic/391 > > new file mode 100644 > > index 0000000..17c3fd3 > > --- /dev/null > > +++ b/tests/generic/391 > > @@ -0,0 +1,125 @@ > > +#! /bin/bash > > +# FS QA Test 391 > > +# > > +# Test inode's metadata after fsync or fdatasync calls. > > +# In the case of fsync, filesystem should recover all the inode metadata, while > > +# recovering i_blocks and i_size at least for fdatasync. > > +# > > +#----------------------------------------------------------------------- > > +# Copyright (c) 2016 Jaegeuk Kim. 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 "rm -f $tmp.*; exit \$status" 0 1 2 3 15 > > Better to follow the template and other tests on how to do cleanup, i.e. > trap _cleanup on exit and do real cleanups in it. e.g. > > trap "_cleanup; exit \$status" 0 1 2 3 15 > > _cleanup() > { > cd / > rm -f $tmp.* > } > > Not a big deal, but it's better to maintain consistency :) > > > + > > +# get standard environment, filters and checks > > +. ./common/rc > > +. ./common/filter > > +. ./common/punch > > + > > +# real QA test starts here > > +_supported_fs generic > > +_supported_os Linux > > + > > +rm -f $seqres.full > > +_require_scratch > > +_require_scratch_shutdown > > +_require_xfs_io_command "fpunch" > > + > > +_scratch_mkfs >/dev/null 2>&1 > > +_scratch_mount > > + > > +testfile=$SCRATCH_MNT/testfile > > + > > +# check inode metadata after shutdown > > +_check_inode_metadata() > > No need to name local functions with the leading "_", that's for global > helper functions. > > > +{ > > + src/godown $SCRATCH_MNT >> $seqres.full > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.before > > Shouldn't we call godown *after* xfs_io -c stat? I saw EIO on this > xfs_io command and all sub-tests reported stat diff. > Yeah.. I haven't run the test, but I would expect pretty much anything to return an error after an fs shutdown. > And perhaps we need to flush the log on godown for XFS? i.e. > > src/godown -f $SCRATCH_MNT >> $seqres.full > I don't think this is necessary. The semantics of fsync() dictate that the fs do what is necessary to make the file persistent on disk. This means it is the fs responsibility to ensure the changes are logged on disk. Indeed, xfs_file_fsync() calls _xfs_log_force_lsn() to flush the log up to the most recent LSN that covered the inode in question. > Otherwise XFS fails all the "1024" & fsync tests (after I fixed the > godown & xfs_io order locally), fdatasync tests are fine. > > @@ -1,8 +1,16 @@ > QA output created by 391 > ==== i_size 1024 test with fsync ==== > +6c6 > +< stat.blocks = 8200 > +--- > +> stat.blocks = 16256 > ==== i_size 4096 test with fsync ==== > ==== i_time test with fsync ==== > ==== fpunch 1024 test with fsync ==== > +6c6 > +< stat.blocks = 8208 > +--- > +> stat.blocks = 24576 > ==== fpunch 4096 test with fsync ==== > > Not sure if this is the expected behavior on XFS. cc'ed xfs list for > some inputs. > Am I reading this correctly that you're seeing more blocks than expected? If so, preallocation perhaps? Brian > > + _scratch_cycle_mount > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after > > + diff $tmp.before $tmp.after >>$tmp.diff > > I think $tmp.diff should be overwritten on each sub-test. > > > + > > + if [ "$2" = "fdatasync" ]; then > > + cat $tmp.diff | grep stat.size > > + cat $tmp.diff | grep stat.blocks > > + else > > + cat $tmp.diff > > + fi > > + cat $tmp.before >> $seqres.full > > + cat $tmp.after >> $seqres.full > > +} > > + > > +# append XX KB with f{data}sync, followed by power-cut > > +_test_i_size() > > +{ > > + echo "==== i_size $2 test with $1 ====" > > I find it's much easier to read the $seqres.full log if we append these > kind of logs to $seqres.full too, e.g. > > echo "==== i_size $2 test with $1 ====" | tee -a $seqres.full > > Thanks, > Eryu > > > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > > + -c "fsync" \ > > + -c "pwrite 4M $2" \ > > + -c "$1" \ > > + $testfile >/dev/null > > + _check_inode_metadata $testfile $1 > > +} > > + > > +# update times with f{data}sync, followed by power-cut > > +_test_i_time() > > +{ > > + echo "==== i_time test with $1 ====" > > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > > + -c "fsync" \ > > + $testfile >/dev/null > > + sleep 1 > > + touch $testfile > > + $XFS_IO_PROG -c "$1" \ > > + $testfile >/dev/null > > + _check_inode_metadata $testfile $1 > > +} > > + > > +# punch XX KB with f{data}sync, followed by power-cut > > +_test_punch() > > +{ > > + echo "==== fpunch $2 test with $1 ====" > > + $XFS_IO_PROG -f -c "pwrite 0 4202496" \ > > + -c "fsync" \ > > + -c "fpunch 4194304 $2"\ > > + -c "$1" \ > > + $testfile >/dev/null > > + > > + _check_inode_metadata $testfile $1 > > +} > > + > > +for i in fsync fdatasync > > +do > > + _test_i_size $i 1024 > > + _test_i_size $i 4096 > > + _test_i_time $i > > + _test_punch $i 1024 > > + _test_punch $i 4096 > > +done > > + > > +rm -f $tmp.* > > + > > +# success, all done > > +status=0 > > +exit > > diff --git a/tests/generic/391.out b/tests/generic/391.out > > new file mode 100644 > > index 0000000..7c66776 > > --- /dev/null > > +++ b/tests/generic/391.out > > @@ -0,0 +1,11 @@ > > +QA output created by 391 > > +==== i_size 1024 test with fsync ==== > > +==== i_size 4096 test with fsync ==== > > +==== i_time test with fsync ==== > > +==== fpunch 1024 test with fsync ==== > > +==== fpunch 4096 test with fsync ==== > > +==== i_size 1024 test with fdatasync ==== > > +==== i_size 4096 test with fdatasync ==== > > +==== i_time test with fdatasync ==== > > +==== fpunch 1024 test with fdatasync ==== > > +==== fpunch 4096 test with fdatasync ==== > > diff --git a/tests/generic/group b/tests/generic/group > > index 08007d7..9de3415 100644 > > --- a/tests/generic/group > > +++ b/tests/generic/group > > @@ -392,3 +392,4 @@ > > 387 auto clone > > 388 auto log metadata > > 389 auto quick acl > > +391 auto quick metadata > > -- > > 2.8.3 > > > > -- > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-17 12:56 ` Brian Foster @ 2016-11-17 14:00 ` Eryu Guan 2016-11-17 16:32 ` Brian Foster 0 siblings, 1 reply; 17+ messages in thread From: Eryu Guan @ 2016-11-17 14:00 UTC (permalink / raw) To: Brian Foster; +Cc: Jaegeuk Kim, fstests, linux-f2fs-devel, linux-xfs On Thu, Nov 17, 2016 at 07:56:58AM -0500, Brian Foster wrote: > On Thu, Nov 17, 2016 at 04:35:27PM +0800, Eryu Guan wrote: > > [cc'ed linux-xfs for inputs on a flush log issue] > > > > On Wed, Nov 16, 2016 at 07:27:53PM -0800, Jaegeuk Kim wrote: > > > This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. > > > > > > The rule to check is: > > > 1) fsync should guarantee all the inode metadata after power-cut, > > > 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. > > > > > > Suggested-by: Chao Yu <yuchao0@huawei.com> > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > --- > > > tests/generic/391 | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > tests/generic/391.out | 11 +++++ > > > tests/generic/group | 1 + > > > 3 files changed, 137 insertions(+) > > > create mode 100644 tests/generic/391 > > > create mode 100644 tests/generic/391.out > > > > > > diff --git a/tests/generic/391 b/tests/generic/391 > > > new file mode 100644 > > > index 0000000..17c3fd3 > > > --- /dev/null > > > +++ b/tests/generic/391 > > > @@ -0,0 +1,125 @@ > > > +#! /bin/bash > > > +# FS QA Test 391 > > > +# > > > +# Test inode's metadata after fsync or fdatasync calls. > > > +# In the case of fsync, filesystem should recover all the inode metadata, while > > > +# recovering i_blocks and i_size at least for fdatasync. > > > +# > > > +#----------------------------------------------------------------------- > > > +# Copyright (c) 2016 Jaegeuk Kim. 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 "rm -f $tmp.*; exit \$status" 0 1 2 3 15 > > > > Better to follow the template and other tests on how to do cleanup, i.e. > > trap _cleanup on exit and do real cleanups in it. e.g. > > > > trap "_cleanup; exit \$status" 0 1 2 3 15 > > > > _cleanup() > > { > > cd / > > rm -f $tmp.* > > } > > > > Not a big deal, but it's better to maintain consistency :) > > > > > + > > > +# get standard environment, filters and checks > > > +. ./common/rc > > > +. ./common/filter > > > +. ./common/punch > > > + > > > +# real QA test starts here > > > +_supported_fs generic > > > +_supported_os Linux > > > + > > > +rm -f $seqres.full > > > +_require_scratch > > > +_require_scratch_shutdown > > > +_require_xfs_io_command "fpunch" > > > + > > > +_scratch_mkfs >/dev/null 2>&1 > > > +_scratch_mount > > > + > > > +testfile=$SCRATCH_MNT/testfile > > > + > > > +# check inode metadata after shutdown > > > +_check_inode_metadata() > > > > No need to name local functions with the leading "_", that's for global > > helper functions. > > > > > +{ > > > + src/godown $SCRATCH_MNT >> $seqres.full > > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.before > > > > Shouldn't we call godown *after* xfs_io -c stat? I saw EIO on this > > xfs_io command and all sub-tests reported stat diff. > > > > Yeah.. I haven't run the test, but I would expect pretty much anything > to return an error after an fs shutdown. > > > And perhaps we need to flush the log on godown for XFS? i.e. > > > > src/godown -f $SCRATCH_MNT >> $seqres.full > > > > I don't think this is necessary. The semantics of fsync() dictate that > the fs do what is necessary to make the file persistent on disk. This > means it is the fs responsibility to ensure the changes are logged on > disk. Indeed, xfs_file_fsync() calls _xfs_log_force_lsn() to flush the > log up to the most recent LSN that covered the inode in question. > > > Otherwise XFS fails all the "1024" & fsync tests (after I fixed the > > godown & xfs_io order locally), fdatasync tests are fine. > > > > @@ -1,8 +1,16 @@ > > QA output created by 391 > > ==== i_size 1024 test with fsync ==== > > +6c6 > > +< stat.blocks = 8200 > > +--- > > +> stat.blocks = 16256 > > ==== i_size 4096 test with fsync ==== > > ==== i_time test with fsync ==== > > ==== fpunch 1024 test with fsync ==== > > +6c6 > > +< stat.blocks = 8208 > > +--- > > +> stat.blocks = 24576 > > ==== fpunch 4096 test with fsync ==== > > > > Not sure if this is the expected behavior on XFS. cc'ed xfs list for > > some inputs. > > > > Am I reading this correctly that you're seeing more blocks than > expected? If so, preallocation perhaps? Yes, you're correct, I see more blocks after godown than before godown. I tried adding "-o allocsize=4k" to MOUNT_OPTIONS, it works but not always. e.g. on a host with sunit/swidth reported from underlying block device, test still fails. # xfs_info /mnt/xfs meta-data=/dev/mapper/systemvg-testlv2 isize=512 agcount=16, agsize=245696 blks = sectsz=512 attr=2, projid32bit=1 = crc=1 finobt=1 spinodes=0 rmapbt=0 = reflink=0 data = bsize=4096 blocks=3931136, imaxpct=25 = sunit=64 swidth=192 blks naming =version 2 bsize=4096 ascii-ci=0 ftype=1 log =internal bsize=4096 blocks=2560, version=2 = sectsz=512 sunit=64 blks, lazy-count=1 realtime =none extsz=4096 blocks=0, rtextents=0 Part of the test diff: ==== i_size 1024 test with fsync ==== +6c6 +< stat.blocks = 8200 +--- +> stat.blocks = 8704 On the other hand, adding "-f" to godown always works for me. Thanks, Eryu > > Brian > > > > + _scratch_cycle_mount > > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after > > > + diff $tmp.before $tmp.after >>$tmp.diff > > > > I think $tmp.diff should be overwritten on each sub-test. > > > > > + > > > + if [ "$2" = "fdatasync" ]; then > > > + cat $tmp.diff | grep stat.size > > > + cat $tmp.diff | grep stat.blocks > > > + else > > > + cat $tmp.diff > > > + fi > > > + cat $tmp.before >> $seqres.full > > > + cat $tmp.after >> $seqres.full > > > +} > > > + > > > +# append XX KB with f{data}sync, followed by power-cut > > > +_test_i_size() > > > +{ > > > + echo "==== i_size $2 test with $1 ====" > > > > I find it's much easier to read the $seqres.full log if we append these > > kind of logs to $seqres.full too, e.g. > > > > echo "==== i_size $2 test with $1 ====" | tee -a $seqres.full > > > > Thanks, > > Eryu > > > > > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > > > + -c "fsync" \ > > > + -c "pwrite 4M $2" \ > > > + -c "$1" \ > > > + $testfile >/dev/null > > > + _check_inode_metadata $testfile $1 > > > +} > > > + > > > +# update times with f{data}sync, followed by power-cut > > > +_test_i_time() > > > +{ > > > + echo "==== i_time test with $1 ====" > > > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > > > + -c "fsync" \ > > > + $testfile >/dev/null > > > + sleep 1 > > > + touch $testfile > > > + $XFS_IO_PROG -c "$1" \ > > > + $testfile >/dev/null > > > + _check_inode_metadata $testfile $1 > > > +} > > > + > > > +# punch XX KB with f{data}sync, followed by power-cut > > > +_test_punch() > > > +{ > > > + echo "==== fpunch $2 test with $1 ====" > > > + $XFS_IO_PROG -f -c "pwrite 0 4202496" \ > > > + -c "fsync" \ > > > + -c "fpunch 4194304 $2"\ > > > + -c "$1" \ > > > + $testfile >/dev/null > > > + > > > + _check_inode_metadata $testfile $1 > > > +} > > > + > > > +for i in fsync fdatasync > > > +do > > > + _test_i_size $i 1024 > > > + _test_i_size $i 4096 > > > + _test_i_time $i > > > + _test_punch $i 1024 > > > + _test_punch $i 4096 > > > +done > > > + > > > +rm -f $tmp.* > > > + > > > +# success, all done > > > +status=0 > > > +exit > > > diff --git a/tests/generic/391.out b/tests/generic/391.out > > > new file mode 100644 > > > index 0000000..7c66776 > > > --- /dev/null > > > +++ b/tests/generic/391.out > > > @@ -0,0 +1,11 @@ > > > +QA output created by 391 > > > +==== i_size 1024 test with fsync ==== > > > +==== i_size 4096 test with fsync ==== > > > +==== i_time test with fsync ==== > > > +==== fpunch 1024 test with fsync ==== > > > +==== fpunch 4096 test with fsync ==== > > > +==== i_size 1024 test with fdatasync ==== > > > +==== i_size 4096 test with fdatasync ==== > > > +==== i_time test with fdatasync ==== > > > +==== fpunch 1024 test with fdatasync ==== > > > +==== fpunch 4096 test with fdatasync ==== > > > diff --git a/tests/generic/group b/tests/generic/group > > > index 08007d7..9de3415 100644 > > > --- a/tests/generic/group > > > +++ b/tests/generic/group > > > @@ -392,3 +392,4 @@ > > > 387 auto clone > > > 388 auto log metadata > > > 389 auto quick acl > > > +391 auto quick metadata > > > -- > > > 2.8.3 > > > > > > -- > > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-17 14:00 ` Eryu Guan @ 2016-11-17 16:32 ` Brian Foster 2016-11-17 16:51 ` Eryu Guan 2016-11-17 19:17 ` Jaegeuk Kim 0 siblings, 2 replies; 17+ messages in thread From: Brian Foster @ 2016-11-17 16:32 UTC (permalink / raw) To: Eryu Guan; +Cc: Jaegeuk Kim, fstests, linux-f2fs-devel, linux-xfs On Thu, Nov 17, 2016 at 10:00:19PM +0800, Eryu Guan wrote: > On Thu, Nov 17, 2016 at 07:56:58AM -0500, Brian Foster wrote: > > On Thu, Nov 17, 2016 at 04:35:27PM +0800, Eryu Guan wrote: > > > [cc'ed linux-xfs for inputs on a flush log issue] > > > > > > On Wed, Nov 16, 2016 at 07:27:53PM -0800, Jaegeuk Kim wrote: > > > > This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. > > > > > > > > The rule to check is: > > > > 1) fsync should guarantee all the inode metadata after power-cut, > > > > 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. > > > > > > > > Suggested-by: Chao Yu <yuchao0@huawei.com> > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > --- > > > > tests/generic/391 | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > tests/generic/391.out | 11 +++++ > > > > tests/generic/group | 1 + > > > > 3 files changed, 137 insertions(+) > > > > create mode 100644 tests/generic/391 > > > > create mode 100644 tests/generic/391.out > > > > > > > > diff --git a/tests/generic/391 b/tests/generic/391 > > > > new file mode 100644 > > > > index 0000000..17c3fd3 > > > > --- /dev/null > > > > +++ b/tests/generic/391 > > > > @@ -0,0 +1,125 @@ > > > > +#! /bin/bash > > > > +# FS QA Test 391 > > > > +# > > > > +# Test inode's metadata after fsync or fdatasync calls. > > > > +# In the case of fsync, filesystem should recover all the inode metadata, while > > > > +# recovering i_blocks and i_size at least for fdatasync. > > > > +# > > > > +#----------------------------------------------------------------------- > > > > +# Copyright (c) 2016 Jaegeuk Kim. 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 "rm -f $tmp.*; exit \$status" 0 1 2 3 15 > > > > > > Better to follow the template and other tests on how to do cleanup, i.e. > > > trap _cleanup on exit and do real cleanups in it. e.g. > > > > > > trap "_cleanup; exit \$status" 0 1 2 3 15 > > > > > > _cleanup() > > > { > > > cd / > > > rm -f $tmp.* > > > } > > > > > > Not a big deal, but it's better to maintain consistency :) > > > > > > > + > > > > +# get standard environment, filters and checks > > > > +. ./common/rc > > > > +. ./common/filter > > > > +. ./common/punch > > > > + > > > > +# real QA test starts here > > > > +_supported_fs generic > > > > +_supported_os Linux > > > > + > > > > +rm -f $seqres.full > > > > +_require_scratch > > > > +_require_scratch_shutdown > > > > +_require_xfs_io_command "fpunch" > > > > + > > > > +_scratch_mkfs >/dev/null 2>&1 > > > > +_scratch_mount > > > > + > > > > +testfile=$SCRATCH_MNT/testfile > > > > + > > > > +# check inode metadata after shutdown > > > > +_check_inode_metadata() > > > > > > No need to name local functions with the leading "_", that's for global > > > helper functions. > > > > > > > +{ > > > > + src/godown $SCRATCH_MNT >> $seqres.full > > > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.before > > > > > > Shouldn't we call godown *after* xfs_io -c stat? I saw EIO on this > > > xfs_io command and all sub-tests reported stat diff. > > > > > > > Yeah.. I haven't run the test, but I would expect pretty much anything > > to return an error after an fs shutdown. > > > > > And perhaps we need to flush the log on godown for XFS? i.e. > > > > > > src/godown -f $SCRATCH_MNT >> $seqres.full > > > > > > > I don't think this is necessary. The semantics of fsync() dictate that > > the fs do what is necessary to make the file persistent on disk. This > > means it is the fs responsibility to ensure the changes are logged on > > disk. Indeed, xfs_file_fsync() calls _xfs_log_force_lsn() to flush the > > log up to the most recent LSN that covered the inode in question. > > > > > Otherwise XFS fails all the "1024" & fsync tests (after I fixed the > > > godown & xfs_io order locally), fdatasync tests are fine. > > > > > > @@ -1,8 +1,16 @@ > > > QA output created by 391 > > > ==== i_size 1024 test with fsync ==== > > > +6c6 > > > +< stat.blocks = 8200 > > > +--- > > > +> stat.blocks = 16256 > > > ==== i_size 4096 test with fsync ==== > > > ==== i_time test with fsync ==== > > > ==== fpunch 1024 test with fsync ==== > > > +6c6 > > > +< stat.blocks = 8208 > > > +--- > > > +> stat.blocks = 24576 > > > ==== fpunch 4096 test with fsync ==== > > > > > > Not sure if this is the expected behavior on XFS. cc'ed xfs list for > > > some inputs. > > > > > > > Am I reading this correctly that you're seeing more blocks than > > expected? If so, preallocation perhaps? > > Yes, you're correct, I see more blocks after godown than before godown. > > I tried adding "-o allocsize=4k" to MOUNT_OPTIONS, it works but not > always. e.g. on a host with sunit/swidth reported from underlying block > device, test still fails. > I'm not quite sure where the preallocation is coming from in that case. It looks like it should honor allocsize, so that might be worth looking into. > # xfs_info /mnt/xfs > meta-data=/dev/mapper/systemvg-testlv2 isize=512 agcount=16, agsize=245696 blks > = sectsz=512 attr=2, projid32bit=1 > = crc=1 finobt=1 spinodes=0 rmapbt=0 > = reflink=0 > data = bsize=4096 blocks=3931136, imaxpct=25 > = sunit=64 swidth=192 blks > naming =version 2 bsize=4096 ascii-ci=0 ftype=1 > log =internal bsize=4096 blocks=2560, version=2 > = sectsz=512 sunit=64 blks, lazy-count=1 > realtime =none extsz=4096 blocks=0, rtextents=0 > > Part of the test diff: > ==== i_size 1024 test with fsync ==== > +6c6 > +< stat.blocks = 8200 > +--- > +> stat.blocks = 8704 > > On the other hand, adding "-f" to godown always works for me. > I'm guessing the difference here is that fsync flushes the inode with preallocation, but preallocation is typically cleaned up on file close (when xfs_io exits). So a subsequent log flush at shutdown may flush the transaction that clears out post-eof blocks. Note that it may also hit the disk without the log forcing shutdown, it's just not guaranteed in that case. The right thing to do is probably deal with preallocation explicitly in the test. E.g., a truncate of the file to the current size after a potentially preallocated write, but before the fsync, should always result in an equivalent blocks count post-recovery. (Another angle here might be to consider whether the block count is the right metric for this test, as opposed to a hexdump that validates the written data actually made it to disk.). Brian > Thanks, > Eryu > > > > > Brian > > > > > > + _scratch_cycle_mount > > > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after > > > > + diff $tmp.before $tmp.after >>$tmp.diff > > > > > > I think $tmp.diff should be overwritten on each sub-test. > > > > > > > + > > > > + if [ "$2" = "fdatasync" ]; then > > > > + cat $tmp.diff | grep stat.size > > > > + cat $tmp.diff | grep stat.blocks > > > > + else > > > > + cat $tmp.diff > > > > + fi > > > > + cat $tmp.before >> $seqres.full > > > > + cat $tmp.after >> $seqres.full > > > > +} > > > > + > > > > +# append XX KB with f{data}sync, followed by power-cut > > > > +_test_i_size() > > > > +{ > > > > + echo "==== i_size $2 test with $1 ====" > > > > > > I find it's much easier to read the $seqres.full log if we append these > > > kind of logs to $seqres.full too, e.g. > > > > > > echo "==== i_size $2 test with $1 ====" | tee -a $seqres.full > > > > > > Thanks, > > > Eryu > > > > > > > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > > > > + -c "fsync" \ > > > > + -c "pwrite 4M $2" \ > > > > + -c "$1" \ > > > > + $testfile >/dev/null > > > > + _check_inode_metadata $testfile $1 > > > > +} > > > > + > > > > +# update times with f{data}sync, followed by power-cut > > > > +_test_i_time() > > > > +{ > > > > + echo "==== i_time test with $1 ====" > > > > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > > > > + -c "fsync" \ > > > > + $testfile >/dev/null > > > > + sleep 1 > > > > + touch $testfile > > > > + $XFS_IO_PROG -c "$1" \ > > > > + $testfile >/dev/null > > > > + _check_inode_metadata $testfile $1 > > > > +} > > > > + > > > > +# punch XX KB with f{data}sync, followed by power-cut > > > > +_test_punch() > > > > +{ > > > > + echo "==== fpunch $2 test with $1 ====" > > > > + $XFS_IO_PROG -f -c "pwrite 0 4202496" \ > > > > + -c "fsync" \ > > > > + -c "fpunch 4194304 $2"\ > > > > + -c "$1" \ > > > > + $testfile >/dev/null > > > > + > > > > + _check_inode_metadata $testfile $1 > > > > +} > > > > + > > > > +for i in fsync fdatasync > > > > +do > > > > + _test_i_size $i 1024 > > > > + _test_i_size $i 4096 > > > > + _test_i_time $i > > > > + _test_punch $i 1024 > > > > + _test_punch $i 4096 > > > > +done > > > > + > > > > +rm -f $tmp.* > > > > + > > > > +# success, all done > > > > +status=0 > > > > +exit > > > > diff --git a/tests/generic/391.out b/tests/generic/391.out > > > > new file mode 100644 > > > > index 0000000..7c66776 > > > > --- /dev/null > > > > +++ b/tests/generic/391.out > > > > @@ -0,0 +1,11 @@ > > > > +QA output created by 391 > > > > +==== i_size 1024 test with fsync ==== > > > > +==== i_size 4096 test with fsync ==== > > > > +==== i_time test with fsync ==== > > > > +==== fpunch 1024 test with fsync ==== > > > > +==== fpunch 4096 test with fsync ==== > > > > +==== i_size 1024 test with fdatasync ==== > > > > +==== i_size 4096 test with fdatasync ==== > > > > +==== i_time test with fdatasync ==== > > > > +==== fpunch 1024 test with fdatasync ==== > > > > +==== fpunch 4096 test with fdatasync ==== > > > > diff --git a/tests/generic/group b/tests/generic/group > > > > index 08007d7..9de3415 100644 > > > > --- a/tests/generic/group > > > > +++ b/tests/generic/group > > > > @@ -392,3 +392,4 @@ > > > > 387 auto clone > > > > 388 auto log metadata > > > > 389 auto quick acl > > > > +391 auto quick metadata > > > > -- > > > > 2.8.3 > > > > > > > > -- > > > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-17 16:32 ` Brian Foster @ 2016-11-17 16:51 ` Eryu Guan 2016-11-17 19:17 ` Jaegeuk Kim 1 sibling, 0 replies; 17+ messages in thread From: Eryu Guan @ 2016-11-17 16:51 UTC (permalink / raw) To: Brian Foster; +Cc: Jaegeuk Kim, fstests, linux-f2fs-devel, linux-xfs On Thu, Nov 17, 2016 at 11:32:03AM -0500, Brian Foster wrote: [snip some unrelated context] > > > > > +{ > > > > > + src/godown $SCRATCH_MNT >> $seqres.full > > > > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.before > > > > > > > > Shouldn't we call godown *after* xfs_io -c stat? I saw EIO on this > > > > xfs_io command and all sub-tests reported stat diff. > > > > > > > > > > Yeah.. I haven't run the test, but I would expect pretty much anything > > > to return an error after an fs shutdown. > > > > > > > And perhaps we need to flush the log on godown for XFS? i.e. > > > > > > > > src/godown -f $SCRATCH_MNT >> $seqres.full > > > > > > > > > > I don't think this is necessary. The semantics of fsync() dictate that > > > the fs do what is necessary to make the file persistent on disk. This > > > means it is the fs responsibility to ensure the changes are logged on > > > disk. Indeed, xfs_file_fsync() calls _xfs_log_force_lsn() to flush the > > > log up to the most recent LSN that covered the inode in question. > > > > > > > Otherwise XFS fails all the "1024" & fsync tests (after I fixed the > > > > godown & xfs_io order locally), fdatasync tests are fine. > > > > > > > > @@ -1,8 +1,16 @@ > > > > QA output created by 391 > > > > ==== i_size 1024 test with fsync ==== > > > > +6c6 > > > > +< stat.blocks = 8200 > > > > +--- > > > > +> stat.blocks = 16256 > > > > ==== i_size 4096 test with fsync ==== > > > > ==== i_time test with fsync ==== > > > > ==== fpunch 1024 test with fsync ==== > > > > +6c6 > > > > +< stat.blocks = 8208 > > > > +--- > > > > +> stat.blocks = 24576 > > > > ==== fpunch 4096 test with fsync ==== > > > > > > > > Not sure if this is the expected behavior on XFS. cc'ed xfs list for > > > > some inputs. > > > > > > > > > > Am I reading this correctly that you're seeing more blocks than > > > expected? If so, preallocation perhaps? > > > > Yes, you're correct, I see more blocks after godown than before godown. > > > > I tried adding "-o allocsize=4k" to MOUNT_OPTIONS, it works but not > > always. e.g. on a host with sunit/swidth reported from underlying block > > device, test still fails. > > > > I'm not quite sure where the preallocation is coming from in that case. > It looks like it should honor allocsize, so that might be worth looking > into. > > > # xfs_info /mnt/xfs > > meta-data=/dev/mapper/systemvg-testlv2 isize=512 agcount=16, agsize=245696 blks > > = sectsz=512 attr=2, projid32bit=1 > > = crc=1 finobt=1 spinodes=0 rmapbt=0 > > = reflink=0 > > data = bsize=4096 blocks=3931136, imaxpct=25 > > = sunit=64 swidth=192 blks > > naming =version 2 bsize=4096 ascii-ci=0 ftype=1 > > log =internal bsize=4096 blocks=2560, version=2 > > = sectsz=512 sunit=64 blks, lazy-count=1 > > realtime =none extsz=4096 blocks=0, rtextents=0 > > > > Part of the test diff: > > ==== i_size 1024 test with fsync ==== > > +6c6 > > +< stat.blocks = 8200 > > +--- > > +> stat.blocks = 8704 > > > > On the other hand, adding "-f" to godown always works for me. > > > > I'm guessing the difference here is that fsync flushes the inode with > preallocation, but preallocation is typically cleaned up on file close > (when xfs_io exits). So a subsequent log flush at shutdown may flush > the transaction that clears out post-eof blocks. Note that it may also > hit the disk without the log forcing shutdown, it's just not guaranteed > in that case. > > The right thing to do is probably deal with preallocation explicitly in > the test. E.g., a truncate of the file to the current size after a > potentially preallocated write, but before the fsync, should always > result in an equivalent blocks count post-recovery. You're right, I added truncate operation to isize test and punch test, and this case passed without problem on XFS. Thanks! Eryu ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-17 16:32 ` Brian Foster 2016-11-17 16:51 ` Eryu Guan @ 2016-11-17 19:17 ` Jaegeuk Kim 1 sibling, 0 replies; 17+ messages in thread From: Jaegeuk Kim @ 2016-11-17 19:17 UTC (permalink / raw) To: Brian Foster; +Cc: Eryu Guan, fstests, linux-f2fs-devel, linux-xfs Hello, On Thu, Nov 17, 2016 at 11:32:03AM -0500, Brian Foster wrote: > On Thu, Nov 17, 2016 at 10:00:19PM +0800, Eryu Guan wrote: > > On Thu, Nov 17, 2016 at 07:56:58AM -0500, Brian Foster wrote: > > > On Thu, Nov 17, 2016 at 04:35:27PM +0800, Eryu Guan wrote: > > > > [cc'ed linux-xfs for inputs on a flush log issue] > > > > > > > > On Wed, Nov 16, 2016 at 07:27:53PM -0800, Jaegeuk Kim wrote: > > > > > This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. > > > > > > > > > > The rule to check is: > > > > > 1) fsync should guarantee all the inode metadata after power-cut, > > > > > 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. > > > > > > > > > > Suggested-by: Chao Yu <yuchao0@huawei.com> > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > --- > > > > > tests/generic/391 | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > tests/generic/391.out | 11 +++++ > > > > > tests/generic/group | 1 + > > > > > 3 files changed, 137 insertions(+) > > > > > create mode 100644 tests/generic/391 > > > > > create mode 100644 tests/generic/391.out > > > > > ... > > > > > + > > > > > +seq=`basename $0` > > > > > +seqres=$RESULT_DIR/$seq > > > > > +echo "QA output created by $seq" > > > > > + > > > > > +here=`pwd` > > > > > +tmp=/tmp/$$ > > > > > +status=1 # failure is the default! > > > > > +trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15 > > > > > > > > Better to follow the template and other tests on how to do cleanup, i.e. > > > > trap _cleanup on exit and do real cleanups in it. e.g. > > > > > > > > trap "_cleanup; exit \$status" 0 1 2 3 15 > > > > > > > > _cleanup() > > > > { > > > > cd / > > > > rm -f $tmp.* > > > > } > > > > > > > > Not a big deal, but it's better to maintain consistency :) > > > > > > > > > + > > > > > +# get standard environment, filters and checks > > > > > +. ./common/rc > > > > > +. ./common/filter > > > > > +. ./common/punch > > > > > + > > > > > +# real QA test starts here > > > > > +_supported_fs generic > > > > > +_supported_os Linux > > > > > + > > > > > +rm -f $seqres.full > > > > > +_require_scratch > > > > > +_require_scratch_shutdown > > > > > +_require_xfs_io_command "fpunch" > > > > > + > > > > > +_scratch_mkfs >/dev/null 2>&1 > > > > > +_scratch_mount > > > > > + > > > > > +testfile=$SCRATCH_MNT/testfile > > > > > + > > > > > +# check inode metadata after shutdown > > > > > +_check_inode_metadata() > > > > > > > > No need to name local functions with the leading "_", that's for global > > > > helper functions. > > > > > > > > > +{ > > > > > + src/godown $SCRATCH_MNT >> $seqres.full > > > > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.before > > > > > > > > Shouldn't we call godown *after* xfs_io -c stat? I saw EIO on this > > > > xfs_io command and all sub-tests reported stat diff. > > > > > > > > > > Yeah.. I haven't run the test, but I would expect pretty much anything > > > to return an error after an fs shutdown. > > > > > > > And perhaps we need to flush the log on godown for XFS? i.e. > > > > > > > > src/godown -f $SCRATCH_MNT >> $seqres.full > > > > > > > > > > I don't think this is necessary. The semantics of fsync() dictate that > > > the fs do what is necessary to make the file persistent on disk. This > > > means it is the fs responsibility to ensure the changes are logged on > > > disk. Indeed, xfs_file_fsync() calls _xfs_log_force_lsn() to flush the > > > log up to the most recent LSN that covered the inode in question. > > > > > > > Otherwise XFS fails all the "1024" & fsync tests (after I fixed the > > > > godown & xfs_io order locally), fdatasync tests are fine. > > > > > > > > @@ -1,8 +1,16 @@ > > > > QA output created by 391 > > > > ==== i_size 1024 test with fsync ==== > > > > +6c6 > > > > +< stat.blocks = 8200 > > > > +--- > > > > +> stat.blocks = 16256 > > > > ==== i_size 4096 test with fsync ==== > > > > ==== i_time test with fsync ==== > > > > ==== fpunch 1024 test with fsync ==== > > > > +6c6 > > > > +< stat.blocks = 8208 > > > > +--- > > > > +> stat.blocks = 24576 > > > > ==== fpunch 4096 test with fsync ==== > > > > > > > > Not sure if this is the expected behavior on XFS. cc'ed xfs list for > > > > some inputs. > > > > > > > > > > Am I reading this correctly that you're seeing more blocks than > > > expected? If so, preallocation perhaps? > > > > Yes, you're correct, I see more blocks after godown than before godown. > > > > I tried adding "-o allocsize=4k" to MOUNT_OPTIONS, it works but not > > always. e.g. on a host with sunit/swidth reported from underlying block > > device, test still fails. > > > > I'm not quite sure where the preallocation is coming from in that case. > It looks like it should honor allocsize, so that might be worth looking > into. > > > # xfs_info /mnt/xfs > > meta-data=/dev/mapper/systemvg-testlv2 isize=512 agcount=16, agsize=245696 blks > > = sectsz=512 attr=2, projid32bit=1 > > = crc=1 finobt=1 spinodes=0 rmapbt=0 > > = reflink=0 > > data = bsize=4096 blocks=3931136, imaxpct=25 > > = sunit=64 swidth=192 blks > > naming =version 2 bsize=4096 ascii-ci=0 ftype=1 > > log =internal bsize=4096 blocks=2560, version=2 > > = sectsz=512 sunit=64 blks, lazy-count=1 > > realtime =none extsz=4096 blocks=0, rtextents=0 > > > > Part of the test diff: > > ==== i_size 1024 test with fsync ==== > > +6c6 > > +< stat.blocks = 8200 > > +--- > > +> stat.blocks = 8704 > > > > On the other hand, adding "-f" to godown always works for me. > > > > I'm guessing the difference here is that fsync flushes the inode with > preallocation, but preallocation is typically cleaned up on file close > (when xfs_io exits). So a subsequent log flush at shutdown may flush > the transaction that clears out post-eof blocks. Note that it may also > hit the disk without the log forcing shutdown, it's just not guaranteed > in that case. > > The right thing to do is probably deal with preallocation explicitly in > the test. E.g., a truncate of the file to the current size after a > potentially preallocated write, but before the fsync, should always > result in an equivalent blocks count post-recovery. > > (Another angle here might be to consider whether the block count is the > right metric for this test, as opposed to a hexdump that validates the > written data actually made it to disk.). So, it seems it'd be better to avoid checking i_blocks of XFS because of such the preallocation, but for F2FS, I'd prefer to check i_blocks mismatch to detect any regression. Let me submit v2. Thanks, ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-17 8:35 ` [PATCH] generic/391: check inode metadata on f{data}sync after power-cut Eryu Guan 2016-11-17 12:56 ` Brian Foster @ 2016-11-17 18:31 ` Jaegeuk Kim 1 sibling, 0 replies; 17+ messages in thread From: Jaegeuk Kim @ 2016-11-17 18:31 UTC (permalink / raw) To: Eryu Guan; +Cc: fstests, linux-f2fs-devel, linux-xfs Hi, Thank you for the review. Yup, I'll submit v2 with changes for the comments except log flush. ;) Thanks, On Thu, Nov 17, 2016 at 04:35:27PM +0800, Eryu Guan wrote: > [cc'ed linux-xfs for inputs on a flush log issue] > > On Wed, Nov 16, 2016 at 07:27:53PM -0800, Jaegeuk Kim wrote: > > This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. > > > > The rule to check is: > > 1) fsync should guarantee all the inode metadata after power-cut, > > 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. > > > > Suggested-by: Chao Yu <yuchao0@huawei.com> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > tests/generic/391 | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > tests/generic/391.out | 11 +++++ > > tests/generic/group | 1 + > > 3 files changed, 137 insertions(+) > > create mode 100644 tests/generic/391 > > create mode 100644 tests/generic/391.out > > > > diff --git a/tests/generic/391 b/tests/generic/391 > > new file mode 100644 > > index 0000000..17c3fd3 > > --- /dev/null > > +++ b/tests/generic/391 > > @@ -0,0 +1,125 @@ > > +#! /bin/bash > > +# FS QA Test 391 > > +# > > +# Test inode's metadata after fsync or fdatasync calls. > > +# In the case of fsync, filesystem should recover all the inode metadata, while > > +# recovering i_blocks and i_size at least for fdatasync. > > +# > > +#----------------------------------------------------------------------- > > +# Copyright (c) 2016 Jaegeuk Kim. 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 "rm -f $tmp.*; exit \$status" 0 1 2 3 15 > > Better to follow the template and other tests on how to do cleanup, i.e. > trap _cleanup on exit and do real cleanups in it. e.g. > > trap "_cleanup; exit \$status" 0 1 2 3 15 > > _cleanup() > { > cd / > rm -f $tmp.* > } > > Not a big deal, but it's better to maintain consistency :) > > > + > > +# get standard environment, filters and checks > > +. ./common/rc > > +. ./common/filter > > +. ./common/punch > > + > > +# real QA test starts here > > +_supported_fs generic > > +_supported_os Linux > > + > > +rm -f $seqres.full > > +_require_scratch > > +_require_scratch_shutdown > > +_require_xfs_io_command "fpunch" > > + > > +_scratch_mkfs >/dev/null 2>&1 > > +_scratch_mount > > + > > +testfile=$SCRATCH_MNT/testfile > > + > > +# check inode metadata after shutdown > > +_check_inode_metadata() > > No need to name local functions with the leading "_", that's for global > helper functions. > > > +{ > > + src/godown $SCRATCH_MNT >> $seqres.full > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.before > > Shouldn't we call godown *after* xfs_io -c stat? I saw EIO on this > xfs_io command and all sub-tests reported stat diff. > > And perhaps we need to flush the log on godown for XFS? i.e. > > src/godown -f $SCRATCH_MNT >> $seqres.full > > Otherwise XFS fails all the "1024" & fsync tests (after I fixed the > godown & xfs_io order locally), fdatasync tests are fine. > > @@ -1,8 +1,16 @@ > QA output created by 391 > ==== i_size 1024 test with fsync ==== > +6c6 > +< stat.blocks = 8200 > +--- > +> stat.blocks = 16256 > ==== i_size 4096 test with fsync ==== > ==== i_time test with fsync ==== > ==== fpunch 1024 test with fsync ==== > +6c6 > +< stat.blocks = 8208 > +--- > +> stat.blocks = 24576 > ==== fpunch 4096 test with fsync ==== > > Not sure if this is the expected behavior on XFS. cc'ed xfs list for > some inputs. > > > + _scratch_cycle_mount > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after > > + diff $tmp.before $tmp.after >>$tmp.diff > > I think $tmp.diff should be overwritten on each sub-test. > > > + > > + if [ "$2" = "fdatasync" ]; then > > + cat $tmp.diff | grep stat.size > > + cat $tmp.diff | grep stat.blocks > > + else > > + cat $tmp.diff > > + fi > > + cat $tmp.before >> $seqres.full > > + cat $tmp.after >> $seqres.full > > +} > > + > > +# append XX KB with f{data}sync, followed by power-cut > > +_test_i_size() > > +{ > > + echo "==== i_size $2 test with $1 ====" > > I find it's much easier to read the $seqres.full log if we append these > kind of logs to $seqres.full too, e.g. > > echo "==== i_size $2 test with $1 ====" | tee -a $seqres.full > > Thanks, > Eryu > > > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > > + -c "fsync" \ > > + -c "pwrite 4M $2" \ > > + -c "$1" \ > > + $testfile >/dev/null > > + _check_inode_metadata $testfile $1 > > +} > > + > > +# update times with f{data}sync, followed by power-cut > > +_test_i_time() > > +{ > > + echo "==== i_time test with $1 ====" > > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > > + -c "fsync" \ > > + $testfile >/dev/null > > + sleep 1 > > + touch $testfile > > + $XFS_IO_PROG -c "$1" \ > > + $testfile >/dev/null > > + _check_inode_metadata $testfile $1 > > +} > > + > > +# punch XX KB with f{data}sync, followed by power-cut > > +_test_punch() > > +{ > > + echo "==== fpunch $2 test with $1 ====" > > + $XFS_IO_PROG -f -c "pwrite 0 4202496" \ > > + -c "fsync" \ > > + -c "fpunch 4194304 $2"\ > > + -c "$1" \ > > + $testfile >/dev/null > > + > > + _check_inode_metadata $testfile $1 > > +} > > + > > +for i in fsync fdatasync > > +do > > + _test_i_size $i 1024 > > + _test_i_size $i 4096 > > + _test_i_time $i > > + _test_punch $i 1024 > > + _test_punch $i 4096 > > +done > > + > > +rm -f $tmp.* > > + > > +# success, all done > > +status=0 > > +exit > > diff --git a/tests/generic/391.out b/tests/generic/391.out > > new file mode 100644 > > index 0000000..7c66776 > > --- /dev/null > > +++ b/tests/generic/391.out > > @@ -0,0 +1,11 @@ > > +QA output created by 391 > > +==== i_size 1024 test with fsync ==== > > +==== i_size 4096 test with fsync ==== > > +==== i_time test with fsync ==== > > +==== fpunch 1024 test with fsync ==== > > +==== fpunch 4096 test with fsync ==== > > +==== i_size 1024 test with fdatasync ==== > > +==== i_size 4096 test with fdatasync ==== > > +==== i_time test with fdatasync ==== > > +==== fpunch 1024 test with fdatasync ==== > > +==== fpunch 4096 test with fdatasync ==== > > diff --git a/tests/generic/group b/tests/generic/group > > index 08007d7..9de3415 100644 > > --- a/tests/generic/group > > +++ b/tests/generic/group > > @@ -392,3 +392,4 @@ > > 387 auto clone > > 388 auto log metadata > > 389 auto quick acl > > +391 auto quick metadata > > -- > > 2.8.3 > > > > -- > > 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] 17+ messages in thread
* Re: [PATCH v2] generic/391: check inode metadata on f{data}sync after power-cut [not found] <20161117032753.69315-1-jaegeuk@kernel.org> 2016-11-17 8:35 ` [PATCH] generic/391: check inode metadata on f{data}sync after power-cut Eryu Guan @ 2016-11-17 19:20 ` Jaegeuk Kim 2016-11-18 6:39 ` Eryu Guan 2016-11-18 19:45 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim 1 sibling, 2 replies; 17+ messages in thread From: Jaegeuk Kim @ 2016-11-17 19:20 UTC (permalink / raw) To: fstests, linux-f2fs-devel; +Cc: linux-xfs This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. The rule to check is: 1) fsync should guarantee all the inode metadata after power-cut, 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. Suggested-by: Chao Yu <yuchao0@huawei.com> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- tests/generic/391 | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/391.out | 11 ++++ tests/generic/group | 1 + 3 files changed, 148 insertions(+) create mode 100644 tests/generic/391 create mode 100644 tests/generic/391.out diff --git a/tests/generic/391 b/tests/generic/391 new file mode 100644 index 0000000..2b95151 --- /dev/null +++ b/tests/generic/391 @@ -0,0 +1,136 @@ +#! /bin/bash +# FS QA Test 391 +# +# Test inode's metadata after fsync or fdatasync calls. +# In the case of fsync, filesystem should recover all the inode metadata, while +# recovering i_blocks and i_size at least for fdatasync. +# +#----------------------------------------------------------------------- +# Copyright (c) 2016 Jaegeuk Kim. 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/punch + +# real QA test starts here +_supported_fs generic +_supported_os Linux + +rm -f $seqres.full +_require_scratch +_require_scratch_shutdown +_require_xfs_io_command "fpunch" + +_scratch_mkfs >/dev/null 2>&1 +_scratch_mount + +testfile=$SCRATCH_MNT/testfile + +# check inode metadata after shutdown +check_inode_metadata() +{ + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.before + src/godown $SCRATCH_MNT >> $seqres.full + _scratch_cycle_mount + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after + + if [ $FSTYP = xfs ]; then + sed -i '/stat.blocks/d' $tmp.before + sed -i '/stat.blocks/d' $tmp.after + fi + diff $tmp.before $tmp.after >$tmp.diff + + if [ "$2" = "fdatasync" ]; then + cat $tmp.diff | grep stat.size + cat $tmp.diff | grep stat.blocks + else + cat $tmp.diff $dont_care + fi + cat $tmp.before >> $seqres.full + cat $tmp.after >> $seqres.full +} + +# append XX KB with f{data}sync, followed by power-cut +test_i_size() +{ + echo "==== i_size $2 test with $1 ====" | tee -a $seqres.full + $XFS_IO_PROG -f -c "pwrite 0 4M" \ + -c "fsync" \ + -c "pwrite 4M $2" \ + -c "$1" \ + $testfile >/dev/null + check_inode_metadata $testfile $1 +} + +# update times with f{data}sync, followed by power-cut +test_i_time() +{ + echo "==== i_time test with $1 ====" | tee -a $seqres.full + $XFS_IO_PROG -f -c "pwrite 0 4M" \ + -c "fsync" \ + $testfile >/dev/null + sleep 1 + touch $testfile + $XFS_IO_PROG -c "$1" \ + $testfile >/dev/null + check_inode_metadata $testfile $1 +} + +# punch XX KB with f{data}sync, followed by power-cut +test_punch() +{ + echo "==== fpunch $2 test with $1 ====" | tee -a $seqres.full + $XFS_IO_PROG -f -c "pwrite 0 4202496" \ + -c "fsync" \ + -c "fpunch 4194304 $2"\ + -c "$1" \ + $testfile >/dev/null + + check_inode_metadata $testfile $1 +} + +for i in fsync fdatasync +do + test_i_size $i 1024 + test_i_size $i 4096 + test_i_time $i + test_punch $i 1024 + test_punch $i 4096 +done + +rm -f $tmp.* + +# success, all done +status=0 +exit diff --git a/tests/generic/391.out b/tests/generic/391.out new file mode 100644 index 0000000..7c66776 --- /dev/null +++ b/tests/generic/391.out @@ -0,0 +1,11 @@ +QA output created by 391 +==== i_size 1024 test with fsync ==== +==== i_size 4096 test with fsync ==== +==== i_time test with fsync ==== +==== fpunch 1024 test with fsync ==== +==== fpunch 4096 test with fsync ==== +==== i_size 1024 test with fdatasync ==== +==== i_size 4096 test with fdatasync ==== +==== i_time test with fdatasync ==== +==== fpunch 1024 test with fdatasync ==== +==== fpunch 4096 test with fdatasync ==== diff --git a/tests/generic/group b/tests/generic/group index 08007d7..9de3415 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -392,3 +392,4 @@ 387 auto clone 388 auto log metadata 389 auto quick acl +391 auto quick metadata -- 2.8.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-17 19:20 ` [PATCH v2] " Jaegeuk Kim @ 2016-11-18 6:39 ` Eryu Guan 2016-11-18 19:44 ` Jaegeuk Kim 2016-11-18 19:45 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim 1 sibling, 1 reply; 17+ messages in thread From: Eryu Guan @ 2016-11-18 6:39 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: fstests, linux-f2fs-devel, linux-xfs On Thu, Nov 17, 2016 at 11:20:51AM -0800, Jaegeuk Kim wrote: > This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. > > The rule to check is: > 1) fsync should guarantee all the inode metadata after power-cut, > 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. > > Suggested-by: Chao Yu <yuchao0@huawei.com> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > tests/generic/391 | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/generic/391.out | 11 ++++ > tests/generic/group | 1 + > 3 files changed, 148 insertions(+) > create mode 100644 tests/generic/391 > create mode 100644 tests/generic/391.out > > diff --git a/tests/generic/391 b/tests/generic/391 > new file mode 100644 > index 0000000..2b95151 > --- /dev/null > +++ b/tests/generic/391 > @@ -0,0 +1,136 @@ > +#! /bin/bash > +# FS QA Test 391 > +# > +# Test inode's metadata after fsync or fdatasync calls. > +# In the case of fsync, filesystem should recover all the inode metadata, while > +# recovering i_blocks and i_size at least for fdatasync. > +# > +#----------------------------------------------------------------------- > +# Copyright (c) 2016 Jaegeuk Kim. 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/punch > + > +# real QA test starts here > +_supported_fs generic > +_supported_os Linux > + > +rm -f $seqres.full > +_require_scratch > +_require_scratch_shutdown > +_require_xfs_io_command "fpunch" > + > +_scratch_mkfs >/dev/null 2>&1 > +_scratch_mount > + > +testfile=$SCRATCH_MNT/testfile > + > +# check inode metadata after shutdown > +check_inode_metadata() > +{ > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.before > + src/godown $SCRATCH_MNT >> $seqres.full > + _scratch_cycle_mount > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after > + > + if [ $FSTYP = xfs ]; then > + sed -i '/stat.blocks/d' $tmp.before > + sed -i '/stat.blocks/d' $tmp.after > + fi Blacklist some fs "randomly" seems not easy to maintain, at least we need some comments to explain why we do this. I prefer the other way Brian suggested, i.e. truncate the file to correct size before the real fsync/fdatasync to remove any preallocated blocks past eof. I made the following update and test passed on XFS without problems, what do you think? --- a/tests/generic/391 +++ b/tests/generic/391 @@ -65,10 +65,6 @@ check_inode_metadata() _scratch_cycle_mount $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after - if [ $FSTYP = xfs ]; then - sed -i '/stat.blocks/d' $tmp.before - sed -i '/stat.blocks/d' $tmp.after - fi diff $tmp.before $tmp.after >$tmp.diff if [ "$2" = "fdatasync" ]; then @@ -82,13 +78,19 @@ check_inode_metadata() } # append XX KB with f{data}sync, followed by power-cut +# truncate file to the current size to avoid post-eof preallocated +# blocks on XFS test_i_size() { + local len=$2 + local base_len=$((4 * 1024 * 1024)) + local new_len=$((base_len + len)) echo "==== i_size $2 test with $1 ====" | tee -a $seqres.full - $XFS_IO_PROG -f -c "pwrite 0 4M" \ - -c "fsync" \ - -c "pwrite 4M $2" \ - -c "$1" \ + $XFS_IO_PROG -f -c "pwrite 0 $base_len" \ + -c "fsync" \ + -c "pwrite $base_len $len" \ + -c "truncate $new_len" \ + -c "$1" \ $testfile >/dev/null check_inode_metadata $testfile $1 } @@ -108,12 +110,15 @@ test_i_time() } # punch XX KB with f{data}sync, followed by power-cut +# truncate file to the current size to avoid post-eof preallocated +# blocks on XFS test_punch() { echo "==== fpunch $2 test with $1 ====" | tee -a $seqres.full $XFS_IO_PROG -f -c "pwrite 0 4202496" \ -c "fsync" \ - -c "fpunch 4194304 $2"\ + -c "fpunch 4194304 $2" \ + -c "truncate 4202496" \ -c "$1" \ $testfile >/dev/null Thanks, Eryu ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-18 6:39 ` Eryu Guan @ 2016-11-18 19:44 ` Jaegeuk Kim 2016-11-19 0:42 ` Brian Foster 0 siblings, 1 reply; 17+ messages in thread From: Jaegeuk Kim @ 2016-11-18 19:44 UTC (permalink / raw) To: Eryu Guan; +Cc: fstests, linux-f2fs-devel, linux-xfs On Fri, Nov 18, 2016 at 02:39:43PM +0800, Eryu Guan wrote: > On Thu, Nov 17, 2016 at 11:20:51AM -0800, Jaegeuk Kim wrote: > > This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. > > > > The rule to check is: > > 1) fsync should guarantee all the inode metadata after power-cut, > > 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. > > > > Suggested-by: Chao Yu <yuchao0@huawei.com> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > tests/generic/391 | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > tests/generic/391.out | 11 ++++ > > tests/generic/group | 1 + > > 3 files changed, 148 insertions(+) > > create mode 100644 tests/generic/391 > > create mode 100644 tests/generic/391.out > > > > diff --git a/tests/generic/391 b/tests/generic/391 > > new file mode 100644 > > index 0000000..2b95151 > > --- /dev/null > > +++ b/tests/generic/391 > > @@ -0,0 +1,136 @@ > > +#! /bin/bash > > +# FS QA Test 391 > > +# > > +# Test inode's metadata after fsync or fdatasync calls. > > +# In the case of fsync, filesystem should recover all the inode metadata, while > > +# recovering i_blocks and i_size at least for fdatasync. > > +# > > +#----------------------------------------------------------------------- > > +# Copyright (c) 2016 Jaegeuk Kim. 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/punch > > + > > +# real QA test starts here > > +_supported_fs generic > > +_supported_os Linux > > + > > +rm -f $seqres.full > > +_require_scratch > > +_require_scratch_shutdown > > +_require_xfs_io_command "fpunch" > > + > > +_scratch_mkfs >/dev/null 2>&1 > > +_scratch_mount > > + > > +testfile=$SCRATCH_MNT/testfile > > + > > +# check inode metadata after shutdown > > +check_inode_metadata() > > +{ > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.before > > + src/godown $SCRATCH_MNT >> $seqres.full > > + _scratch_cycle_mount > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after > > + > > + if [ $FSTYP = xfs ]; then > > + sed -i '/stat.blocks/d' $tmp.before > > + sed -i '/stat.blocks/d' $tmp.after > > + fi > > Blacklist some fs "randomly" seems not easy to maintain, at least we > need some comments to explain why we do this. > > I prefer the other way Brian suggested, i.e. truncate the file to > correct size before the real fsync/fdatasync to remove any preallocated > blocks past eof. I made the following update and test passed on XFS > without problems, what do you think? I don't think that is a right way, since it breaks the existing IO behavior. I tested a little bit, and it seems the key is to call fclose to truncate out-of-eof blocks. Indeed, I could get the expected i_blocks by adding open and close after shutdown->remount; please check v3. BTW, does it make sense user needs to do fclose after power-cut to reclaim such the preallocated blocks? I feel that XFS must reclaim them during recovery on mount. Thanks, > > --- a/tests/generic/391 > +++ b/tests/generic/391 > @@ -65,10 +65,6 @@ check_inode_metadata() > _scratch_cycle_mount > $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after > > - if [ $FSTYP = xfs ]; then > - sed -i '/stat.blocks/d' $tmp.before > - sed -i '/stat.blocks/d' $tmp.after > - fi > diff $tmp.before $tmp.after >$tmp.diff > > if [ "$2" = "fdatasync" ]; then > @@ -82,13 +78,19 @@ check_inode_metadata() > } > > # append XX KB with f{data}sync, followed by power-cut > +# truncate file to the current size to avoid post-eof preallocated > +# blocks on XFS > test_i_size() > { > + local len=$2 > + local base_len=$((4 * 1024 * 1024)) > + local new_len=$((base_len + len)) > echo "==== i_size $2 test with $1 ====" | tee -a $seqres.full > - $XFS_IO_PROG -f -c "pwrite 0 4M" \ > - -c "fsync" \ > - -c "pwrite 4M $2" \ > - -c "$1" \ > + $XFS_IO_PROG -f -c "pwrite 0 $base_len" \ > + -c "fsync" \ > + -c "pwrite $base_len $len" \ > + -c "truncate $new_len" \ > + -c "$1" \ > $testfile >/dev/null > check_inode_metadata $testfile $1 > } > @@ -108,12 +110,15 @@ test_i_time() > } > > # punch XX KB with f{data}sync, followed by power-cut > +# truncate file to the current size to avoid post-eof preallocated > +# blocks on XFS > test_punch() > { > echo "==== fpunch $2 test with $1 ====" | tee -a $seqres.full > $XFS_IO_PROG -f -c "pwrite 0 4202496" \ > -c "fsync" \ > - -c "fpunch 4194304 $2"\ > + -c "fpunch 4194304 $2" \ > + -c "truncate 4202496" \ > -c "$1" \ > $testfile >/dev/null > > Thanks, > Eryu ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-18 19:44 ` Jaegeuk Kim @ 2016-11-19 0:42 ` Brian Foster 2016-11-19 1:56 ` Jaegeuk Kim 0 siblings, 1 reply; 17+ messages in thread From: Brian Foster @ 2016-11-19 0:42 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: Eryu Guan, fstests, linux-f2fs-devel, linux-xfs On Fri, Nov 18, 2016 at 11:44:42AM -0800, Jaegeuk Kim wrote: > On Fri, Nov 18, 2016 at 02:39:43PM +0800, Eryu Guan wrote: > > On Thu, Nov 17, 2016 at 11:20:51AM -0800, Jaegeuk Kim wrote: > > > This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. > > > > > > The rule to check is: > > > 1) fsync should guarantee all the inode metadata after power-cut, > > > 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. > > > > > > Suggested-by: Chao Yu <yuchao0@huawei.com> > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > --- > > > tests/generic/391 | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > tests/generic/391.out | 11 ++++ > > > tests/generic/group | 1 + > > > 3 files changed, 148 insertions(+) > > > create mode 100644 tests/generic/391 > > > create mode 100644 tests/generic/391.out > > > > > > diff --git a/tests/generic/391 b/tests/generic/391 > > > new file mode 100644 > > > index 0000000..2b95151 > > > --- /dev/null > > > +++ b/tests/generic/391 > > > @@ -0,0 +1,136 @@ > > > +#! /bin/bash > > > +# FS QA Test 391 > > > +# > > > +# Test inode's metadata after fsync or fdatasync calls. > > > +# In the case of fsync, filesystem should recover all the inode metadata, while > > > +# recovering i_blocks and i_size at least for fdatasync. > > > +# > > > +#----------------------------------------------------------------------- > > > +# Copyright (c) 2016 Jaegeuk Kim. 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/punch > > > + > > > +# real QA test starts here > > > +_supported_fs generic > > > +_supported_os Linux > > > + > > > +rm -f $seqres.full > > > +_require_scratch > > > +_require_scratch_shutdown > > > +_require_xfs_io_command "fpunch" > > > + > > > +_scratch_mkfs >/dev/null 2>&1 > > > +_scratch_mount > > > + > > > +testfile=$SCRATCH_MNT/testfile > > > + > > > +# check inode metadata after shutdown > > > +check_inode_metadata() > > > +{ > > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.before > > > + src/godown $SCRATCH_MNT >> $seqres.full > > > + _scratch_cycle_mount > > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after > > > + > > > + if [ $FSTYP = xfs ]; then > > > + sed -i '/stat.blocks/d' $tmp.before > > > + sed -i '/stat.blocks/d' $tmp.after > > > + fi > > > > Blacklist some fs "randomly" seems not easy to maintain, at least we > > need some comments to explain why we do this. > > > > I prefer the other way Brian suggested, i.e. truncate the file to > > correct size before the real fsync/fdatasync to remove any preallocated > > blocks past eof. I made the following update and test passed on XFS > > without problems, what do you think? > > I don't think that is a right way, since it breaks the existing IO behavior. > I tested a little bit, and it seems the key is to call fclose to truncate > out-of-eof blocks. Indeed, I could get the expected i_blocks by adding open and > close after shutdown->remount; please check v3. > I'm not sure what you mean by truncate breaking existing behavior. FWIW, you can probably get similar behavior by issuing the fsync in a subsequent xfs_io call rather than adding the '-c fsync' to the same xfs_io call that issues the write. This is because in most cases XFS cleans up such preallocation on file close. Do note however that if a file is repeatedly open-write-closed, the preallocation can stick around longer. > BTW, does it make sense user needs to do fclose after power-cut to reclaim such > the preallocated blocks? I feel that XFS must reclaim them during recovery on > mount. > I'm not sure there's much we can do about this, particularly since XFS uses physical style logging and thus it's not straightforward to tell when log recovery is extending the bmap of a file beyond i_size. Even then, there are cases where we can't free post-eof blocks even if we know they exist, such as when an inode has explicit (fallocate) preallocation. Brian > Thanks, > > > > > --- a/tests/generic/391 > > +++ b/tests/generic/391 > > @@ -65,10 +65,6 @@ check_inode_metadata() > > _scratch_cycle_mount > > $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after > > > > - if [ $FSTYP = xfs ]; then > > - sed -i '/stat.blocks/d' $tmp.before > > - sed -i '/stat.blocks/d' $tmp.after > > - fi > > diff $tmp.before $tmp.after >$tmp.diff > > > > if [ "$2" = "fdatasync" ]; then > > @@ -82,13 +78,19 @@ check_inode_metadata() > > } > > > > # append XX KB with f{data}sync, followed by power-cut > > +# truncate file to the current size to avoid post-eof preallocated > > +# blocks on XFS > > test_i_size() > > { > > + local len=$2 > > + local base_len=$((4 * 1024 * 1024)) > > + local new_len=$((base_len + len)) > > echo "==== i_size $2 test with $1 ====" | tee -a $seqres.full > > - $XFS_IO_PROG -f -c "pwrite 0 4M" \ > > - -c "fsync" \ > > - -c "pwrite 4M $2" \ > > - -c "$1" \ > > + $XFS_IO_PROG -f -c "pwrite 0 $base_len" \ > > + -c "fsync" \ > > + -c "pwrite $base_len $len" \ > > + -c "truncate $new_len" \ > > + -c "$1" \ > > $testfile >/dev/null > > check_inode_metadata $testfile $1 > > } > > @@ -108,12 +110,15 @@ test_i_time() > > } > > > > # punch XX KB with f{data}sync, followed by power-cut > > +# truncate file to the current size to avoid post-eof preallocated > > +# blocks on XFS > > test_punch() > > { > > echo "==== fpunch $2 test with $1 ====" | tee -a $seqres.full > > $XFS_IO_PROG -f -c "pwrite 0 4202496" \ > > -c "fsync" \ > > - -c "fpunch 4194304 $2"\ > > + -c "fpunch 4194304 $2" \ > > + -c "truncate 4202496" \ > > -c "$1" \ > > $testfile >/dev/null > > > > Thanks, > > Eryu > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" 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] 17+ messages in thread
* Re: [PATCH v2] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-19 0:42 ` Brian Foster @ 2016-11-19 1:56 ` Jaegeuk Kim 0 siblings, 0 replies; 17+ messages in thread From: Jaegeuk Kim @ 2016-11-19 1:56 UTC (permalink / raw) To: Brian Foster; +Cc: Eryu Guan, fstests, linux-f2fs-devel, linux-xfs On Fri, Nov 18, 2016 at 07:42:17PM -0500, Brian Foster wrote: > On Fri, Nov 18, 2016 at 11:44:42AM -0800, Jaegeuk Kim wrote: > > On Fri, Nov 18, 2016 at 02:39:43PM +0800, Eryu Guan wrote: > > > On Thu, Nov 17, 2016 at 11:20:51AM -0800, Jaegeuk Kim wrote: > > > > This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. > > > > > > > > The rule to check is: > > > > 1) fsync should guarantee all the inode metadata after power-cut, > > > > 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. > > > > > > > > Suggested-by: Chao Yu <yuchao0@huawei.com> > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > --- > > > > tests/generic/391 | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > tests/generic/391.out | 11 ++++ > > > > tests/generic/group | 1 + > > > > 3 files changed, 148 insertions(+) > > > > create mode 100644 tests/generic/391 > > > > create mode 100644 tests/generic/391.out > > > > > > > > diff --git a/tests/generic/391 b/tests/generic/391 > > > > new file mode 100644 > > > > index 0000000..2b95151 > > > > --- /dev/null > > > > +++ b/tests/generic/391 > > > > @@ -0,0 +1,136 @@ > > > > +#! /bin/bash > > > > +# FS QA Test 391 > > > > +# > > > > +# Test inode's metadata after fsync or fdatasync calls. > > > > +# In the case of fsync, filesystem should recover all the inode metadata, while > > > > +# recovering i_blocks and i_size at least for fdatasync. > > > > +# > > > > +#----------------------------------------------------------------------- > > > > +# Copyright (c) 2016 Jaegeuk Kim. 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/punch > > > > + > > > > +# real QA test starts here > > > > +_supported_fs generic > > > > +_supported_os Linux > > > > + > > > > +rm -f $seqres.full > > > > +_require_scratch > > > > +_require_scratch_shutdown > > > > +_require_xfs_io_command "fpunch" > > > > + > > > > +_scratch_mkfs >/dev/null 2>&1 > > > > +_scratch_mount > > > > + > > > > +testfile=$SCRATCH_MNT/testfile > > > > + > > > > +# check inode metadata after shutdown > > > > +check_inode_metadata() > > > > +{ > > > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.before > > > > + src/godown $SCRATCH_MNT >> $seqres.full > > > > + _scratch_cycle_mount > > > > + $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after > > > > + > > > > + if [ $FSTYP = xfs ]; then > > > > + sed -i '/stat.blocks/d' $tmp.before > > > > + sed -i '/stat.blocks/d' $tmp.after > > > > + fi > > > > > > Blacklist some fs "randomly" seems not easy to maintain, at least we > > > need some comments to explain why we do this. > > > > > > I prefer the other way Brian suggested, i.e. truncate the file to > > > correct size before the real fsync/fdatasync to remove any preallocated > > > blocks past eof. I made the following update and test passed on XFS > > > without problems, what do you think? > > > > I don't think that is a right way, since it breaks the existing IO behavior. > > I tested a little bit, and it seems the key is to call fclose to truncate > > out-of-eof blocks. Indeed, I could get the expected i_blocks by adding open and > > close after shutdown->remount; please check v3. > > > > I'm not sure what you mean by truncate breaking existing behavior. FWIW, What I mean is this testcase was intended to check inode's metadata, and if we do truncate the file before fsync, I believe that would become just another testcase, when treating filesystem as a black box. > you can probably get similar behavior by issuing the fsync in a > subsequent xfs_io call rather than adding the '-c fsync' to the same > xfs_io call that issues the write. This is because in most cases XFS > cleans up such preallocation on file close. Do note however that if a > file is repeatedly open-write-closed, the preallocation can stick around > longer. Oh, right. Indeed, that is a better choice which makes sence to me. Also, I put $testfile deletion in each iteration. > > BTW, does it make sense user needs to do fclose after power-cut to reclaim such > > the preallocated blocks? I feel that XFS must reclaim them during recovery on > > mount. > > > > I'm not sure there's much we can do about this, particularly since XFS > uses physical style logging and thus it's not straightforward to tell > when log recovery is extending the bmap of a file beyond i_size. Even > then, there are cases where we can't free post-eof blocks even if we > know they exist, such as when an inode has explicit (fallocate) > preallocation. Okay, I see. Thank you for the explanation. Thanks, > > Brian > > > Thanks, > > > > > > > > --- a/tests/generic/391 > > > +++ b/tests/generic/391 > > > @@ -65,10 +65,6 @@ check_inode_metadata() > > > _scratch_cycle_mount > > > $XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after > > > > > > - if [ $FSTYP = xfs ]; then > > > - sed -i '/stat.blocks/d' $tmp.before > > > - sed -i '/stat.blocks/d' $tmp.after > > > - fi > > > diff $tmp.before $tmp.after >$tmp.diff > > > > > > if [ "$2" = "fdatasync" ]; then > > > @@ -82,13 +78,19 @@ check_inode_metadata() > > > } > > > > > > # append XX KB with f{data}sync, followed by power-cut > > > +# truncate file to the current size to avoid post-eof preallocated > > > +# blocks on XFS > > > test_i_size() > > > { > > > + local len=$2 > > > + local base_len=$((4 * 1024 * 1024)) > > > + local new_len=$((base_len + len)) > > > echo "==== i_size $2 test with $1 ====" | tee -a $seqres.full > > > - $XFS_IO_PROG -f -c "pwrite 0 4M" \ > > > - -c "fsync" \ > > > - -c "pwrite 4M $2" \ > > > - -c "$1" \ > > > + $XFS_IO_PROG -f -c "pwrite 0 $base_len" \ > > > + -c "fsync" \ > > > + -c "pwrite $base_len $len" \ > > > + -c "truncate $new_len" \ > > > + -c "$1" \ > > > $testfile >/dev/null > > > check_inode_metadata $testfile $1 > > > } > > > @@ -108,12 +110,15 @@ test_i_time() > > > } > > > > > > # punch XX KB with f{data}sync, followed by power-cut > > > +# truncate file to the current size to avoid post-eof preallocated > > > +# blocks on XFS > > > test_punch() > > > { > > > echo "==== fpunch $2 test with $1 ====" | tee -a $seqres.full > > > $XFS_IO_PROG -f -c "pwrite 0 4202496" \ > > > -c "fsync" \ > > > - -c "fpunch 4194304 $2"\ > > > + -c "fpunch 4194304 $2" \ > > > + -c "truncate 4202496" \ > > > -c "$1" \ > > > $testfile >/dev/null > > > > > > Thanks, > > > Eryu > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" 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] 17+ messages in thread
* Re: [f2fs-dev] [PATCH v3] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-17 19:20 ` [PATCH v2] " Jaegeuk Kim 2016-11-18 6:39 ` Eryu Guan @ 2016-11-18 19:45 ` Jaegeuk Kim 2016-11-19 1:57 ` [f2fs-dev] [PATCH v4] " Jaegeuk Kim 1 sibling, 1 reply; 17+ messages in thread From: Jaegeuk Kim @ 2016-11-18 19:45 UTC (permalink / raw) To: fstests, linux-f2fs-devel; +Cc: linux-xfs This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. The rule to check is: 1) fsync should guarantee all the inode metadata after power-cut, 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. Note that, in XFS, it allocates more blocks when doing writes, and truncates them during fclose. If power-cut happens after fsync, recovery starts with the preallocated blocks. So, it needs to truncate them by fclose after mount. Suggested-by: Chao Yu <yuchao0@huawei.com> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- tests/generic/391 | 140 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/391.out | 11 ++++ tests/generic/group | 1 + 3 files changed, 152 insertions(+) create mode 100755 tests/generic/391 create mode 100644 tests/generic/391.out diff --git a/tests/generic/391 b/tests/generic/391 new file mode 100755 index 0000000..c6bb63e --- /dev/null +++ b/tests/generic/391 @@ -0,0 +1,140 @@ +#! /bin/bash +# FS QA Test 391 +# +# Test inode's metadata after fsync or fdatasync calls. +# In the case of fsync, filesystem should recover all the inode metadata, while +# recovering i_blocks and i_size at least for fdatasync. +# +#----------------------------------------------------------------------- +# Copyright (c) 2016 Jaegeuk Kim. 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/punch + +# real QA test starts here +_supported_fs generic +_supported_os Linux + +rm -f $seqres.full +_require_scratch +_require_scratch_shutdown +_require_xfs_io_command "fpunch" + +_scratch_mkfs >/dev/null 2>&1 +_scratch_mount + +testfile=$SCRATCH_MNT/testfile + +stat() +{ + $XFS_IO_PROG -r -c "stat -v" $testfile >$tmp.$1 +} + +# check inode metadata after shutdown +check_inode_metadata() +{ + stat before + src/godown $SCRATCH_MNT >> $seqres.full + _scratch_cycle_mount + + # XFS needs to truncate preallocated blocks by close + $XFS_IO_PROG -c "open" $testfile >/dev/null + stat after + + diff $tmp.before $tmp.after >$tmp.diff + + if [ "$1" = "fdatasync" ]; then + cat $tmp.diff | grep stat.size + cat $tmp.diff | grep stat.blocks + else + cat $tmp.diff + fi + cat $tmp.before >> $seqres.full + cat $tmp.after >> $seqres.full +} + +# append XX KB with f{data}sync, followed by power-cut +test_i_size() +{ + echo "==== i_size $2 test with $1 ====" | tee -a $seqres.full + $XFS_IO_PROG -f -c "pwrite 0 4M" \ + -c "fsync" \ + -c "pwrite 4M $2" \ + -c "$1" \ + $testfile >/dev/null + check_inode_metadata $1 +} + +# update times with f{data}sync, followed by power-cut +test_i_time() +{ + echo "==== i_time test with $1 ====" | tee -a $seqres.full + $XFS_IO_PROG -f -c "pwrite 0 4M" \ + -c "fsync" \ + $testfile >/dev/null + sleep 1 + touch $testfile + $XFS_IO_PROG -c "$1" \ + $testfile >/dev/null + check_inode_metadata $1 +} + +# punch XX KB with f{data}sync, followed by power-cut +test_punch() +{ + echo "==== fpunch $2 test with $1 ====" | tee -a $seqres.full + $XFS_IO_PROG -f -c "pwrite 0 4202496" \ + -c "fsync" \ + -c "fpunch 4194304 $2"\ + -c "$1" \ + $testfile >/dev/null + + check_inode_metadata $1 +} + +for i in fsync fdatasync +do + test_i_size $i 1024 + test_i_size $i 4096 + test_i_time $i + test_punch $i 1024 + test_punch $i 4096 +done + +rm -f $tmp.* + +# success, all done +status=0 +exit diff --git a/tests/generic/391.out b/tests/generic/391.out new file mode 100644 index 0000000..7c66776 --- /dev/null +++ b/tests/generic/391.out @@ -0,0 +1,11 @@ +QA output created by 391 +==== i_size 1024 test with fsync ==== +==== i_size 4096 test with fsync ==== +==== i_time test with fsync ==== +==== fpunch 1024 test with fsync ==== +==== fpunch 4096 test with fsync ==== +==== i_size 1024 test with fdatasync ==== +==== i_size 4096 test with fdatasync ==== +==== i_time test with fdatasync ==== +==== fpunch 1024 test with fdatasync ==== +==== fpunch 4096 test with fdatasync ==== diff --git a/tests/generic/group b/tests/generic/group index 08007d7..9de3415 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -392,3 +392,4 @@ 387 auto clone 388 auto log metadata 389 auto quick acl +391 auto quick metadata -- 2.8.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [f2fs-dev] [PATCH v4] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-18 19:45 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim @ 2016-11-19 1:57 ` Jaegeuk Kim 2016-11-20 21:19 ` Dave Chinner 2016-11-21 20:02 ` [f2fs-dev] [PATCH v5] " Jaegeuk Kim 0 siblings, 2 replies; 17+ messages in thread From: Jaegeuk Kim @ 2016-11-19 1:57 UTC (permalink / raw) To: fstests, linux-f2fs-devel; +Cc: linux-xfs This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. The rule to check is: 1) fsync should guarantee all the inode metadata after power-cut, 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. Note that, in XFS, it allocates more blocks when doing writes, so it should close the file before fsync in order to get actual i_blocks after power-cut. Suggested-by: Chao Yu <yuchao0@huawei.com> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- tests/generic/391 | 149 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/391.out | 11 ++++ tests/generic/group | 1 + 3 files changed, 161 insertions(+) create mode 100755 tests/generic/391 create mode 100644 tests/generic/391.out diff --git a/tests/generic/391 b/tests/generic/391 new file mode 100755 index 0000000..121e01d --- /dev/null +++ b/tests/generic/391 @@ -0,0 +1,149 @@ +#! /bin/bash +# FS QA Test 391 +# +# Test inode's metadata after fsync or fdatasync calls. +# In the case of fsync, filesystem should recover all the inode metadata, while +# recovering i_blocks and i_size at least for fdatasync. +# +#----------------------------------------------------------------------- +# Copyright (c) 2016 Jaegeuk Kim. 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/punch + +# real QA test starts here +_supported_fs generic +_supported_os Linux + +rm -f $seqres.full +_require_scratch +_require_scratch_shutdown +_require_xfs_io_command "fpunch" + +_scratch_mkfs >/dev/null 2>&1 +_scratch_mount + +testfile=$SCRATCH_MNT/testfile + +f_stat() +{ + $XFS_IO_PROG -r -c "stat -v" $testfile >$tmp.$1 +} + +f_sync() +{ + $XFS_IO_PROG -c "$1" $testfile >/dev/null +} + +# check inode metadata after shutdown +check_inode_metadata() +{ + # prepare syncing metadata + f_sync $1 + + # get inode metadata #1 + f_stat before + + # shutdown + src/godown $SCRATCH_MNT >> $seqres.full + _scratch_cycle_mount + + # get inode metadata #2 + f_stat after + + # compare #1 and #2 + diff $tmp.before $tmp.after >$tmp.diff + + # fdatasync only cares about i_size and i_blocks + if [ "$1" = "fdatasync" ]; then + cat $tmp.diff | grep stat.size + cat $tmp.diff | grep stat.blocks + else + cat $tmp.diff + fi + cat $tmp.before >> $seqres.full + cat $tmp.after >> $seqres.full + + rm $testfile +} + +# append XX KB with f{data}sync, followed by power-cut +test_i_size() +{ + echo "==== i_size $2 test with $1 ====" | tee -a $seqres.full + $XFS_IO_PROG -f -c "pwrite 0 4M" \ + -c "fsync" \ + -c "pwrite 4M $2" \ + $testfile >/dev/null + check_inode_metadata $1 +} + +# update times with f{data}sync, followed by power-cut +test_i_time() +{ + echo "==== i_time test with $1 ====" | tee -a $seqres.full + $XFS_IO_PROG -f -c "pwrite 0 4M" \ + -c "fsync" \ + $testfile >/dev/null + sleep 1 + touch $testfile + check_inode_metadata $1 +} + +# punch XX KB with f{data}sync, followed by power-cut +test_punch() +{ + echo "==== fpunch $2 test with $1 ====" | tee -a $seqres.full + $XFS_IO_PROG -f -c "pwrite 0 4202496" \ + -c "fsync" \ + -c "fpunch 4194304 $2"\ + $testfile >/dev/null + check_inode_metadata $1 +} + +for i in fsync fdatasync +do + test_i_size $i 1024 + test_i_size $i 4096 + test_i_time $i + test_punch $i 1024 + test_punch $i 4096 +done + +rm -f $tmp.* + +# success, all done +status=0 +exit diff --git a/tests/generic/391.out b/tests/generic/391.out new file mode 100644 index 0000000..7c66776 --- /dev/null +++ b/tests/generic/391.out @@ -0,0 +1,11 @@ +QA output created by 391 +==== i_size 1024 test with fsync ==== +==== i_size 4096 test with fsync ==== +==== i_time test with fsync ==== +==== fpunch 1024 test with fsync ==== +==== fpunch 4096 test with fsync ==== +==== i_size 1024 test with fdatasync ==== +==== i_size 4096 test with fdatasync ==== +==== i_time test with fdatasync ==== +==== fpunch 1024 test with fdatasync ==== +==== fpunch 4096 test with fdatasync ==== diff --git a/tests/generic/group b/tests/generic/group index 08007d7..9de3415 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -392,3 +392,4 @@ 387 auto clone 388 auto log metadata 389 auto quick acl +391 auto quick metadata -- 2.8.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [f2fs-dev] [PATCH v4] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-19 1:57 ` [f2fs-dev] [PATCH v4] " Jaegeuk Kim @ 2016-11-20 21:19 ` Dave Chinner 2016-11-21 20:00 ` Jaegeuk Kim 2016-11-21 20:02 ` [f2fs-dev] [PATCH v5] " Jaegeuk Kim 1 sibling, 1 reply; 17+ messages in thread From: Dave Chinner @ 2016-11-20 21:19 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: fstests, linux-f2fs-devel, linux-xfs On Fri, Nov 18, 2016 at 05:57:45PM -0800, Jaegeuk Kim wrote: > This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. > > The rule to check is: > 1) fsync should guarantee all the inode metadata after power-cut, > 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. > > Note that, in XFS, it allocates more blocks when doing writes, so it should > close the file before fsync in order to get actual i_blocks after power-cut. > > Suggested-by: Chao Yu <yuchao0@huawei.com> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > tests/generic/391 | 149 ++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/generic/391.out | 11 ++++ > tests/generic/group | 1 + > 3 files changed, 161 insertions(+) > create mode 100755 tests/generic/391 > create mode 100644 tests/generic/391.out > > diff --git a/tests/generic/391 b/tests/generic/391 > new file mode 100755 > index 0000000..121e01d > --- /dev/null > +++ b/tests/generic/391 > @@ -0,0 +1,149 @@ > +#! /bin/bash > +# FS QA Test 391 > +# > +# Test inode's metadata after fsync or fdatasync calls. > +# In the case of fsync, filesystem should recover all the inode metadata, while > +# recovering i_blocks and i_size at least for fdatasync. > +# > +#----------------------------------------------------------------------- > +# Copyright (c) 2016 Jaegeuk Kim. 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/punch > + > +# real QA test starts here > +_supported_fs generic > +_supported_os Linux > + > +rm -f $seqres.full > +_require_scratch > +_require_scratch_shutdown > +_require_xfs_io_command "fpunch" > + > +_scratch_mkfs >/dev/null 2>&1 > +_scratch_mount > + > +testfile=$SCRATCH_MNT/testfile > + > +f_stat() > +{ > + $XFS_IO_PROG -r -c "stat -v" $testfile >$tmp.$1 > +} > + > +f_sync() > +{ > + $XFS_IO_PROG -c "$1" $testfile >/dev/null > +} This is factoring overkill and... > + > +# check inode metadata after shutdown > +check_inode_metadata() > +{ > + # prepare syncing metadata > + f_sync $1 > + > + # get inode metadata #1 > + f_stat before > + > + # shutdown > + src/godown $SCRATCH_MNT >> $seqres.full > + _scratch_cycle_mount > + > + # get inode metadata #2 > + f_stat after > + > + # compare #1 and #2 > + diff $tmp.before $tmp.after >$tmp.diff > + > + # fdatasync only cares about i_size and i_blocks > + if [ "$1" = "fdatasync" ]; then > + cat $tmp.diff | grep stat.size > + cat $tmp.diff | grep stat.blocks > + else > + cat $tmp.diff > + fi .... forces you to do things like this. We have the stat binary for getting specific fields from files. We use it all over the place for exactly this sort of thing. See, for example, _defrag(), which also does this "stat before/stat after/compare results" for specific metadata. i.e.: $ stat -c "blocks: %b size: %s" Makefile blocks: 8 size: 3069 $ check_inode_metadata() { sync_mode=$1 stat_opts="" if [ $sync_mode = "fdatasync" ]; then stat_opts='-c "blocks: %b size: %s"' fi $XFS_IO_PROG -c "$sync_mode" $testfile | _filter_xfs_io before=`stat $stat_opts $testfile` ... after=`stat $stat_opts $testfile` if [ $before != $after ]; then echo "Before: $before" echo "After: $after" status=1; # this is a failure! fi > + cat $tmp.before >> $seqres.full > + cat $tmp.after >> $seqres.full > + > + rm $testfile > +} > + > +# append XX KB with f{data}sync, followed by power-cut > +test_i_size() > +{ > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > + -c "fsync" \ > + -c "pwrite 4M $2" \ > + $testfile >/dev/null Make these all truncate to the final size first like this: $XFS_IO_PROG -f -c "truncate 4M" \ -c "pwrite 0 4M" \ -c "fsync" \ -c "pwrite 4M $2" \ $testfile | _filter_xfs_io check_inode_metadata $1 And that will make all the problems with XFS preallocation go away, because the initial truncate to set the file size will turn off the preallocation. > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > + -c "fsync" \ > + $testfile >/dev/null > + sleep 1 > + touch $testfile > + check_inode_metadata $1 > +} > + > +# punch XX KB with f{data}sync, followed by power-cut > +test_punch() > +{ > + echo "==== fpunch $2 test with $1 ====" | tee -a $seqres.full > + $XFS_IO_PROG -f -c "pwrite 0 4202496" \ > + -c "fsync" \ > + -c "fpunch 4194304 $2"\ > + $testfile >/dev/null > + check_inode_metadata $1 > +} > + > +for i in fsync fdatasync > +do > + test_i_size $i 1024 > + test_i_size $i 4096 > + test_i_time $i > + test_punch $i 1024 > + test_punch $i 4096 > +done > + > +rm -f $tmp.* No need for this here. > + > +# success, all done > +status=0 Set that at the start before we run the test so that if we detect a failure we can set status=1 for exit to handle appropriately. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [f2fs-dev] [PATCH v4] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-20 21:19 ` Dave Chinner @ 2016-11-21 20:00 ` Jaegeuk Kim 0 siblings, 0 replies; 17+ messages in thread From: Jaegeuk Kim @ 2016-11-21 20:00 UTC (permalink / raw) To: Dave Chinner; +Cc: fstests, linux-f2fs-devel, linux-xfs On Mon, Nov 21, 2016 at 08:19:58AM +1100, Dave Chinner wrote: > On Fri, Nov 18, 2016 at 05:57:45PM -0800, Jaegeuk Kim wrote: > > This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. > > > > The rule to check is: > > 1) fsync should guarantee all the inode metadata after power-cut, > > 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. > > > > Note that, in XFS, it allocates more blocks when doing writes, so it should > > close the file before fsync in order to get actual i_blocks after power-cut. > > > > Suggested-by: Chao Yu <yuchao0@huawei.com> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > tests/generic/391 | 149 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > tests/generic/391.out | 11 ++++ > > tests/generic/group | 1 + > > 3 files changed, 161 insertions(+) > > create mode 100755 tests/generic/391 > > create mode 100644 tests/generic/391.out > > > > diff --git a/tests/generic/391 b/tests/generic/391 > > new file mode 100755 > > index 0000000..121e01d > > --- /dev/null > > +++ b/tests/generic/391 > > @@ -0,0 +1,149 @@ > > +#! /bin/bash > > +# FS QA Test 391 > > +# > > +# Test inode's metadata after fsync or fdatasync calls. > > +# In the case of fsync, filesystem should recover all the inode metadata, while > > +# recovering i_blocks and i_size at least for fdatasync. > > +# > > +#----------------------------------------------------------------------- > > +# Copyright (c) 2016 Jaegeuk Kim. 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/punch > > + > > +# real QA test starts here > > +_supported_fs generic > > +_supported_os Linux > > + > > +rm -f $seqres.full > > +_require_scratch > > +_require_scratch_shutdown > > +_require_xfs_io_command "fpunch" > > + > > +_scratch_mkfs >/dev/null 2>&1 > > +_scratch_mount > > + > > +testfile=$SCRATCH_MNT/testfile > > + > > +f_stat() > > +{ > > + $XFS_IO_PROG -r -c "stat -v" $testfile >$tmp.$1 > > +} > > + > > +f_sync() > > +{ > > + $XFS_IO_PROG -c "$1" $testfile >/dev/null > > +} > > This is factoring overkill and... > > > + > > +# check inode metadata after shutdown > > +check_inode_metadata() > > +{ > > + # prepare syncing metadata > > + f_sync $1 > > + > > + # get inode metadata #1 > > + f_stat before > > + > > + # shutdown > > + src/godown $SCRATCH_MNT >> $seqres.full > > + _scratch_cycle_mount > > + > > + # get inode metadata #2 > > + f_stat after > > + > > + # compare #1 and #2 > > + diff $tmp.before $tmp.after >$tmp.diff > > + > > + # fdatasync only cares about i_size and i_blocks > > + if [ "$1" = "fdatasync" ]; then > > + cat $tmp.diff | grep stat.size > > + cat $tmp.diff | grep stat.blocks > > + else > > + cat $tmp.diff > > + fi > > .... forces you to do things like this. > > We have the stat binary for getting specific fields from files. We > use it all over the place for exactly this sort of thing. See, for > example, _defrag(), which also does this "stat before/stat > after/compare results" for specific metadata. > > i.e.: > > $ stat -c "blocks: %b size: %s" Makefile > blocks: 8 size: 3069 > $ > > check_inode_metadata() > { > sync_mode=$1 > > stat_opts="" > if [ $sync_mode = "fdatasync" ]; then > stat_opts='-c "blocks: %b size: %s"' > fi > > $XFS_IO_PROG -c "$sync_mode" $testfile | _filter_xfs_io > before=`stat $stat_opts $testfile` > ... > after=`stat $stat_opts $testfile` > > if [ $before != $after ]; then > echo "Before: $before" > echo "After: $after" > status=1; # this is a failure! > fi > > > + cat $tmp.before >> $seqres.full > > + cat $tmp.after >> $seqres.full > > + > > + rm $testfile > > +} > > + > > +# append XX KB with f{data}sync, followed by power-cut > > +test_i_size() > > +{ > > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > > + -c "fsync" \ > > + -c "pwrite 4M $2" \ > > + $testfile >/dev/null > > Make these all truncate to the final size first like this: > > $XFS_IO_PROG -f -c "truncate 4M" \ > -c "pwrite 0 4M" \ > -c "fsync" \ > -c "pwrite 4M $2" \ > $testfile | _filter_xfs_io > > check_inode_metadata $1 > > And that will make all the problems with XFS preallocation go away, > because the initial truncate to set the file size will turn off the > preallocation. > > > + $XFS_IO_PROG -f -c "pwrite 0 4M" \ > > + -c "fsync" \ > > + $testfile >/dev/null > > + sleep 1 > > + touch $testfile > > + check_inode_metadata $1 > > +} > > + > > +# punch XX KB with f{data}sync, followed by power-cut > > +test_punch() > > +{ > > + echo "==== fpunch $2 test with $1 ====" | tee -a $seqres.full > > + $XFS_IO_PROG -f -c "pwrite 0 4202496" \ > > + -c "fsync" \ > > + -c "fpunch 4194304 $2"\ > > + $testfile >/dev/null > > + check_inode_metadata $1 > > +} > > + > > +for i in fsync fdatasync > > +do > > + test_i_size $i 1024 > > + test_i_size $i 4096 > > + test_i_time $i > > + test_punch $i 1024 > > + test_punch $i 4096 > > +done > > + > > +rm -f $tmp.* > > No need for this here. > > > + > > +# success, all done > > +status=0 > > Set that at the start before we run the test so that if we detect a > failure we can set status=1 for exit to handle appropriately. Thank you so much for the review. Agreed to all, and let me send v5. Thanks, > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > -- > 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] 17+ messages in thread
* Re: [f2fs-dev] [PATCH v5] generic/391: check inode metadata on f{data}sync after power-cut 2016-11-19 1:57 ` [f2fs-dev] [PATCH v4] " Jaegeuk Kim 2016-11-20 21:19 ` Dave Chinner @ 2016-11-21 20:02 ` Jaegeuk Kim 1 sibling, 0 replies; 17+ messages in thread From: Jaegeuk Kim @ 2016-11-21 20:02 UTC (permalink / raw) To: fstests, linux-f2fs-devel; +Cc: linux-xfs This patch adds tests/generic/391 to test fsync and fdatasync with power-cuts. The rule to check is: 1) fsync should guarantee all the inode metadata after power-cut, 2) fdatasync should guarantee i_size and i_blocks at least after power-cut. Note that, in XFS, it allocates more blocks when doing writes, so it should close the file before fsync in order to get actual i_blocks after power-cut. Or, it needs to do truncate the file with a specific size to turn it off at the beginning. Suggested-by: Chao Yu <yuchao0@huawei.com> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- tests/generic/391 | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/391.out | 11 ++++ tests/generic/group | 1 + 3 files changed, 148 insertions(+) create mode 100755 tests/generic/391 create mode 100644 tests/generic/391.out diff --git a/tests/generic/391 b/tests/generic/391 new file mode 100755 index 0000000..fd44a34 --- /dev/null +++ b/tests/generic/391 @@ -0,0 +1,136 @@ +#! /bin/bash +# FS QA Test 391 +# +# Test inode's metadata after fsync or fdatasync calls. +# In the case of fsync, filesystem should recover all the inode metadata, while +# recovering i_blocks and i_size at least for fdatasync. +# +#----------------------------------------------------------------------- +# Copyright (c) 2016 Jaegeuk Kim. 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=0 # failure will be detected in runtime! +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/punch + +# real QA test starts here +_supported_fs generic +_supported_os Linux + +rm -f $seqres.full +_require_scratch +_require_scratch_shutdown +_require_xfs_io_command "fpunch" + +_scratch_mkfs >/dev/null 2>&1 +_scratch_mount + +testfile=$SCRATCH_MNT/testfile + +# check inode metadata after shutdown +check_inode_metadata() +{ + sync_mode=$1 + + # fsync or fdatasync + if [ $sync_mode = "fsync" ]; then + stat_opt='-c "b: %b s: %s a: %x m: %y c: %z"' + else + stat_opt='-c "b: %b s: %s"' + fi + + before=`stat "$stat_opt" $testfile` + + $XFS_IO_PROG -c "$sync_mode" $testfile | _filter_xfs_io + src/godown $SCRATCH_MNT | tee -a $seqres.full + _scratch_cycle_mount + + after=`stat "$stat_opt" $testfile` + + if [ "$before" != "$after" ]; then + echo "Before: $before" + echo "After : $after" + status=1; # this is a failure! + fi + echo "Before: $before" >> $seqres.full + echo "After : $after" >> $seqres.full + rm $testfile +} + +# append XX KB with f{data}sync, followed by power-cut +test_i_size() +{ + echo "==== i_size $2 test with $1 ====" | tee -a $seqres.full + $XFS_IO_PROG -f -c "truncate 4M" \ + -c "pwrite 0 4M" \ + -c "fsync" \ + -c "pwrite 4M $2" \ + $testfile >/dev/null + check_inode_metadata $1 +} + +# update times with f{data}sync, followed by power-cut +test_i_time() +{ + echo "==== i_time test with $1 ====" | tee -a $seqres.full + $XFS_IO_PROG -f -c "truncate 4M" \ + -c "pwrite 0 4M" \ + -c "fsync" \ + $testfile >/dev/null + sleep 1 + touch $testfile + check_inode_metadata $1 +} + +# punch XX KB with f{data}sync, followed by power-cut +test_punch() +{ + echo "==== fpunch $2 test with $1 ====" | tee -a $seqres.full + $XFS_IO_PROG -f -c "truncate 4202496" \ + -c "pwrite 0 4202496" \ + -c "fsync" \ + -c "fpunch 4194304 $2"\ + $testfile >/dev/null + check_inode_metadata $1 +} + +for i in fsync fdatasync +do + test_i_size $i 1024 + test_i_size $i 4096 + test_i_time $i + test_punch $i 1024 + test_punch $i 4096 +done + +exit diff --git a/tests/generic/391.out b/tests/generic/391.out new file mode 100644 index 0000000..7c66776 --- /dev/null +++ b/tests/generic/391.out @@ -0,0 +1,11 @@ +QA output created by 391 +==== i_size 1024 test with fsync ==== +==== i_size 4096 test with fsync ==== +==== i_time test with fsync ==== +==== fpunch 1024 test with fsync ==== +==== fpunch 4096 test with fsync ==== +==== i_size 1024 test with fdatasync ==== +==== i_size 4096 test with fdatasync ==== +==== i_time test with fdatasync ==== +==== fpunch 1024 test with fdatasync ==== +==== fpunch 4096 test with fdatasync ==== diff --git a/tests/generic/group b/tests/generic/group index 08007d7..9de3415 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -392,3 +392,4 @@ 387 auto clone 388 auto log metadata 389 auto quick acl +391 auto quick metadata -- 2.8.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-11-21 20:02 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20161117032753.69315-1-jaegeuk@kernel.org>
2016-11-17 8:35 ` [PATCH] generic/391: check inode metadata on f{data}sync after power-cut Eryu Guan
2016-11-17 12:56 ` Brian Foster
2016-11-17 14:00 ` Eryu Guan
2016-11-17 16:32 ` Brian Foster
2016-11-17 16:51 ` Eryu Guan
2016-11-17 19:17 ` Jaegeuk Kim
2016-11-17 18:31 ` Jaegeuk Kim
2016-11-17 19:20 ` [PATCH v2] " Jaegeuk Kim
2016-11-18 6:39 ` Eryu Guan
2016-11-18 19:44 ` Jaegeuk Kim
2016-11-19 0:42 ` Brian Foster
2016-11-19 1:56 ` Jaegeuk Kim
2016-11-18 19:45 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim
2016-11-19 1:57 ` [f2fs-dev] [PATCH v4] " Jaegeuk Kim
2016-11-20 21:19 ` Dave Chinner
2016-11-21 20:00 ` Jaegeuk Kim
2016-11-21 20:02 ` [f2fs-dev] [PATCH v5] " Jaegeuk Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).