On Wed, 10 Jun 2015, Dave Chinner wrote:
> Date: Wed, 10 Jun 2015 21:11:53 +1000
> From: Dave Chinner <[email protected]>
> To: Lukáš Czerner <[email protected]>
> Cc: Eryu Guan <[email protected]>, [email protected]
> 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 <[email protected]>
> > > To: Eryu Guan <[email protected]>
> > > Cc: [email protected], [email protected]
> > > 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 <[email protected]>
> > > > ---
> > > >
> > > > 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.
Removes immediatelly
common/attr _acl_get_max
common/filter _within_tolerance
common/rc _check_generic_filesystem
common/rc _check_xfs_filesystem
common/rc _check_btrfs_filesystem
Does not remove
common/dump _check_onl
_create_dumpdir_stress_num
_mk_fillconfig1
_mk_fillconfig2
_mk_fillconfig_ea
_mk_fillconfig_xattr
_mk_fillconfig_multi
_append_dumpdir_fill
_do_create_dump_symlinks
_mk_symlink_config
_do_dump_restore
_ls_compare_sub
_ls_nodate_compare_sub
_create_dumpdir_stress_num
common/quota _check_quota_usage
common/rc _do
Most of the users are in the common/dump, however it's broken anyway
since on _cleanup() it does "rm -f $tmp.*" that'll remove all the
tmp file xfstests supposedly use.
>
> > 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....
What debugging ? First of all, all of the $tmp.* files are removed either
immediately or when the script ends and some of them are actually
dumped to the .full report anyway.
But regardless, the "rm -f $tmp*" called from the test _cleanup()
routine will _not_ remove tmp files that the test harness needs as
long as it defined it's own $tmp, so really using 'export' is the
right way for test that does not need their own tmp, or creates a
different name for they temp file, or directory.
Not even mentioning that there is not safety check to prevent us
from overwriting each other tmp files, or even temp files of
completely different unrelated process ( that's why we should use
mktemp).
>
> > 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.
Right, that's a bug. Moreover if the test is the first test, or the
only test you run everything will be ok becuase 'check' script will
_scratch_mkfs and _scratch_mount for you for some reason ...
>
> 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....
Not really. 'check' script calls _check_scratch_fs() and the $tmp is
defined there regardless of what the test itself is doing.
Regards,
-Lukas
>
> Cheers,
>
> Dave.
>
