On Tue, Nov 8, 2022 at 2:42 PM Zorro Lang <zlang@xxxxxxxxxx> wrote:
>
> On Mon, Nov 07, 2022 at 09:38:58AM +0000, fdmanana@xxxxxxxxxx wrote:
> > From: Filipe Manana <fdmanana@xxxxxxxx>
> >
> > Test that direct IO writes with io_uring and O_DSYNC are durable if a power
> > failure happens after they complete.
> >
> > This is motivated by a regression on btrfs, affecting 5.15 stable kernels
> > and kernels up to 6.0, where often the writes were not persisted (same
> > behaviour as if O_DSYNC was not provided). This was recently fixed by the
> > following commit:
> >
> > 51bd9563b678 ("btrfs: fix deadlock due to page faults during direct IO reads and writes")
> >
> > Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
> > ---
> > tests/generic/703 | 104 ++++++++++++++++++++++++++++++++++++++++++
> > tests/generic/703.out | 2 +
> > 2 files changed, 106 insertions(+)
> > create mode 100755 tests/generic/703
> > create mode 100644 tests/generic/703.out
> >
> > diff --git a/tests/generic/703 b/tests/generic/703
> > new file mode 100755
> > index 00000000..39ae3773
> > --- /dev/null
> > +++ b/tests/generic/703
> > @@ -0,0 +1,104 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
> > +#
> > +# FS QA Test 703
> > +#
> > +# Test that direct IO writes with io_uring and O_DSYNC are durable if a power
> > +# failure happens after they complete.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick log prealloc io_uring
> > +
> > +_cleanup()
> > +{
> > + _cleanup_flakey
> > + cd /
> > + rm -r -f $tmp.*
> > +}
> > +
> > +. ./common/filter
>
> This patch looks very good to me, it even doesn't miss any _require_* helpers
> or group names, and its comments are good enough. I can't pick up any problems
> from my side, just two tiny review points (as you will change the commit log
> at least, so might change them by the way:).
>
> 1) This case doesn't use any filter functions, so don't need "common/filter".
Yes. That came from that template.
Certainly, it doesn't hurt anyone to have the file sourced.
>
> > +. ./common/dmflakey
> > +
> > +fio_config=$tmp.fio
> > +fio_out=$tmp.fio.out
> > +test_file="${SCRATCH_MNT}/foo"
> > +
> > +[ $FSTYP == "btrfs" ] &&
> > + _fixed_by_kernel_commit 8184620ae212 \
> > + "btrfs: fix lost file sync on direct IO write with nowait and dsync iocb"
> > +
> > +_supported_fs generic
> > +# We allocate 256M of data for the test file, so require a higher size of 512M
> > +# which gives a margin of safety for a COW filesystem like btrfs (where metadata
> > +# is always COWed).
> > +_require_scratch_size $((512 * 1024))
>
> 2) I think nearly no one use a SCRATCH_DEV < 512M to run fstests, but I can't
> say this's wrong, so you can decide to keep or remove this line by yourself.
Well, that I can't tell. For filesystems other than btrfs, it's
possible someone is interested in
testing very small devices, I don't know. Again, this doesn't hurt.
> Both are good to me.
>
> With these:
> Reviewed-by: Zorro Lang <zlang@xxxxxxxxxx>
So, do you expect me to do something else?
Surely the unnecessary import of common/filter, is something trivial
enough you could amend when you pick the patch?
That's the sort of thing Eryu and Dave did (unless there were really
important things to fix in a test). The changelog also
has a wrong commit as I previously noted.
Please let me know if you expect something more from my side.
Thanks.
>
> > +_require_odirect
> > +_require_io_uring
> > +_require_dm_target flakey
> > +_require_xfs_io_command "falloc"
> > +
> > +cat >$fio_config <<EOF
> > +[test_io_uring_dio_dsync]
> > +ioengine=io_uring
> > +direct=1
> > +bs=64K
> > +sync=1
> > +filename=$test_file
> > +rw=randwrite
> > +time_based
> > +runtime=10
> > +EOF
> > +
> > +_require_fio $fio_config
> > +
> > +_scratch_mkfs >>$seqres.full 2>&1
> > +_require_metadata_journaling $SCRATCH_DEV
> > +_init_flakey
> > +_mount_flakey
> > +
> > +# We do 64K writes in the fio job.
> > +_require_congruent_file_oplen $SCRATCH_MNT $((64 * 1024))
> > +
> > +touch $test_file
> > +
> > +# On btrfs IOCB_NOWAIT writes can only be done on NOCOW files, so enable
> > +# nodatacow on the file if we are running on btrfs.
> > +if [ $FSTYP == "btrfs" ]; then
> > + _require_chattr C
> > + $CHATTR_PROG +C $test_file
> > +fi
> > +
> > +$XFS_IO_PROG -c "falloc 0 256M" $test_file
> > +
> > +# Persist everything, make sure the file exists after power failure.
> > +sync
> > +
> > +echo -e "Running fio with config:\n" >> $seqres.full
> > +cat $fio_config >> $seqres.full
> > +
> > +$FIO_PROG $fio_config --output=$fio_out
> > +
> > +echo -e "\nOutput from fio:\n" >> $seqres.full
> > +cat $fio_out >> $seqres.full
> > +
> > +digest_before=$(_md5_checksum $test_file)
> > +
> > +# Simulate a power failure and mount the filesystem to check that all the data
> > +# previously written are available.
> > +_flakey_drop_and_remount
> > +
> > +digest_after=$(_md5_checksum $test_file)
> > +
> > +if [ "$digest_after" != "$digest_before" ]; then
> > + echo "Error: not all file data got persisted."
> > + echo "Digest before power failure: $digest_before"
> > + echo "Digest after power failure: $digest_after"
> > +fi
> > +
> > +_unmount_flakey
> > +
> > +# success, all done
> > +echo "Silence is golden"
> > +status=0
> > +exit
> > diff --git a/tests/generic/703.out b/tests/generic/703.out
> > new file mode 100644
> > index 00000000..fba62571
> > --- /dev/null
> > +++ b/tests/generic/703.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 703
> > +Silence is golden
> > --
> > 2.35.1
> >
>
