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

[Dave Chinner <david@fromorbit.com>]

On Wed, Jun 10, 2015 at 02:22:55PM +0200, Lukáš Czerner wrote:
> On Wed, 10 Jun 2015, Dave Chinner wrote:
> 
> > Date: Wed, 10 Jun 2015 21:11:53 +1000
> > From: Dave Chinner <david@fromorbit.com>
> > To: Lukáš Czerner <lczerner@redhat.com>
> > Cc: Eryu Guan <eguan@redhat.com>, fstests@vger.kernel.org
> > Subject: Re: [PATCH v2] generic: concurrent IO test with mixed IO types
> > 
> > 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....
> 
> Ah, come on. There are not that many functions using the $tmp and
> some of them even removes the file immediately.

$ git grep -lw "^tmp=" |wc -l
484
$ git grep -lw "\$tmp" |wc -l
417

That's roughly 80% of files that use $tmp /in some way/. The tests
are all suppose dto define $tmp the same way, and they are all
expected to clean up after themselves, thereby making it possible to
safely use $tmp in linrary functions, whether they clean up after
tehmselves or not.

You're spending way more time and effort than is necessary on basic
infrastructure that has worked for 15 years and, quite frankly,
*doesn't need fixing because it's not broken*.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com