Re: [PATCH v2] generic: test read around EOF

Zorro Lang
2017-07-27T03:46:14Z

On Wed, Jul 26, 2017 at 10:48:43AM -0400, Brian Foster wrote:
> On Wed, Jul 26, 2017 at 06:48:04PM +0800, Zorro Lang wrote:
> > As posix standard, if the file offset is at or past the end of file,
> > no bytes are read, and read() returns zero. But there was a bug,
> > when DIO read offset is just past the EOF a little, but in the same
> > block with EOF, read returns different negative values.
> >
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > ---
> >
> > Hi,
> >
> > V2 did below changes:
> > 1) move falloc, resvsp, allocsp related test to be special test,
> > no standard output
> > 2) pick up pread related test into read_test() function
> >
>
> I think Eric eventually narrowed down this problem to something
> that could be reproduced by the following:
>
> xfs_io -fc "pwrite 0 1024" /mnt/file
> xfs_io -d -c "pread 1536 512" /mnt/file
>
> The problem only occurred when a block was allocated at the offset of
> the post-eof DIO read. I suppose it couldn't hurt to test buffered read
> along with DIO, as well as the other cases of an unwritten block or
> hole. But given all that, I'm still wondering if this test could be
> simplified quite a bit to something that effectively does this:
>
> BSIZE=4096
> SSIZE=512
> FILE=/mnt/file
>
> rm -f $FILE
> xfs_io -fc "pwrite 0 $SSIZE" $FILE
> xfs_io -d -c "pread $(($BSIZE - $SSIZE)) $SSIZE" $FILE || echo "fail"
> xfs_io -c "pread $(($BSIZE - $SSIZE)) $SSIZE" $FILE || echo "fail"
>
> ... and then maybe repeat the above stanza 2 more times using a
> "truncate" and "falloc" to create the file. I don't think the
> XFS-specific preallocation commands are necessary. They result in the
> same layout and we have other tests for those commands. Otherwise, this
> could be cleaned up a bit, perhaps detect the BSIZE==SSIZE case and
> _notrun, etc. Thoughts in general?

Thanks Brian, I just try to cover more test conditions

I. different read range around EOF
1) read from a range < EOF
2) read from a range contains EOF
3) read from a range > EOF, but in same block.
4) read from a range far far away > EOF

II. different read mode:
1) buffer read
2) DIO read
3) sync buffer read
4) sync DIO read

III. different extents
1) no extents before and after EOF
2) written extents before EOF
3) unwritten extents before and after EOF
4) unwritten extents before EOF

And "I.3 + II.2/4 + III.2" is the reproducer for "74cedf9b6c60
direct-io: Fix negative return from dio read beyond eof". Others
are only extend testing, they'll all pass, except hit some bugs.

I was wondering _notrun when BSIZE==SSIZE, but for the current
code, the case won't fail if BSIZE==SSIZE, so I keep it for it
no hurt.

Thanks,
Zorro

