generic: check direct IO writes with io_uring and O_DSYNC are durable

Commit Message

Filipe Manana Nov. 7, 2022, 9:38 a.m. UTC

From: Filipe Manana <[e-mail]>

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 <[e-mail]>
---
 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

Comments

Filipe Manana Nov. 7, 2022, 9:45 a.m. UTC | #1

On Mon, Nov 7, 2022 at 9:44 AM <[e-mail]> wrote:
>
> From: Filipe Manana <[e-mail]>
>
> 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")

Wrong copy-paste, this was meant to be:

8184620ae212 ("btrfs: fix lost file sync on direct IO write with
nowait and dsync iocb")

Thanks.


>
> Signed-off-by: Filipe Manana <[e-mail]>
> ---
>  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
> +. ./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))
> +_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
>

Zorro Lang Nov. 8, 2022, 2:42 p.m. UTC | #2

On Mon, Nov 07, 2022 at 09:38:58AM +0000, [e-mail] wrote:
> From: Filipe Manana <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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".

> +. ./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.
Both are good to me.

With these:
Reviewed-by: Zorro Lang <[e-mail]>

> +_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
>

Filipe Manana Nov. 8, 2022, 2:50 p.m. UTC | #3

On Tue, Nov 8, 2022 at 2:42 PM Zorro Lang <[e-mail]> wrote:
>
> On Mon, Nov 07, 2022 at 09:38:58AM +0000, [e-mail] wrote:
> > From: Filipe Manana <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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 <[e-mail]>

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

Zorro Lang Nov. 8, 2022, 3:01 p.m. UTC | #4

On Tue, Nov 08, 2022 at 02:50:10PM +0000, Filipe Manana wrote:
> On Tue, Nov 8, 2022 at 2:42 PM Zorro Lang <[e-mail]> wrote:
> >
> > On Mon, Nov 07, 2022 at 09:38:58AM +0000, [e-mail] wrote:
> > > From: Filipe Manana <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > ---
> > >  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 <[e-mail]>
> 
> 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.

No, just check with you before I change your patch a bit.

Thanks,
Zorro

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

Filipe Manana Nov. 8, 2022, 3:09 p.m. UTC | #5

On Tue, Nov 8, 2022 at 3:02 PM Zorro Lang <[e-mail]> wrote:
>
> On Tue, Nov 08, 2022 at 02:50:10PM +0000, Filipe Manana wrote:
> > On Tue, Nov 8, 2022 at 2:42 PM Zorro Lang <[e-mail]> wrote:
> > >
> > > On Mon, Nov 07, 2022 at 09:38:58AM +0000, [e-mail] wrote:
> > > > From: Filipe Manana <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > ---
> > > >  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 <[e-mail]>
> >
> > 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.
>
> No, just check with you before I change your patch a bit.

Ok, thanks!

>
> Thanks,
> Zorro
>
> >
> > 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
> > > >
> > >
> >
>

Patch

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
+. ./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))
+_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