[PATCH 1/2] common/rc: modify _require_xfs_io_command function

[PATCH 1/2] common/rc: modify _require_xfs_io_command function

[Zorro Lang]

1. This function try to run $XFS_IO_PROG -c "$command help", that's
wrong. It should be "help $command".

2. The $param can't be used for all command's options, for example
"help pwrite" include:

 -Z N -- zeed the random number generator (used when writing randomly)
         (heh, zorry, the -s/-S arguments were already in use in pwrite)

We should make param="-Z N", not only "-Z". After this patch, we can
run this function as:

  _require_xfs_io_command pwrite -Z N

Signed-off-by: Zorro Lang <zlang@redhat.com>
---
 common/rc | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index 51092a0..3b87d7e 100644
--- a/common/rc
+++ b/common/rc
@@ -1874,7 +1874,8 @@ _require_xfs_io_command()
 		exit 1
 	fi
 	command=$1
-	param=$2
+	shift
+	param="$*"
 
 	testfile=$TEST_DIR/$$.xfs_io
 	case $command in
@@ -1896,7 +1897,7 @@ _require_xfs_io_command()
 			_notrun "xfs_io $command support is missing"
 		;;
 	*)
-		testio=`$XFS_IO_PROG -c "$command help" 2>&1`
+		testio=`$XFS_IO_PROG -c "help $command" 2>&1`
 	esac
 
 	rm -f $testfile 2>&1 > /dev/null
-- 
2.5.5

[PATCH 2/2] generic/352: new case test mmaped data when blocksize less than pagesize

[Zorro Lang]

This case is a regression test for linux commit 90a80202:

  vfs: fix data corruption when blocksize < pagesize for mmaped data

  ->page_mkwrite() is used by filesystems to allocate blocks under a page
  which is becoming writeably mmapped in some process' address space. This
  allows a filesystem to return a page fault if there is not enough space
  available, user exceeds quota or similar problem happens, rather than
  silently discarding data later when writepage is called.

  However VFS fails to call ->page_mkwrite() in all the cases where
  filesystems need it when blocksize < pagesize. For example when
  blocksize = 1024, pagesize = 4096 the following is problematic:
    ftruncate(fd, 0);
    pwrite(fd, buf, 1024, 0);
    map = mmap(NULL, 1024, PROT_WRITE, MAP_SHARED, fd, 0);
    map[0] = 'a';       ----> page_mkwrite() for index 0 is called
    ftruncate(fd, 10000); /* or even pwrite(fd, buf, 1, 10000) */
    mremap(map, 1024, 10000, 0);
    map[4095] = 'a';    ----> no page_mkwrite() called

  At the moment ->page_mkwrite() is called, filesystem can allocate only
  one block for the page because i_size == 1024. Otherwise it would create
  blocks beyond i_size which is generally undesirable. But later at
  ->writepage() time, we also need to store data at offset 4095 but we
  don't have block allocated for it.

  This patch introduces a helper function filesystems can use to have
  ->page_mkwrite() called at all the necessary moments.

Signed-off-by: Zorro Lang <zlang@redhat.com>
---

Hi,

After xfsprogs merged commit:
  e0a8808 xfs_io: allow mmap command to reserve some free space

We can reserve(not 100% sure, but pretty high rate) some memory for mremap
extend the map space. Due to it, I can use xfs_io to implement this
case.

There're 2 problems when I write this case:
1. The reserved memory space can't be kept 100%, especially when
system run heavy memory load. But I think it hard to resolve.

2. When xfs_io process be killed by SIGBUS, I can't catch the "Bus error"
output(it not on stdout/stderr). But it will printed into .out file. So
I use a pipe stop it(Generally, that _filter_xfs_io after the pipe is
useless. I just need a pipe.). But I'm not sure if it's a good idea.
Anyone have better idea?

(BTW, this case rely on my last patch about _require_xfs_io_command.)

Thanks,
Zorro

 tests/generic/352     | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/352.out |   3 ++
 tests/generic/group   |   1 +
 3 files changed, 113 insertions(+)
 create mode 100755 tests/generic/352
 create mode 100644 tests/generic/352.out

