----- 原始邮件 -----
> 发件人: "Eryu Guan" <eguan@xxxxxxxxxx>
> 收件人: "Zorro Lang" <zlang@xxxxxxxxxx>
> 抄送: fstests@xxxxxxxxxxxxxxx
> 发送时间: 星期一, 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@xxxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
