| Message ID | abbc821350c8ef515e0a0317b5cbd64e3c5b81ab.1645099449.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: test log replay after fsync of file with prealloc extents beyond eof | expand |
On Thu, Feb 17, 2022 at 12:14:21PM +0000, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > Test that after a full fsync of a file with preallocated extents beyond > the file's size, if a power failure happens, the preallocated extents > still exist after we mount the filesystem. > > This test currently fails and there is a patch for btrfs that fixes this > issue and has the following subject: > > "btrfs: fix lost prealloc extents beyond eof after full fsync" > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > tests/btrfs/261 | 79 +++++++++++++++++++++++++++++++++++++++++++++ > tests/btrfs/261.out | 10 ++++++ What is btrfs specific about this test? Cheers, Dave.
On Tue, Feb 22, 2022 at 11:44 PM Dave Chinner <david@fromorbit.com> wrote: > > On Thu, Feb 17, 2022 at 12:14:21PM +0000, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > Test that after a full fsync of a file with preallocated extents beyond > > the file's size, if a power failure happens, the preallocated extents > > still exist after we mount the filesystem. > > > > This test currently fails and there is a patch for btrfs that fixes this > > issue and has the following subject: > > > > "btrfs: fix lost prealloc extents beyond eof after full fsync" > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > --- > > tests/btrfs/261 | 79 +++++++++++++++++++++++++++++++++++++++++++++ > > tests/btrfs/261.out | 10 ++++++ > > What is btrfs specific about this test? The comments that explain the steps are very btrfs specific. Without them it would be hard to understand why the test uses that specific file size, block size, mention of the btrfs inode's full sync bit, etc. Some years ago, when you maintained fstests, you complained once about this type of "too btrfs specific" comments on generic tests. > > Cheers, > > Dave. > > -- > Dave Chinner > david@fromorbit.com
On Wed, Feb 23, 2022 at 12:12:10PM +0000, Filipe Manana wrote: > On Tue, Feb 22, 2022 at 11:44 PM Dave Chinner <david@fromorbit.com> wrote: > > > > On Thu, Feb 17, 2022 at 12:14:21PM +0000, fdmanana@kernel.org wrote: > > > From: Filipe Manana <fdmanana@suse.com> > > > > > > Test that after a full fsync of a file with preallocated extents beyond > > > the file's size, if a power failure happens, the preallocated extents > > > still exist after we mount the filesystem. > > > > > > This test currently fails and there is a patch for btrfs that fixes this > > > issue and has the following subject: > > > > > > "btrfs: fix lost prealloc extents beyond eof after full fsync" > > > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > > --- > > > tests/btrfs/261 | 79 +++++++++++++++++++++++++++++++++++++++++++++ > > > tests/btrfs/261.out | 10 ++++++ > > > > What is btrfs specific about this test? > > The comments that explain the steps are very btrfs specific. > Without them it would be hard to understand why the test uses that > specific file size, block size, mention of the btrfs inode's full sync > bit, etc. But the behaviour and layout should end up being the same for all filesystems, right? We have plenty of generic tests that are designed with a specific configuration on a specific filesystem to reproduce a bug seen on that filesystem, but as long as all filesystems should be expected to behave the same way, it's a generic test. AFAICT, the behaviour described and exercised by the test should be the same for all filesystems that support preallocation beyond EOF. Hence it isn't btrfs specific behaviour being exercised, just a reproducing a bug where btrfs deviates from the correct behaviour that should be consistent across all filesystems. > Some years ago, when you maintained fstests, you complained once about > this type of "too btrfs specific" comments on generic tests. I can change my mind if I want. Besides, I'm looking at this new test purely on it's own merits so past discussions aren't really relevant. Maybe you didn't understand the context under which I was considering a patch to be "too fs specific" rather than generic. There's a big difference between "only btrfs will behave this way" and "all filesystems should get the same result, but btrfs sometimes fails to get that results in this specific case". The former should be a btrfs-only test, the latter should be a generic test. Which class this test falls into is exactly what I'm asking here - should all filesystems get the same result, or is successful result encoded in the golden output behaviour that only btrfs will ever produce? Cheers, Dave.
On Thu, Feb 24, 2022 at 2:21 AM Dave Chinner <david@fromorbit.com> wrote: > > On Wed, Feb 23, 2022 at 12:12:10PM +0000, Filipe Manana wrote: > > On Tue, Feb 22, 2022 at 11:44 PM Dave Chinner <david@fromorbit.com> wrote: > > > > > > On Thu, Feb 17, 2022 at 12:14:21PM +0000, fdmanana@kernel.org wrote: > > > > From: Filipe Manana <fdmanana@suse.com> > > > > > > > > Test that after a full fsync of a file with preallocated extents beyond > > > > the file's size, if a power failure happens, the preallocated extents > > > > still exist after we mount the filesystem. > > > > > > > > This test currently fails and there is a patch for btrfs that fixes this > > > > issue and has the following subject: > > > > > > > > "btrfs: fix lost prealloc extents beyond eof after full fsync" > > > > > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > > > --- > > > > tests/btrfs/261 | 79 +++++++++++++++++++++++++++++++++++++++++++++ > > > > tests/btrfs/261.out | 10 ++++++ > > > > > > What is btrfs specific about this test? > > > > The comments that explain the steps are very btrfs specific. > > Without them it would be hard to understand why the test uses that > > specific file size, block size, mention of the btrfs inode's full sync > > bit, etc. > > But the behaviour and layout should end up being the same for all > filesystems, right? Right. > > We have plenty of generic tests that are designed with a > specific configuration on a specific filesystem to reproduce a bug > seen on that filesystem, but as long as all filesystems should be > expected to behave the same way, it's a generic test. > > AFAICT, the behaviour described and exercised by the test should be > the same for all filesystems that support preallocation beyond EOF. > Hence it isn't btrfs specific behaviour being exercised, just a > reproducing a bug where btrfs deviates from the correct behaviour > that should be consistent across all filesystems. > > > Some years ago, when you maintained fstests, you complained once about > > this type of "too btrfs specific" comments on generic tests. > > I can change my mind if I want. Besides, I'm looking at this new > test purely on it's own merits so past discussions aren't really > relevant. Maybe you didn't understand the context under which I was > considering a patch to be "too fs specific" rather than generic. > > There's a big difference between "only btrfs will behave this way" > and "all filesystems should get the same result, but btrfs sometimes > fails to get that results in this specific case". The former should > be a btrfs-only test, the latter should be a generic test. > > Which class this test falls into is exactly what I'm asking here - > should all filesystems get the same result, or is successful > result encoded in the golden output behaviour that only btrfs will > ever produce? As far as I know, all filesystems supporting fallocate should behave the same way. Ok, I can move into the generic group. Thanks. > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
diff --git a/tests/btrfs/261 b/tests/btrfs/261 new file mode 100755 index 00000000..8275e6a5 --- /dev/null +++ b/tests/btrfs/261 @@ -0,0 +1,79 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2022 SUSE Linux Products GmbH. All Rights Reserved. +# +# FS QA Test 261 +# +# Test that after a full fsync of a file with preallocated extents beyond the +# file's size, if a power failure happens, the preallocated extents still exist +# after we mount the filesystem. +# +. ./common/preamble +_begin_fstest auto quick log prealloc + +_cleanup() +{ + _cleanup_flakey + cd / + rm -r -f $tmp.* +} + +. ./common/rc +. ./common/filter +. ./common/dmflakey +. ./common/punch + +# real QA test starts here + +_supported_fs btrfs +_require_scratch +_require_dm_target flakey +_require_xfs_io_command "falloc" "-k" +_require_xfs_io_command "fiemap" +_require_odirect + +rm -f $seqres.full + +_scratch_mkfs >>$seqres.full 2>&1 +_require_metadata_journaling $SCRATCH_DEV +_init_flakey +_mount_flakey + +# Create our test file with many file extent items, so that they span several +# leaves of metadata, even if the node/page size is 64K. We use direct IO and +# not fsync/O_SYNC because it's both faster and it avoids clearing the full sync +# flag from the inode - we want the fsync below to trigger the slow full sync +# code path. +$XFS_IO_PROG -f -d -c "pwrite -b 4K 0 16M" $SCRATCH_MNT/foo | _filter_xfs_io + +# Now add two preallocated extents to our file without extending the file's size. +# One right at i_size, and another further beyond, leaving a gap between the two +# prealloc extents. +$XFS_IO_PROG -c "falloc -k 16M 1M" $SCRATCH_MNT/foo +$XFS_IO_PROG -c "falloc -k 20M 1M" $SCRATCH_MNT/foo + +# Make sure everything is durably persisted and the transaction is committed. +# This makes all created extents to have a generation lower than the generation +# of the transaction used by the next write and fsync. +sync + +# Now overwrite only the first extent, which will result in modifying only the +# first leaf of metadata for our inode. Then fsync it. This fsync will use the +# slow code path (inode full sync bit is set) because it's the first fsync since +# the inode was created/loaded. +$XFS_IO_PROG -c "pwrite 0 4K" -c "fsync" $SCRATCH_MNT/foo | _filter_xfs_io + +# Simulate a power failure and then mount again the filesystem to replay the log +# tree. +_flakey_drop_and_remount + +# After the power failure we expect that the preallocated extents, beyond the +# inode's i_size, still exist. +echo "List of extents after power failure:" +$XFS_IO_PROG -c "fiemap -v" $SCRATCH_MNT/foo | _filter_fiemap + +_unmount_flakey + +# success, all done +status=0 +exit diff --git a/tests/btrfs/261.out b/tests/btrfs/261.out new file mode 100644 index 00000000..e9cfe1e8 --- /dev/null +++ b/tests/btrfs/261.out @@ -0,0 +1,10 @@ +QA output created by 261 +wrote 16777216/16777216 bytes at offset 0 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 0 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +List of extents after power failure: +0: [0..32767]: data +1: [32768..34815]: unwritten +2: [34816..40959]: hole +3: [40960..43007]: unwritten