On Fri, Feb 27, 2015 at 9:40 AM, Lukáš Czerner <lczerner@xxxxxxxxxx> wrote:
> On Thu, 26 Feb 2015, Filipe Manana wrote:
>
>> Date: Thu, 26 Feb 2015 19:53:14 +0000
>> From: Filipe Manana <fdmanana@xxxxxxxx>
>> To: fstests@xxxxxxxxxxxxxxx
>> Cc: linux-btrfs@xxxxxxxxxxxxxxx, Filipe Manana <fdmanana@xxxxxxxx>
>> Subject: [PATCH] Generic test for file fsync after moving files across
>> directories
>>
>> Test file fsync after moving one file between directories and fsyncing the
>> old parent directory before fsyncing the file. Verify that after a crash the
>> file data we fsynced is available.
>>
>> This test is motivated by an issue discovered in btrfs which caused the file
>> data to be lost (despite fsync returning success to user space). That btrfs
>> bug is fixed by the following linux kernel patch:
>>
>> Btrfs: fix data loss in the fast fsync path
>
> Hi,
>
> some comments below.
>
>>
>> Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
>> ---
>> tests/generic/067 | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> tests/generic/067.out | 11 +++++
>> tests/generic/group | 1 +
>> 3 files changed, 125 insertions(+)
>> create mode 100755 tests/generic/067
>> create mode 100644 tests/generic/067.out
>>
>> diff --git a/tests/generic/067 b/tests/generic/067
>> new file mode 100755
>> index 0000000..90f26b2
>> --- /dev/null
>> +++ b/tests/generic/067
>> @@ -0,0 +1,113 @@
>> +#! /bin/bash
>> +# FS QA Test No. 067
>> +#
>> +# Test file fsync after moving one file between directories and fsyncing the
>> +# old parent directory before fsyncing the file. Verify that after a crash the
>> +# file data we fsynced is available.
>> +#
>> +# This test is motivated by an issue discovered in btrfs which caused the file
>> +# data to be lost (despite fsync returning success to user space). That btrfs
>> +# bug was fixed by the following linux kernel patch:
>> +#
>> +# Btrfs: fix data loss in the fast fsync path
>> +#
>> +#-----------------------------------------------------------------------
>> +# Copyright (C) 2015 SUSE Linux Products GmbH. All Rights Reserved.
>> +# Author: Filipe Manana <fdmanana@xxxxxxxx>
>> +#
>> +# This program is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU General Public License as
>> +# published by the Free Software Foundation.
>> +#
>> +# This program is distributed in the hope that it would be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write the Free Software Foundation,
>> +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
>> +#-----------------------------------------------------------------------
>> +#
>> +
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1 # failure is the default!
>> +
>> +_cleanup()
>> +{
>> + _cleanup_flakey
>> + rm -f $tmp.*
>> +}
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +. ./common/dmflakey
>> +
>> +# real QA test starts here
>> +_supported_fs generic
>> +_supported_os Linux
>> +_need_to_be_root
>> +_require_scratch
>> +_require_dm_flakey
>> +_require_metadata_journaling $SCRATCH_DEV
>> +
>> +rm -f $seqres.full
>> +
>> +_scratch_mkfs >> $seqres.full 2>&1
>> +_init_flakey
>> +_mount_flakey
>> +
>> +# Create our main test file 'foo', the one we check for data loss.
>> +# By doing an fsync against our file, it makes btrfs clear the 'needs_full_sync'
>> +# bit from its flags (btrfs inode specific flags).
>
>
> I am not sure we need that last sentence, it's btrfs specific and I am sure
> it has been already explained in detail in the btrfs commit
> description.
Right, I left it because without this fsync the bug isn't triggered
and the reader could find it weird because shortly after there's a
'sync' call. So I wanted to emphasize that fsync's importance.
>
>
>> +$XFS_IO_PROG -f -c "pwrite -S 0xaa 0 8K" \
>> + -c "fsync" $SCRATCH_MNT/foo | _filter_xfs_io
>> +
>> +# Now create one other file and 2 directories. We will move this second file
>> +# from one directory to the other later because it forces btrfs to commit its
>> +# currently open transaction if we fsync the old parent directory. This is
>> +# necessary to trigger the data loss bug that affected btrfs.
>> +mkdir $SCRATCH_MNT/testdir_1
>> +touch $SCRATCH_MNT/testdir_1/bar
>> +mkdir $SCRATCH_MNT/testdir_2
>> +
>> +# Make sure everything is durably persisted.
>> +sync
>> +
>> +# Write more 8Kb of data to our file.
>> +$XFS_IO_PROG -c "pwrite -S 0xbb 8K 8K" $SCRATCH_MNT/foo | _filter_xfs_io
>> +
>> +# Move our 'bar' file into a new directory.
>> +mv $SCRATCH_MNT/testdir_1/bar $SCRATCH_MNT/testdir_2/bar
>> +
>> +# Fsync our first directory. Because it had a file moved into some other
>> +# directory, this made btrfs commit the currently open transaction. This is
>> +# a condition necessary to trigger the data loss bug.
>> +$XFS_IO_PROG -c "fsync" $SCRATCH_MNT/testdir_1
>> +
>> +# Now fsync our main test file. If the fsync succeeds, we expect the 8Kb of
>> +# data we wrote previously to be persisted and available if a crash happens.
>> +# This did not happen with btrfs, because of the transaction commit that
>> +# happened when we fsynced the parent directory.
>> +$XFS_IO_PROG -c "fsync" $SCRATCH_MNT/foo
>
> Ok, so from the description of the test I though that the data loss
> will happen on the file we're moving, but apparently its about a
> different unrelated file.
Good point, should have been more explicit. How about the following description?
"# Test file A fsync after moving one other unrelated file B between directories
# and fsyncing B's old parent directory before fsyncing the file A. Check that
# after a crash all the file A data we fsynced is available."
Thanks.
>
> Otherwise the test looks fine.
>
> Reviewed-by: Lukas Czerner <lczerner@xxxxxxxxxx>
>
> Thanks!
> -Lukas
>
>> +
>> +# Simulate a crash/power loss.
>> +_load_flakey_table $FLAKEY_DROP_WRITES
>> +_unmount_flakey
>> +
>> +_load_flakey_table $FLAKEY_ALLOW_WRITES
>> +_mount_flakey
>> +
>> +# Now check that all data we wrote before are available.
>> +echo "File content after log replay:"
>> +od -t x1 $SCRATCH_MNT/foo
>> +
>> +status=0
>> +exit
>> diff --git a/tests/generic/067.out b/tests/generic/067.out
>> new file mode 100644
>> index 0000000..f30f6ef
>> --- /dev/null
>> +++ b/tests/generic/067.out
>> @@ -0,0 +1,11 @@
>> +QA output created by 067
>> +wrote 8192/8192 bytes at offset 0
>> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +wrote 8192/8192 bytes at offset 8192
>> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +File content after log replay:
>> +0000000 aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa
>> +*
>> +0020000 bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb
>> +*
>> +0040000
>> diff --git a/tests/generic/group b/tests/generic/group
>> index e5db772..05cc6a9 100644
>> --- a/tests/generic/group
>> +++ b/tests/generic/group
>> @@ -69,6 +69,7 @@
>> 064 auto quick prealloc
>> 065 metadata auto quick
>> 066 metadata auto quick
>> +067 metadata auto quick
>> 068 other auto freeze dangerous stress
>> 069 rw udf auto quick
>> 070 attr udf auto quick stress
>>
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Filipe David Manana,
"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves.
That's why all progress depends on unreasonable men."
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
