Re: [PATCH] generic: test race when checking i_size on direct i/o read

[Eryu Guan <eguan@redhat.com>]

On Mon, Oct 09, 2017 at 12:39:52PM -0400, Brian Foster wrote:
> On Mon, Sep 25, 2017 at 06:18:47PM +0800, Eryu Guan wrote:
> > From: Zheng Liu <wenqing.lz@taobao.com>
> > 
> > In this commit a new test case is added to test that i_size races
> > don't occur under dio reads/writes.  We add a program in /src dir,
> > which has a writer to issue some append dio writes.  Meanwhile it
> > has a reader in this test to do some dio reads.  As we expect,
> > reader should read nothing or data with 'a'. But it might read some
> > data with '0'.
> > 
> > The bug can be reproduced by this test case [1].
> > 
> > 1.  http://patchwork.ozlabs.org/patch/311761/
> > 
> > This ostensibly tests commit:
> > 9fe55eea7 Fix race when checking i_size on direct i/o read
> > 
> > Update by Eric Sandeen:
> > - update to recent xfstests
> > - update commit log
> > 
> > Update by Eryu Guan:
> > - add aio-dio support to the test and add 'aio' group
> > - add ability to test different alignments
> > - move test from src/ to src/aio-dio-regress/
> > - add .gitignore entry
> > - rebase against latest xfstests with various minor fixes & cleanups
> > - update commit log
> > 
> > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > Signed-off-by: Eryu Guan <eguan@redhat.com>
> > ---
> > This test fails on XFS, the following patch fixed the XFS failure
> > https://www.spinics.net/lists/linux-xfs/msg10865.html
> > 
> > btrfs also fails when io alignment is not 4k, perhaps a btrfs bug?
> > 
> > Eric's original post
> > http://www.spinics.net/lists/fstests/msg06978.html
> > 
> >  .gitignore                                         |   1 +
> >  .../aio-dio-append-write-read-race.c               | 214 +++++++++++++++++++++
> >  tests/generic/462                                  |  75 ++++++++
> >  tests/generic/462.out                              |   3 +
> >  tests/generic/group                                |   1 +
> >  5 files changed, 294 insertions(+)
> >  create mode 100644 src/aio-dio-regress/aio-dio-append-write-read-race.c
> >  create mode 100755 tests/generic/462
> >  create mode 100644 tests/generic/462.out
> > 
> ...
> > diff --git a/src/aio-dio-regress/aio-dio-append-write-read-race.c b/src/aio-dio-regress/aio-dio-append-write-read-race.c
> > new file mode 100644
> > index 000000000000..dce2ab743f11
> > --- /dev/null
> > +++ b/src/aio-dio-regress/aio-dio-append-write-read-race.c
> > @@ -0,0 +1,214 @@
> ...
> > +
> > +int main(int argc, char *argv[])
> > +{
> ...
> > +	for (i = 0; i < max_blocks; i++) {
> > +		wdata.offset = i * blksize;
> > +		if (use_aio)
> > +			ret = pthread_create(&tid, NULL, aio_writer, &wdata);
> > +		else
> > +			ret = pthread_create(&tid, NULL, dio_writer, &wdata);
> > +		if (ret) {
> > +			fprintf(stderr, "create thread failed: %s\n",
> > +				strerror(ret));
> > +			ret = 1;
> > +			goto err;
> > +		}
> > +
> > +		memset(rbuf, 'b', blksize);
> > +		do {
> > +			ret = pread(rfd, rbuf, blksize, i * blksize);
> > +			if (ret < 0)
> > +				perror("read file");
> > +		} while (ret <= 0);
> 
> Hmm, so IIUC the objective is to kick off a writer thread then issue
> reads in a loop to try and guarantee that the first valid read returns
> valid data. By kicking off the writer first, aren't we risking the test
> could be silently ineffective if the writer thread happens to run early
> enough to miss the conversion? I'm wondering if it might be more
> reliable to start the reader in a background thread first and signal a
> condition once it starts issuing reads. Then wait for the condition here
> and invoke the appropriate writer func directly.

Makes sense, I'll convert it to run reader in a thread and only start
writer after reader is ready. Thanks for the review!

Eryu

> 
> Brian
> 
> > +
> > +		ret = pthread_join(tid, NULL);
> > +		if (ret) {
> > +			fprintf(stderr, "pthread join failed: %s\n",
> > +				strerror(ret));
> > +			ret = 1;
> > +			goto err;
> > +		}
> > +
> > +		for (j = 0; j < blksize; j++) {
> > +			if (rbuf[j] != 'a') {
> > +				fprintf(stderr, "encounter an error: "
> > +					"block %d offset %d, content %x\n",
> > +					i, j, rbuf[j]);
> > +				ret = 1;
> > +				goto err;
> > +			}
> > +		}
> > +	}
> > +
> > +err:
> > +	if (rfd)
> > +		close(rfd);
> > +	if (wfd)
> > +		close(wfd);
> > +	if (rbuf)
> > +		free(rbuf);
> > +	if (wbuf)
> > +		free(wbuf);
> > +
> > +	exit(ret);
> > +}
> > diff --git a/tests/generic/462 b/tests/generic/462
> > new file mode 100755
> > index 000000000000..7abadcb11921
> > --- /dev/null
> > +++ b/tests/generic/462
> > @@ -0,0 +1,75 @@
> > +#! /bin/bash
> > +# FS QA Test No. 462
> > +#
> > +# Test i_size is updated properly under dio read/write
> > +#
> > +#-----------------------------------------------------------------------
> > +# Copyright (c) 2013 Alibaba Group.  All Rights Reserved.
> > +# 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.* $testfile.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +_supported_fs generic
> > +_supported_os Linux
> > +
> > +_require_aiodio aio-dio-append-write-read-race
> > +_require_test_program "feature"
> > +
> > +testfile=$TEST_DIR/$seq.$$
> > +min_dio_align=`_min_dio_alignment $TEST_DEV`
> > +page_size=`$here/src/feature -s`
> > +
> > +rm -f $seqres.full
> > +
> > +echo "non-aio dio test"
> > +align=$min_dio_align
> > +while [ $align -le $page_size ]; do
> > +	echo "$AIO_TEST -a $align -d $testfile.$align" >> $seqres.full
> > +	$AIO_TEST -a $align -d $testfile.$align 2>&1 | tee -a $seqres.full
> > +	align=$((align * 2))
> > +done
> > +
> > +echo "aio-dio test"
> > +align=$min_dio_align
> > +while [ $align -le $page_size ]; do
> > +	echo "$AIO_TEST -a $align $testfile.$align" >> $seqres.full
> > +	$AIO_TEST -a $align $testfile.$align 2>&1 | tee -a $seqres.full
> > +	align=$((align * 2))
> > +done
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/generic/462.out b/tests/generic/462.out
> > new file mode 100644
> > index 000000000000..9a368005af49
> > --- /dev/null
> > +++ b/tests/generic/462.out
> > @@ -0,0 +1,3 @@
> > +QA output created by 462
> > +non-aio dio test
> > +aio-dio test
> > diff --git a/tests/generic/group b/tests/generic/group
> > index dfa86edc4a93..c65058976f1e 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -464,3 +464,4 @@
> >  459 auto dangerous
> >  460 auto quick rw
> >  461 auto rw
> > +462 auto rw quick aio
> > -- 
> > 2.13.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