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
