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@xxxxxxx>
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