>
> Brian
>
> > Thanks,
> > Zorro
> >
> > tests/generic/450 | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > tests/generic/450.out | 50 +++++++++++++++
> > tests/generic/group | 1 +
> > 3 files changed, 222 insertions(+)
> > create mode 100755 tests/generic/450
> > create mode 100644 tests/generic/450.out
> >
> > diff --git a/tests/generic/450 b/tests/generic/450
> > new file mode 100755
> > index 00000000..5fbe7f0c
> > --- /dev/null
> > +++ b/tests/generic/450
> > @@ -0,0 +1,171 @@
> > +#! /bin/bash
> > +# FS QA Test 450
> > +#
> > +# Test read around EOF. If the file offset is at or past the end of file,
> > +# no bytes are read, and read() returns zero. But there was a bug, when
> > +# DIO read offset is just past the EOF a little, but in the same block
> > +# with EOF, read returns different negative values.
> > +#
> > +#-----------------------------------------------------------------------
> > +# Copyright (c) 2017 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.*
> > + rm -f $tfile
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs generic
> > +_supported_os Linux
> > +_require_test
> > +
> > +tfile=$TEST_DIR/testfile_${seq}
> > +ssize=`blockdev --getss $TEST_DEV`
> > +bsize=`_get_block_size $TEST_DIR`
> > +
> > +rm -f $tfile 2>/dev/null
> > +
> > +# check xfs_io pread result, especially for
> > +# Param1: expected pread return
> > +# Param2: expected pread count
> > +# Param3: expected pread offset
> > +#
> > +# If any of above values are not as expected, the output keeps
> > +# using the real value
> > +check_xfs_io_read()
> > +{
> > + RETURN=$1
> > + COUNT=$2
> > + OFFSET=$3
> > +
> > + $AWK_PROG -v ret="$RETURN" -v cnt="$COUNT" -v off="$OFFSET" '
> > + /read/{
> > + split($2, bytes, "/")
> > +
> > + retval=bytes[1]
> > + count=bytes[2]
> > + offset=$NF
> > +
> > + if(retval == ret) retval="X"
> > + if(count == cnt) count="XX"
> > + if(offset == off) offset="XXX"
> > +
> > + printf("read [%s/%s %s]\n", retval, count, offset)
> > +
> > + next
> > + }
> > + '
> > +}
> > +
> > +
> > +# +-----------------------------------------+
> > +# | block | block | block |
> > +# +------------------------------------------
> > +# | sect | sect | sect | sect | sect | sect |
> > +# |
> > +# EOF
> > +# |<----------- move EOF --------->| xxxxxx |
> > +# [pread1]
> > +# [ pread2 ]
> > +# [pread3] ... [pread4]
> > +#
> > +# Run below steps with differnt $operation and $openflag
> > +#
> > +# 1) use $operation (alloc/reserve/truncate ...) to move EOF
> > +# 2) read (pread1) the first sector doesn't contain EOF
> > +# 3) read (pread2) the 2nd and 3rd blocks contain EOF
> > +# 4) read (pread3) the last sector which just out of EOF
> > +# 5) read (pread4) at far far away from EOF
> > +#
> > +# NOTE: This case generally test on sector size < block size, but equal is fine
> > +#
> > +asize=$((bsize * 3))
> > +tsize=$((asize - ssize - 100))
> > +
> > +read_test()
> > +{
> > + $XFS_IO_PROG $oflag -c "pread 0 $ssize" $tfile | \
> > + check_xfs_io_read "$ssize" "$ssize" "0"
> > +
> > + $XFS_IO_PROG $oflag -c "pread $bsize $((bsize * 2))" $tfile | \
> > + check_xfs_io_read "$((tsize - bsize))" "$((bsize * 2))" "$bsize"
> > +
> > + $XFS_IO_PROG $oflag -c "pread $((bsize * 3 - ssize)) $ssize" $tfile | \
> > + check_xfs_io_read "0" "$ssize" "$((bsize * 3 - ssize))"
> > +
> > + $XFS_IO_PROG $oflag -c "pread $((bsize * 100)) $ssize" $tfile | \
> > + check_xfs_io_read "0" "$ssize" "$((bsize * 100))"
> > +}
> > +
> > +# Generic test
> > +for oflag in "" "-d" "-s" "-sd"; do
> > + for oper in "pwrite 0 ${tsize}" "truncate ${tsize}"; do
> > + echo "## Test ${oper%% *} and read with open ${oflag} ##" | \
> > + tee -a $seqres.full
> > +
> > + $XFS_IO_PROG -ft -c "$oper" $tfile >>$seqres.full
> > +
> > + read_test
> > +
> > + echo | tee -a $seqres.full
> > + done
> > +done
> > +
> > +# XFS test
> > +echo "## Extra testing, no output is expected below ##"
> > +for oflag in "" "-d" "-s" "-sd"; do
> > + for oper in "falloc 0 ${tsize}" \
> > + "resvsp 0 ${asize}" \
> > + "allocsp ${tsize} 0"; do
> > + echo "## Test ${oper%% *} and read with open ${oflag} ##" >> $seqres.full
> > +
> > + # Drop all output, for some filesystems don't support these
> > + # test operations
> > + $XFS_IO_PROG -ft -c "truncate ${tsize}" -c "$oper" $tfile >>$seqres.full 2>&1
> > +
> > + # For keeping golden image consistency in different filesystems,
> > + # drop the output, but not error output
> > + read_test >>$seqres.full
> > +
> > + echo >> $seqres.full
> > + done
> > +done
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/generic/450.out b/tests/generic/450.out
> > new file mode 100644
> > index 00000000..ad2570e8
> > --- /dev/null
> > +++ b/tests/generic/450.out
> > @@ -0,0 +1,50 @@
> > +QA output created by 450
> > +## Test pwrite and read with open ##
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +
> > +## Test truncate and read with open ##
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +
> > +## Test pwrite and read with open -d ##
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +
> > +## Test truncate and read with open -d ##
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +
> > +## Test pwrite and read with open -s ##
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +
> > +## Test truncate and read with open -s ##
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +
> > +## Test pwrite and read with open -sd ##
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +
> > +## Test truncate and read with open -sd ##
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +read [X/XX XXX]
> > +
> > +## Extra testing, no output is expected below ##
> > diff --git a/tests/generic/group b/tests/generic/group
> > index a04cc900..0b29be0a 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -452,3 +452,4 @@
> > 447 auto quick clone
> > 448 auto quick rw
> > 449 auto quick acl enospc
> > +450 auto quick rw
> > --
> > 2.13.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