diff --git a/tests/generic/352 b/tests/generic/352
new file mode 100755
index 0000000..5e69127
--- /dev/null
+++ b/tests/generic/352
@@ -0,0 +1,109 @@
+#! /bin/bash
+# FS QA Test 352
+#
+# Regression test for linux commit 90a80202, data corruption when
+# blocksize < pagesize for mmaped data.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2016 Red Hat Inc.  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
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# support fs which can make block size < page size
+_supported_fs xfs ext2 ext3 ext4 ext4dev udf
+_supported_os Linux
+_require_scratch
+_require_xfs_io_command mmap "-s <size>"
+_require_xfs_io_command mremap
+_require_xfs_io_command truncate
+_require_xfs_io_command pwrite
+_require_xfs_io_command mwrite
+
+sector_size=`blockdev --getss $SCRATCH_DEV`
+page_size=$(get_page_size)
+block_size=$((page_size/2))
+
+if [ $sector_size -gt $block_size ];then
+    _notrun "need sector size < page size"
+fi
+
+unset MKFS_OPTIONS
+# For save time, 512MiB is enough to reproduce bug
+_scratch_mkfs_sized 536870912 $block_size >$seqres.full 2>&1
+
+# avoid the interference of XFS preallocation
+if [ "$FSTYP" = xfs ];then
+    MOUNT_OPTIONS="$MOUNT_OPTIONS -o allocsize=$page_size"
+fi
+_scratch_mount
+
+# take one block size space
+dd if=/dev/zero of=$SCRATCH_MNT/testfile bs=$block_size count=1 >$seqres.full 2>&1
+# fill all free space
+dd if=/dev/zero of=$SCRATCH_MNT/full bs=$block_size >$seqres.full 2>&1
+sync
+
+# truncate 0
+# pwrite -S 0x61 0 $block_size
+# mmap -rw -s $((page_size * 2)) 0 $block_size
+# mwrite -S 0x61 0 1  --> page_mkwrite() for index 0 is called
+# truncate $((page_size * 2))
+# mremap $((page_size * 2))
+# mwrite -S 0x61 $((page_size - 1)) 1  --> [bug] no page_mkwrite() called
+#
+# If there's a bug, the last step will be killed by SIGBUS, and it won't
+# write a "0x61" on the disk.
+#
+# Note: mremap maybe failed when memory load is heavy. 
+$XFS_IO_PROG -f \
+	     -c "truncate 0" \
+	     -c "pwrite -S 0x61 0 $block_size" \
+	     -c "mmap -rw -s $((page_size * 2)) 0 $block_size" \
+	     -c "mwrite -S 0x61 0 1" \
+	     -c "truncate $((page_size * 2))" \
+	     -c "mremap $((page_size * 2))" \
+	     -c "mwrite -S 0x61 $((page_size - 1)) 1" \
+	     $SCRATCH_MNT/testfile | _filter_xfs_io
+
+# we will see 0x61 written by "mwrite -S 0x61 0 1"
+# we shouldn't see one more 0x61 by the last mwrite.
+hexdump -e '1/1 "%02x "' $SCRATCH_MNT/testfile
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/352.out b/tests/generic/352.out
new file mode 100644
index 0000000..8be005d
--- /dev/null
+++ b/tests/generic/352.out
@@ -0,0 +1,3 @@
+QA output created by 352
+61 *
+00 *
diff --git a/tests/generic/group b/tests/generic/group
index 36fb759..7a72f6b 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -354,3 +354,4 @@
 349 blockdev quick rw
 350 blockdev quick rw
 351 blockdev quick rw
+352 auto quick
-- 
2.5.5

Re: [PATCH 2/2] generic/352: new case test mmaped data when blocksize less than pagesize

[Eryu Guan]

