Re: [PATCH] xfs: test a race between dio reads and mapped writes

[Eryu Guan <eguan@redhat.com>]

On Thu, Jun 29, 2017 at 01:21:12PM +0800, Xiao Yang wrote:
> On 2017/06/29 12:34, Eryu Guan wrote:
> > On Thu, Jun 29, 2017 at 12:01:25PM +0800, Xiao Yang wrote:
> > > On 2017/06/28 18:13, Eryu Guan wrote:
> > > > On Tue, Jun 27, 2017 at 05:03:52PM +0800, Xiao Yang wrote:
> > > > > This regression test reproduces a race between a direct
> > > > > I/O read and a mapped write to a hole in a file.  If the
> > > > > race occurs, it will trigger a BUG_ON().  This XFS bug
> > > > > has been fixed by:
> > > > > 
> > > > > 04197b3 (xfs: don't BUG() on mixed direct and mapped I/O)
> > > > > 
> > > > > Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> > > > > ---
> > > > >    tests/xfs/422     | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >    tests/xfs/422.out |  2 ++
> > > > >    tests/xfs/group   |  1 +
> > > > >    3 files changed, 89 insertions(+)
> > > > >    create mode 100755 tests/xfs/422
> > > > >    create mode 100644 tests/xfs/422.out
> > > > > 
> > > > > diff --git a/tests/xfs/422 b/tests/xfs/422
> > > > > new file mode 100755
> > > > > index 0000000..a57ffab
> > > > > --- /dev/null
> > > > > +++ b/tests/xfs/422
> > > > > @@ -0,0 +1,86 @@
> > > > > +#! /bin/bash
> > > > > +# FS QA Test No. 422
> > > > > +#
> > > > > +# Regression test for commit:
> > > > > +# 04197b3 ("xfs: don't BUG() on mixed direct and mapped I/O)
> > > >                ^^ missing ending " :)
> > > > > +#
> > > > > +# This case tests a race between a direct I/O read and a
> > > > > +# mapped write to a hole in a file.  If the race occurs,
> > > > > +# it will trigger a BUG_ON().
> > > > > +#
> > > > > +#-----------------------------------------------------------------------
> > > > > +# Copyright (c) 2017 Fujitsu.  All Rights Reserved.
> > > > > +# Author: Xiao Yang<yangx.jy@cn.fujitsu.com>
> > > > > +#
> > > > > +# 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()
> > > > > +{
> > > > > +	rm -rf $tmp.*
> > > > > +}
> > > > > +
> > > > > +# get standard environment and checks
> > > > > +. ./common/rc
> > > > > +
> > > > > +# real QA test starts here
> > > > > +_supported_os Linux
> > > > > +_supported_fs xfs
> > > > I see nothing xfs specific in this test, I think it can be made generic
> > > > easily.
> > > Hi Eryu,
> > > 
> > > Agreed.  It could be moved into generic.
> > > > > +_require_scratch
> > > > > +_disable_dmesg_check
> > > > Better to add comments on ignoring dmesg check result.
> > > For xfs filesystem, this race triggers various warnings on some
> > > distributions(RHEL7) and upstream kernel.
> > > e.g,
> > > 1) This race triggers the following warnings on RHEL7.3GA
> > > -----------------------------------------------------------------------------------------------------------------------------
> > > [    0.297084] WARNING: at arch/x86/mm/ioremap.c:101
> > > __ioremap_caller+0x238/0x320()
> > I'm not sure about this one, but the other xfs or iomap warnings are
> > "known issues" when mixing direct I/O and mmap I/O on the same file.
> > We've filtered out these intentional warnings in generic/095, and Brian
> > has just sent a patch[1] to move this filter to common/xfs and added it
> > to generic/247. I think you can do the same to this new test, filter out
> > these known warnings so we won't miss any other potential bug.
> > 
> > I don't think disable dmesg check only because it triggers warnings is a
> > good idea :)
> Hi Eryu
> 
> It sounds reasonable to me.   I will filter out these known warnings instead
> of disabling dmesg check.
> > Thanks,
> > Eryu
> > 
> > [1] [PATCH] generic/247: filter out expected XFS warnings for mixed mmap/direct I/O
> > 
> > > [  648.745481] WARNING: at fs/xfs/xfs_aops.c:1244
> > > xfs_vm_releasepage+0xcb/0x100 [xfs]()
> > > [  648.745694] WARNING: at fs/xfs/xfs_file.c:377
> > > xfs_file_aio_read+0x2dd/0x340 [xfs]()
> > > -----------------------------------------------------------------------------------------------------------------------------
> > > 2) It triggers the following warnings on RHEL7.4Beta
> > > ----------------------------------------------------------------------------------------------------------------------------
> > > [  497.575319] WARNING: CPU: 0 PID: 26865 at fs/xfs/xfs_file.c:361
> > > xfs_file_dio_aio_read+0x1ba/0x2a0 [xfs]
> > > [  497.577651] WARNING: CPU: 0 PID: 26865 at fs/xfs/xfs_aops.c:1285
> > > __xfs_get_blocks+0x8ca/0x8e0 [xfs]
> Could we add __xfs_get_blocks into filter_xfs_dmesg? :-)
> 
> This known warning was introduced by:
> 04197b3 (xfs: don't BUG() on mixed direct and mapped I/O)
> 
> It has been removed by:
> acdda3a (xfs: use iomap_dio_rw)

Please send another patch with detailed background information to add it
to the filter and cc linux-xfs list, perhaps after both Brian's patch
and this new test patch being merged & pushed out to upstream. Thanks a
lot!

Eryu