[PATCH] generic/391: check inode metadata on f{data}sync after power-cut

[PATCH] generic/391: check inode metadata on f{data}sync after power-cut

[Jaegeuk Kim]

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
+
+# 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()
+{
+	src/godown $SCRATCH_MNT >> $seqres.full
+	$XFS_IO_PROG -r -c "stat -v" $1 >$tmp.before
+	_scratch_cycle_mount
+	$XFS_IO_PROG -r -c "stat -v" $1 >$tmp.after
+	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
+	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 ===="
+	$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

Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut

[Eryu Guan]

[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

Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut

[Brian Foster]

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

Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut

[Brian Foster]

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

------------------------------------------------------------------------------

Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut

[Eryu Guan]

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

Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut

[Eryu Guan]

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

------------------------------------------------------------------------------

Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut

[Brian Foster]

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

Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut

[Brian Foster]

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

------------------------------------------------------------------------------

Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut

[Eryu Guan]

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

Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut

[Eryu Guan]

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

------------------------------------------------------------------------------

Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut

[Jaegeuk Kim]

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,

Re: [PATCH] generic/391: check inode metadata on f{data}sync after power-cut

[Jaegeuk Kim]

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

Re: [PATCH v2] generic/391: check inode metadata on f{data}sync after power-cut

[Jaegeuk Kim]

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

Re: [PATCH v2] generic/391: check inode metadata on f{data}sync after power-cut

[Jaegeuk Kim]

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


------------------------------------------------------------------------------

Re: [PATCH v2] generic/391: check inode metadata on f{data}sync after power-cut

[Eryu Guan]

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

Re: [PATCH v2] generic/391: check inode metadata on f{data}sync after power-cut

[Jaegeuk Kim]

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

Re: [PATCH v2] generic/391: check inode metadata on f{data}sync after power-cut

[Brian Foster]

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

Re: [PATCH v2] generic/391: check inode metadata on f{data}sync after power-cut

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH v3] generic/391: check inode metadata on f{data}sync after power-cut

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH v4] generic/391: check inode metadata on f{data}sync after power-cut

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH v4] generic/391: check inode metadata on f{data}sync after power-cut

[Dave Chinner]

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

Re: [f2fs-dev] [PATCH v4] generic/391: check inode metadata on f{data}sync after power-cut

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH v5] generic/391: check inode metadata on f{data}sync after power-cut

[Jaegeuk Kim]

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