Re: [PATCH v2] generic: concurrent IO test with mixed IO types

[Dave Chinner <david@fromorbit.com>]

On Wed, Jun 10, 2015 at 11:01:57AM +0200, Lukáš Czerner wrote:
> On Wed, 10 Jun 2015, Dave Chinner wrote:
> 
> > Date: Wed, 10 Jun 2015 08:29:33 +1000
> > From: Dave Chinner <david@fromorbit.com>
> > To: Eryu Guan <eguan@redhat.com>
> > Cc: fstests@vger.kernel.org, lczerner@redhat.com
> > Subject: Re: [PATCH v2] generic: concurrent IO test with mixed IO types
> > 
> > On Mon, Jun 08, 2015 at 08:41:11PM +0800, Eryu Guan wrote:
> > > Test concurrent buffered I/O, DIO, AIO, mmap I/O and splice I/O on the
> > > same files.
> > > 
> > > Signed-off-by: Eryu Guan <eguan@redhat.com>
> > > ---
> > > 
> > > This fio job file has been proven to be potent, it triggers WARNINGs on ext4
> > > and xfs with 4.1-rc6 kernel.
> > > 
> > > ext4: WARNING: at fs/ext4/inode.c:1328
> > > xfs: WARNING: CPU: 7 PID: 3090 at fs/xfs/xfs_file.c:726 xfs_file_dio_aio_write+0x176/0x2a8 [xfs]()
> > > 
> > > The ext4 issue should be fixed by Lukas's patch
> > > ext4: fix reservation release on invalidatepage for delalloc fs
> > > 
> > > And it ever paniced kernel in mm code and hung xfs.
> > > 
> > > I reduced the numjobs and iodepth to reduce the test time(~25s on my test host)
> > > and scale them by $LOAD_FACTOR. And it still could trigger the warning on ext4
> > > and xfs with reduced workload.
> > > 
> > > v2:
> > > - use mktemp to create tmp fio job file
> > ....
> > > +seq=`basename $0`
> > > +seqres=$RESULT_DIR/$seq
> > > +echo "QA output created by $seq"
> > > +
> > > +here=`pwd`
> > > +fio_config=`mktemp`
> > > +status=1	# failure is the default!
> > > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > 
> > By removing the definition of $tmp, you are now dumping all
> > the temporary files the test harnes creates in /.
> 
> What temp files ? Yes we're sometimes using $tmp even though there
> is no obvious definition and if we want to rely on the existence of
> this variable we better define it as environment variable in 'check'
> script.

About 80% of the files in the repository use $tmp in some way.
And it's used all over the place in common/*, too. e.g mkfs
and check functions for storing output for parsing....

> It may be enough to simply add
>
> export tmp
> 
> to the 'check' script

Then everything uses the same tmp file prefix (i.e. the pid of the
check script) rather than a test specific pid so we lose out on
debugging capability there, not to mention that "rm -f $tmp*" in a
test (like the majority of tests do in their cleanup() routine) will
remove all the tmp files that the test harness needs to do it's
stuff....

> But regardless of this bug it does not affect this test in any way
> since it's not calling any of those functions and there are other
> tests that does not define $tmp as well.

Which points out a couple more problems with the test to me. It uses
SCRATCH_MNT without calling _scratch_mkfs - which uses $tmp when
FSTYP=xfs - and it doesn't call _scratch_mount, either, so it's
running on the underlying filesystem rather than the filesystem it
is supposed to test.

And because it uses _require_scratch(), then ./check will call
_check_scratch_fs() to check the filesystem, and if FSTYP=xfs then
_check_xfs_filesystem is called and that uses $tmp....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com