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

[Eryu Guan <guan@eryu.me>]

On Wed, Feb 10, 2021 at 03:58:18PM -0500, Theodore Ts'o wrote:
> This test verifies that the an unwritten extent is properly marked as
> written after writing into it.
> 
> There was a hard-to-hit bug which would occasionally trigger with ext4
> for which this test was a reproducer.  This has been fixed after
> moving ext4 to use iomap for Direct I/O's, although as of this
> writing, there are still some occasional failures on ext4 when block
> size < page size.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Sorry for the late review, it was in my to-review list and I just got to
it..

> ---
>  tests/generic/623     | 109 ++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/623.out |   4 ++
>  tests/generic/group   |   1 +
>  3 files changed, 114 insertions(+)
>  create mode 100755 tests/generic/623
>  create mode 100644 tests/generic/623.out
> 
> diff --git a/tests/generic/623 b/tests/generic/623
> new file mode 100755
> index 00000000..74136fef
> --- /dev/null
> +++ b/tests/generic/623
> @@ -0,0 +1,109 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +#
> +# FSQA Test No. 623
> +#
> +# AIO/DIO stress test
> +# Run random AIO/DIO activity on an file system with unwritten regions
> +#
> +# This test verifies that the an unwritten extent is properly marked
> +# as written after writing into it.
> +#
> +# There was a hard-to-hit bug which would occasionally trigger with
> +# ext4 for which this test was a reproducer.  This has been fixed
> +# after moving ext4 to use iomap for Direct I/O's, although as of this
> +# writing, there are still some occasional failures on ext4 when block
> +# size < page size.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +fio_config=$tmp.fio
> +status=1	# failure is the default!
> +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.

> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_fs generic
> +_require_test
> +_require_scratch
> +_require_odirect

_require_aio

> +_require_block_device $SCRATCH_DEV
> +
> +NUM_JOBS=$((4*LOAD_FACTOR))
> +BLK_DEV_SIZE=`blockdev --getsz $SCRATCH_DEV`
> +FILE_SIZE=$(((BLK_DEV_SIZE * 512) * 3 / 4))
> +
> +max_file_size=$((5 * 1024 * 1024 * 1024))
> +if [ $max_file_size -lt $FILE_SIZE ]; then
> +	FILE_SIZE=$max_file_size
> +fi
> +SIZE=$((FILE_SIZE / 2))
> +
> +cat >$fio_config <<EOF
> +###########
> +# $seq test fio activity
> +# Filenames derived from jobsname and jobid like follows:
> +# ${JOB_NAME}.${JOB_ID}.${ITERATION_ID}
> +[global]
> +ioengine=libaio
> +bs=128k
> +directory=${SCRATCH_MNT}
> +filesize=${FILE_SIZE}
> +size=${SIZE}
> +iodepth=$((128*$LOAD_FACTOR))
> +fallocate=native
> +
> +# Perform direct aio and verify data
> +# This test case should check use-after-free issues
> +[aio-dio-verifier]
> +numjobs=1
> +verify=crc32c-intel
> +verify_fatal=1
> +verify_dump=1
> +verify_backlog=1024
> +verify_async=4
> +direct=1
> +blocksize_range=4100k-8200k
> +blockalign=4k
> +rw=randwrite
> +filename=test-file
> +
> +EOF
> +
> +rm -f $seqres.full
> +
> +_require_fio $fio_config
> +_require_xfs_io_command "falloc"
> +
> +_workout()

There's no need to add the leading "_" to local function, it's reserved
to common functions.

> +{
> +	echo ""
> +	echo "Run fio with random aio-dio pattern"
> +	echo ""
> +	cat $fio_config >>  $seqres.full
> +	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.

> +}
> +
> +_scratch_mkfs >> $seqres.full 2>&1
> +_scratch_mount
> +
> +if ! _workout; then
> +	_scratch_unmount 2>/dev/null
> +	exit
> +fi
> +
> +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.

Thanks,
Eryu

> +status=0
> +exit
> diff --git a/tests/generic/623.out b/tests/generic/623.out
> new file mode 100644
> index 00000000..e10c7fd9
> --- /dev/null
> +++ b/tests/generic/623.out
> @@ -0,0 +1,4 @@
> +QA output created by 623
> +
> +Run fio with random aio-dio pattern
> +
> diff --git a/tests/generic/group b/tests/generic/group
> index b10fdea4..24f53ed7 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -625,3 +625,4 @@
>  620 auto mount quick
>  621 auto quick encrypt
>  622 auto shutdown metadata atime
> +623 aio rw stress
> -- 
> 2.30.0