On Wed, May 18, 2016 at 12:36:21AM +0800, Zorro Lang wrote:
> This case is a regression test for linux commit 90a80202:
> 
>   vfs: fix data corruption when blocksize < pagesize for mmaped data
> 
>   ->page_mkwrite() is used by filesystems to allocate blocks under a page
>   which is becoming writeably mmapped in some process' address space. This
>   allows a filesystem to return a page fault if there is not enough space
>   available, user exceeds quota or similar problem happens, rather than
>   silently discarding data later when writepage is called.
> 
>   However VFS fails to call ->page_mkwrite() in all the cases where
>   filesystems need it when blocksize < pagesize. For example when
>   blocksize = 1024, pagesize = 4096 the following is problematic:
>     ftruncate(fd, 0);
>     pwrite(fd, buf, 1024, 0);
>     map = mmap(NULL, 1024, PROT_WRITE, MAP_SHARED, fd, 0);
>     map[0] = 'a';       ----> page_mkwrite() for index 0 is called
>     ftruncate(fd, 10000); /* or even pwrite(fd, buf, 1, 10000) */
>     mremap(map, 1024, 10000, 0);
>     map[4095] = 'a';    ----> no page_mkwrite() called
> 
>   At the moment ->page_mkwrite() is called, filesystem can allocate only
>   one block for the page because i_size == 1024. Otherwise it would create
>   blocks beyond i_size which is generally undesirable. But later at
>   ->writepage() time, we also need to store data at offset 4095 but we
>   don't have block allocated for it.
> 
>   This patch introduces a helper function filesystems can use to have
>   ->page_mkwrite() called at all the necessary moments.
> 
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
> 
> Hi,
> 
> After xfsprogs merged commit:
>   e0a8808 xfs_io: allow mmap command to reserve some free space
> 
> We can reserve(not 100% sure, but pretty high rate) some memory for mremap
> extend the map space. Due to it, I can use xfs_io to implement this
> case.
> 
> There're 2 problems when I write this case:
> 1. The reserved memory space can't be kept 100%, especially when
> system run heavy memory load. But I think it hard to resolve.
> 
> 2. When xfs_io process be killed by SIGBUS, I can't catch the "Bus error"
> output(it not on stdout/stderr). But it will printed into .out file. So
> I use a pipe stop it(Generally, that _filter_xfs_io after the pipe is
> useless. I just need a pipe.). But I'm not sure if it's a good idea.
> Anyone have better idea?
> 
> (BTW, this case rely on my last patch about _require_xfs_io_command.)
> 
> Thanks,
> Zorro
> 
>  tests/generic/352     | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/352.out |   3 ++
>  tests/generic/group   |   1 +
>  3 files changed, 113 insertions(+)
>  create mode 100755 tests/generic/352
>  create mode 100644 tests/generic/352.out
> 
> diff --git a/tests/generic/352 b/tests/generic/352
> new file mode 100755
> index 0000000..5e69127
> --- /dev/null
> +++ b/tests/generic/352
> @@ -0,0 +1,109 @@
> +#! /bin/bash
> +# FS QA Test 352
> +#
> +# Regression test for linux commit 90a80202, data corruption when
> +# blocksize < pagesize for mmaped data.
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2016 Red Hat Inc.  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
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# support fs which can make block size < page size
> +_supported_fs xfs ext2 ext3 ext4 ext4dev udf

No need to filter out other fs, at least we can get some extra test
coverage on other filesystems, even if they cannot reproduce this bug

And all tests in generic dir should have "_supported_fs generic",
otherwise you should put it to "shared" dir or fs-specific dir.

> +_supported_os Linux
> +_require_scratch
> +_require_xfs_io_command mmap "-s <size>"
> +_require_xfs_io_command mremap
> +_require_xfs_io_command truncate
> +_require_xfs_io_command pwrite
> +_require_xfs_io_command mwrite
> +
> +sector_size=`blockdev --getss $SCRATCH_DEV`
> +page_size=$(get_page_size)
> +block_size=$((page_size/2))
> +
> +if [ $sector_size -gt $block_size ];then
> +    _notrun "need sector size < page size"
> +fi
> +
> +unset MKFS_OPTIONS
> +# For save time, 512MiB is enough to reproduce bug
> +_scratch_mkfs_sized 536870912 $block_size >$seqres.full 2>&1
> +
> +# avoid the interference of XFS preallocation
> +if [ "$FSTYP" = xfs ];then
> +    MOUNT_OPTIONS="$MOUNT_OPTIONS -o allocsize=$page_size"
> +fi

