Re: [PATCH] generic: test which tries to exercise AIO/DIO into unwritten space

["Theodore Ts'o" <tytso@mit.edu>]

On Mon, Mar 08, 2021 at 12:24:33AM +0800, Eryu Guan wrote:
> > +trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
> 
> Better to trap a _cleanup function, even we only do "rm -f $tmp.*" in it,
> so it's consistent to other tests, and it's easier to add more cleanups
> in _cleanup() function in the future if needed.

Done.  I had based this test on generic/299 and generic/300, and a lot
of your comments are applicable to them as well.  I can send a cleanup
patch to fix up those patches as well..

> > +_require_odirect
> 
> _require_aio

Hmmm, generic/299 and generic/300 are missing _require_aio, while
doing async I/O.  I'm a bit surprised this hasn't caused problems for
other file systems.

> > +
> > +_workout()
> 
> There's no need to add the leading "_" to local function, it's reserved
> to common functions.

Done.  (Actually, if we're not unmounting $SCRATCH, then we really
don't need a workout local function at all.)

> > +	run_check $FIO_PROG $fio_config
> 
> run_check is not recommanded and should be deprecated (maybe I should
> send a patch to document it in comment), as it hides failure in
> $seqres.full and exits if command returns non-zero.
> 
> Just call $FIO_PROG command directly and check return value if
> necessary.

Thanks for suggesting dropping the run_check.  I found a problem in
the fio receipe which was causing a FIO warning that I had been
missing.

BTW, all but one of the generic are still using run_check, and in the
one exception, generic/095, which uses --output=$seqres.full which is
causing us to lose all of the output of the earlier commands which
redirected their outputs to $seqres.full.  So there's clearly a
"target rich environment" in terms of test clean up opportunities.

> > +if ! _scratch_unmount; then
> > +	echo "failed to umount"
> > +	status=1
> > +	exit
> > +fi
> 
> Is above _scratch_unmount check really needed? The test harness would
> umount $SCRATCH_DEV after test anyway.

Done.

Thanks for the review,

				- Ted