All these blocksize vs pagesize checks, mkfs with specific pagesize,
adding allocsize mount option are not necessary. We want more test
coverage even if the original bug can not be reproduced in such
configurations, it's up to the test runner to specify the blocksize,
mount options etc.

> +_scratch_mount
> +
> +# take one block size space
> +dd if=/dev/zero of=$SCRATCH_MNT/testfile bs=$block_size count=1 >$seqres.full 2>&1
> +# fill all free space
> +dd if=/dev/zero of=$SCRATCH_MNT/full bs=$block_size >$seqres.full 2>&1

We prefer $XFS_IO_PROG over dd in fstests. And please append (>>) the log to
$seqres.full not overwrite (>) the log.

I'd recommend add an explicit "rm -f $seqres.full" and use ">>" to
append the logs, so you don't have to worry when to use ">" and when to
use ">>".

> +sync
> +
> +# truncate 0
> +# pwrite -S 0x61 0 $block_size
> +# mmap -rw -s $((page_size * 2)) 0 $block_size
> +# mwrite -S 0x61 0 1  --> page_mkwrite() for index 0 is called
> +# truncate $((page_size * 2))
> +# mremap $((page_size * 2))
> +# mwrite -S 0x61 $((page_size - 1)) 1  --> [bug] no page_mkwrite() called
> +#
> +# If there's a bug, the last step will be killed by SIGBUS, and it won't
> +# write a "0x61" on the disk.

Please write a different char for the test, e.g. 0x62, so we can easily
know that it's the test char.

> +#
> +# Note: mremap maybe failed when memory load is heavy. 
> +$XFS_IO_PROG -f \
> +	     -c "truncate 0" \
> +	     -c "pwrite -S 0x61 0 $block_size" \
> +	     -c "mmap -rw -s $((page_size * 2)) 0 $block_size" \
> +	     -c "mwrite -S 0x61 0 1" \
> +	     -c "truncate $((page_size * 2))" \
> +	     -c "mremap $((page_size * 2))" \
> +	     -c "mwrite -S 0x61 $((page_size - 1)) 1" \
> +	     $SCRATCH_MNT/testfile | _filter_xfs_io
> +
> +# we will see 0x61 written by "mwrite -S 0x61 0 1"
> +# we shouldn't see one more 0x61 by the last mwrite.
> +hexdump -e '1/1 "%02x "' $SCRATCH_MNT/testfile

I personally perfer od here :)

Thanks,
Eryu

> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/352.out b/tests/generic/352.out
> new file mode 100644
> index 0000000..8be005d
> --- /dev/null
> +++ b/tests/generic/352.out
> @@ -0,0 +1,3 @@
> +QA output created by 352
> +61 *
> +00 *
> diff --git a/tests/generic/group b/tests/generic/group
> index 36fb759..7a72f6b 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -354,3 +354,4 @@
>  349 blockdev quick rw
>  350 blockdev quick rw
>  351 blockdev quick rw
> +352 auto quick
> -- 
> 2.5.5
> 
> --
> 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 2/2] generic/352: new case test mmaped data when blocksize less than pagesize

[Zirong Lang]

----- 原始邮件 -----
> 发件人: "Eryu Guan" <eguan@redhat.com>
> 收件人: "Zorro Lang" <zlang@redhat.com>
> 抄送: fstests@vger.kernel.org
> 发送时间: 星期一, 2016年 5 月 23日 下午 5:07:50
> 主题: Re: [PATCH 2/2] generic/352: new case test mmaped data when blocksize less than pagesize
> 
> On Wed, May 18, 2016 at 12:36:21AM +0800, Zorro Lang wrote:
> > This case is a regression test for linux commit 90a80202:
> > 
> >   vfs: fix data corruption when blocksize < pagesize for mmaped data
> > 
> >   ->page_mkwrite() is used by filesystems to allocate blocks under a page
> >   which is becoming writeably mmapped in some process' address space. This
> >   allows a filesystem to return a page fault if there is not enough space
> >   available, user exceeds quota or similar problem happens, rather than
> >   silently discarding data later when writepage is called.
> > 
> >   However VFS fails to call ->page_mkwrite() in all the cases where
> >   filesystems need it when blocksize < pagesize. For example when
> >   blocksize = 1024, pagesize = 4096 the following is problematic:
> >     ftruncate(fd, 0);
> >     pwrite(fd, buf, 1024, 0);
> >     map = mmap(NULL, 1024, PROT_WRITE, MAP_SHARED, fd, 0);
> >     map[0] = 'a';       ----> page_mkwrite() for index 0 is called
> >     ftruncate(fd, 10000); /* or even pwrite(fd, buf, 1, 10000) */
> >     mremap(map, 1024, 10000, 0);
> >     map[4095] = 'a';    ----> no page_mkwrite() called
> > 
> >   At the moment ->page_mkwrite() is called, filesystem can allocate only
> >   one block for the page because i_size == 1024. Otherwise it would create
> >   blocks beyond i_size which is generally undesirable. But later at
> >   ->writepage() time, we also need to store data at offset 4095 but we
> >   don't have block allocated for it.
> > 
> >   This patch introduces a helper function filesystems can use to have
> >   ->page_mkwrite() called at all the necessary moments.
> > 
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > ---
> > 
> > Hi,
> > 
> > After xfsprogs merged commit:
> >   e0a8808 xfs_io: allow mmap command to reserve some free space
> > 
> > We can reserve(not 100% sure, but pretty high rate) some memory for mremap
> > extend the map space. Due to it, I can use xfs_io to implement this
> > case.
> > 
> > There're 2 problems when I write this case:
> > 1. The reserved memory space can't be kept 100%, especially when
> > system run heavy memory load. But I think it hard to resolve.
> > 
> > 2. When xfs_io process be killed by SIGBUS, I can't catch the "Bus error"
> > output(it not on stdout/stderr). But it will printed into .out file. So
> > I use a pipe stop it(Generally, that _filter_xfs_io after the pipe is
> > useless. I just need a pipe.). But I'm not sure if it's a good idea.
> > Anyone have better idea?
> > 
> > (BTW, this case rely on my last patch about _require_xfs_io_command.)
> > 
> > Thanks,
> > Zorro
> > 
> >  tests/generic/352     | 109
> >  ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/352.out |   3 ++
> >  tests/generic/group   |   1 +
> >  3 files changed, 113 insertions(+)
> >  create mode 100755 tests/generic/352
> >  create mode 100644 tests/generic/352.out
> > 
> > diff --git a/tests/generic/352 b/tests/generic/352
> > new file mode 100755
> > index 0000000..5e69127
> > --- /dev/null
> > +++ b/tests/generic/352
> > @@ -0,0 +1,109 @@
> > +#! /bin/bash
> > +# FS QA Test 352
> > +#
> > +# Regression test for linux commit 90a80202, data corruption when
> > +# blocksize < pagesize for mmaped data.
> > +#
> > +#-----------------------------------------------------------------------
> > +# Copyright (c) 2016 Red Hat Inc.  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
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +# support fs which can make block size < page size
> > +_supported_fs xfs ext2 ext3 ext4 ext4dev udf
> 
> No need to filter out other fs, at least we can get some extra test
> coverage on other filesystems, even if they cannot reproduce this bug
> 
> And all tests in generic dir should have "_supported_fs generic",
> otherwise you should put it to "shared" dir or fs-specific dir.

Because the bug need block size < page size, so I need fs support
mkfs block size.

But this case can run on everyone fs, so if don't care the bug reproduce,
it can support all fs.

> 
> > +_supported_os Linux
> > +_require_scratch
> > +_require_xfs_io_command mmap "-s <size>"
> > +_require_xfs_io_command mremap
> > +_require_xfs_io_command truncate
> > +_require_xfs_io_command pwrite
> > +_require_xfs_io_command mwrite
> > +
> > +sector_size=`blockdev --getss $SCRATCH_DEV`
> > +page_size=$(get_page_size)
> > +block_size=$((page_size/2))
> > +
> > +if [ $sector_size -gt $block_size ];then
> > +    _notrun "need sector size < page size"
> > +fi
> > +
> > +unset MKFS_OPTIONS
> > +# For save time, 512MiB is enough to reproduce bug
> > +_scratch_mkfs_sized 536870912 $block_size >$seqres.full 2>&1
> > +
> > +# avoid the interference of XFS preallocation
> > +if [ "$FSTYP" = xfs ];then
> > +    MOUNT_OPTIONS="$MOUNT_OPTIONS -o allocsize=$page_size"
> > +fi
> 
> All these blocksize vs pagesize checks, mkfs with specific pagesize,
> adding allocsize mount option are not necessary. We want more test
> coverage even if the original bug can not be reproduced in such
> configurations, it's up to the test runner to specify the blocksize,
> mount options etc.

OK, as I said above, if we don't think about bug reproduce 100%, I can
let the tester decide to use what block size in MOUNT_OPTIONS.

> 
> > +_scratch_mount
> > +
> > +# take one block size space
> > +dd if=/dev/zero of=$SCRATCH_MNT/testfile bs=$block_size count=1
> > >$seqres.full 2>&1
> > +# fill all free space
> > +dd if=/dev/zero of=$SCRATCH_MNT/full bs=$block_size >$seqres.full 2>&1
> 
> We prefer $XFS_IO_PROG over dd in fstests. And please append (>>) the log to
> $seqres.full not overwrite (>) the log.
> 
> I'd recommend add an explicit "rm -f $seqres.full" and use ">>" to
> append the logs, so you don't have to worry when to use ">" and when to
> use ">>".

OK, I'll change dd to xfs_io pwrite.

> 
> > +sync
> > +
> > +# truncate 0
> > +# pwrite -S 0x61 0 $block_size
> > +# mmap -rw -s $((page_size * 2)) 0 $block_size
> > +# mwrite -S 0x61 0 1  --> page_mkwrite() for index 0 is called
> > +# truncate $((page_size * 2))
> > +# mremap $((page_size * 2))
> > +# mwrite -S 0x61 $((page_size - 1)) 1  --> [bug] no page_mkwrite() called
> > +#
> > +# If there's a bug, the last step will be killed by SIGBUS, and it won't
> > +# write a "0x61" on the disk.
> 
> Please write a different char for the test, e.g. 0x62, so we can easily
> know that it's the test char.

Sure, I can do that:)

> 
> > +#
> > +# Note: mremap maybe failed when memory load is heavy.
> > +$XFS_IO_PROG -f \
> > +	     -c "truncate 0" \
> > +	     -c "pwrite -S 0x61 0 $block_size" \
> > +	     -c "mmap -rw -s $((page_size * 2)) 0 $block_size" \
> > +	     -c "mwrite -S 0x61 0 1" \
> > +	     -c "truncate $((page_size * 2))" \
> > +	     -c "mremap $((page_size * 2))" \
> > +	     -c "mwrite -S 0x61 $((page_size - 1)) 1" \
> > +	     $SCRATCH_MNT/testfile | _filter_xfs_io
> > +
> > +# we will see 0x61 written by "mwrite -S 0x61 0 1"
> > +# we shouldn't see one more 0x61 by the last mwrite.
> > +hexdump -e '1/1 "%02x "' $SCRATCH_MNT/testfile
> 
> I personally perfer od here :)

OK, I can't sure which one is better. Let me try to change it to od
if fstests like it.

Thanks,
Zorro

> 
> Thanks,
> Eryu
> 
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/generic/352.out b/tests/generic/352.out
> > new file mode 100644
> > index 0000000..8be005d
> > --- /dev/null
> > +++ b/tests/generic/352.out
> > @@ -0,0 +1,3 @@
> > +QA output created by 352
> > +61 *
> > +00 *
> > diff --git a/tests/generic/group b/tests/generic/group
> > index 36fb759..7a72f6b 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -354,3 +354,4 @@
> >  349 blockdev quick rw
> >  350 blockdev quick rw
> >  351 blockdev quick rw
> > +352 auto quick
> > --
> > 2.5.5
> > 
> > --
> > 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 1/2] common/rc: modify _require_xfs_io_command function

[Dave Chinner]

On Wed, May 18, 2016 at 12:36:20AM +0800, Zorro Lang wrote:
> 1. This function try to run $XFS_IO_PROG -c "$command help", that's
> wrong. It should be "help $command".

I think there was more to it that than:

$ sudo xfs_io -c "pwrite help"
$ sudo xfs_io -c "help pwrite"
pwrite [-i infile [-d] [-s skip]] [-b bs] [-S seed] [-wW] [-FBR [-Z N]] [-V N] off len -- writes a number of bytes at a specified offset

 writes a range of bytes (in block size increments) from the given offset
[....]
$ sudo xfs_io -c "foo help"
command "foo" not found

i.e. that check simply tells us whether the command exists or not
with an error message that is captured and then tested. If we
actually run the help command, we got lots of output for commands
that exist and that may confuse the error string checking we then
do.

Yes, it's a bit of a hack, and we may not even need the "help"
parameter anymore, but ISTR older versions of xfs_io needed it to
parse the CLI successfully for all the different commands....

> 2. The $param can't be used for all command's options, for example
> "help pwrite" include:
> 
>  -Z N -- zeed the random number generator (used when writing randomly)
>          (heh, zorry, the -s/-S arguments were already in use in pwrite)

This bit is fine.

In general, you should put separate changes into separate patches.
If you find yourself iterating a list of things a patch fixes in the
description, then that's a good sign it should be split into
multiple patches.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] common/rc: modify _require_xfs_io_command function

[Zirong Lang]

----- 原始邮件 -----
> 发件人: "Dave Chinner" <david@fromorbit.com>
> 收件人: "Zorro Lang" <zlang@redhat.com>
> 抄送: fstests@vger.kernel.org
> 发送时间: 星期四, 2016年 5 月 19日 上午 7:55:31
> 主题: Re: [PATCH 1/2] common/rc: modify _require_xfs_io_command function
> 
> On Wed, May 18, 2016 at 12:36:20AM +0800, Zorro Lang wrote:
> > 1. This function try to run $XFS_IO_PROG -c "$command help", that's
> > wrong. It should be "help $command".
> 
> I think there was more to it that than:
> 
> $ sudo xfs_io -c "pwrite help"
> $ sudo xfs_io -c "help pwrite"
> pwrite [-i infile [-d] [-s skip]] [-b bs] [-S seed] [-wW] [-FBR [-Z N]] [-V
> N] off len -- writes a number of bytes at a specified offset
> 
>  writes a range of bytes (in block size increments) from the given offset
> [....]
> $ sudo xfs_io -c "foo help"
> command "foo" not found
> 
> i.e. that check simply tells us whether the command exists or not
> with an error message that is captured and then tested. If we
> actually run the help command, we got lots of output for commands
> that exist and that may confuse the error string checking we then
> do.
> 
> Yes, it's a bit of a hack, and we may not even need the "help"
> parameter anymore, but ISTR older versions of xfs_io needed it to
> parse the CLI successfully for all the different commands....

Hmm, that's a smart way. So I don't need to change that before someone
new xfsprogs break this "hack rule":)

> 
> > 2. The $param can't be used for all command's options, for example
> > "help pwrite" include:
> > 
> >  -Z N -- zeed the random number generator (used when writing randomly)
> >          (heh, zorry, the -s/-S arguments were already in use in pwrite)
> 
> This bit is fine.
> 
> In general, you should put separate changes into separate patches.
> If you find yourself iterating a list of things a patch fixes in the
> description, then that's a good sign it should be split into
> multiple patches.

Thanks for point that, I'll send a V2 patch only contain this change.

Thanks,
Zorro

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