On Tue, Aug 09, 2022 at 08:29:56AM +0100, Filipe Manana wrote:
> On Tue, Aug 09, 2022 at 11:35:51AM +0800, Zorro Lang wrote:
> > On Mon, Aug 08, 2022 at 12:18:58PM +0100, fdmanana@xxxxxxxxxx wrote:
> > > From: Filipe Manana <fdmanana@xxxxxxxx>
> > >
> > > Test that if we punch a hole adjacent to an existing hole, fsync the file
> > > and then power fail, the new hole exists after mounting again the
> > > filesystem.
> > >
> > > This currently fails on btrfs with kernels 5.18 and 5.19 when not using
> > > the "no-holes" feature. The "no-holes" feature is enabled by default at
> > > mkfs time starting with btrfs-progs 5.15, so to trigger the issue with
> > > btrfs-progs 5.15+ and kernel 5.18 or kernel 5.19, one must set
> > > "-O ^no-holes" in the MKFS_OPTIONS environment variable (part of the
> > > btrfs test matrix).
> > >
> > > The issue is fixed for btrfs with the following kernel patch:
> > >
> > > "btrfs: update generation of hole file extent item when merging holes"
> >
> > CC btrfs list
>
> It was already in cc (and I always cc the btrfs list).
>
> >
> > >
> > > Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
> > > ---
> > > tests/generic/694 | 85 +++++++++++++++++++++++++++++++++++++++++++
> > > tests/generic/694.out | 15 ++++++++
> > > 2 files changed, 100 insertions(+)
> > > create mode 100755 tests/generic/694
> > > create mode 100644 tests/generic/694.out
> > >
> > > diff --git a/tests/generic/694 b/tests/generic/694
> > > new file mode 100755
> > > index 00000000..c034f914
> > > --- /dev/null
> > > +++ b/tests/generic/694
> > > @@ -0,0 +1,85 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
> > > +#
> > > +# FS QA Test 694
> > > +#
> > > +# Test that if we punch a hole adjacent to an existing hole, fsync the file and
> > > +# then power fail, the new hole exists after mounting again the filesystem.
> >
> > Better to explain this's a known regression test at here.
>
> So, duplicate the changelog here?
>
> >
> > And add _fixed_by_kernel_commit later, after that kernel patch is merged and
> > has a fixed commit id.
>
> I wasn't aware we have that nowadays.
It's a recent addition to try to standardize the process of identifying
bugfixes for LTS kernels. Whereas before we just stuffed them adhoc in
the test comments, this new helper will tell you which commits you need
to apply if the regression test fails.
> Does that mean that tests get merged only after the corresponding kernel fix
> is merged in Linus' tree?
I usually put in an obvious placeholder:
_fixed_by_kernel_commit XXXXXX "xfs: fix the frobnitech"
...and hope someone remembers to clean it up.
> >
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest quick log punch
> >
> > "auto" group?
>
> Yes, forgotten when running the "new" script.
>
> >
> > > +
> > > +_cleanup()
> > > +{
> > > + _cleanup_flakey
> > > + cd /
> > > + rm -r -f $tmp.*
> > > +}
> > > +
> > > +# Import common functions.
> > > +. ./common/filter
> > > +. ./common/dmflakey
> > > +. ./common/punch
> > > +
> > > +# real QA test starts here
> > > +
> > > +# Modify as appropriate.
> > ^^^^
> > This's just a reminder, please remove it.
> >
> > > +_supported_fs generic
> > > +_require_scratch
> > > +_require_dm_target flakey
> > > +_require_xfs_io_command "fpunch"
> > > +_require_xfs_io_command "fiemap"
> > > +
> > > +_scratch_mkfs >>$seqres.full 2>&1
> > > +_require_metadata_journaling $SCRATCH_DEV
> > > +_init_flakey
> > > +_mount_flakey
> > > +
> > > +# Create our test file with the following layout:
> > > +#
> > > +# [0, 2M) - hole
> > > +# [2M, 10M) - extent
> > > +# [10M, 12M) - hole
> > > +$XFS_IO_PROG -f -c "truncate 12M" \
> > > + -c "pwrite -S 0xab 2M 8M" \
> > > + $SCRATCH_MNT/foobar | _filter_xfs_io
> > > +
> > > +# Persist everything, commit the filesystem's transaction.
> > > +sync
> > > +
> > > +# Now punch two holes in the file:
> > > +#
> > > +# 1) For the range [2M, 4M), which is adjacent to the existing hole in the range
> > > +# [0, 2M);
> > > +# 2) For the range [8M, 10M), which is adjacent to the existing hole in the
> > > +# range [10M, 12M).
> > > +#
> > > +# These operations start a new filesystem transaction.
> > > +# Then finally fsync the file.
> > > +$XFS_IO_PROG -c "fpunch 2M 2M" \
> > > + -c "fpunch 8M 2M" \
> > > + -c "fsync" $SCRATCH_MNT/foobar
> >
> > Darrick added a new helper _require_congruent_file_oplen(), might worth
> > using it. Any thoughts?
>
> Wasn't aware of it. Seems like it's to deal with some rare xfs realtime configurations.
> So I suppose, this needs:
Weird xfs realtime configs was the initial purpose (e.g. 28k allocation
units) but it also _notruns tests that don't expect things like punching
a 4k hole failing on an fs with 64k blocksize.
(Granted, anything in tests/generic/ should be assuming at least a 64k
block size as a possibility, as you do here...)
> _require_congruent_file_oplen $((2 * 1024 * 1024))
Yep.
>
> >
> > > +
> > > +# Simulate a power failure and mount the filesystem to check that everything
> > > +# is in the same state as before the power failure.
> > > +_flakey_drop_and_remount
> > > +
> > > +# We expect the following file layout:
> > > +#
> > > +# [0, 4M) - hole
> > > +# [4M, 8M) - extent
> > > +# [8M, 12M) - hole
> > > +echo "File layout after power failure:"
> > > +$XFS_IO_PROG -c "fiemap -v" $SCRATCH_MNT/foobar | _filter_fiemap
> > > +
> > > +# When reading the file we expect to get the range [4M, 8M) filled with bytes
> > > +# that have a value of 0xab and 0x00 for anything outside that range.
> > > +echo "File content after power failure:"
> > > +od -A d -t x1 $SCRATCH_MNT/foobar
> >
> > Can _hexdump in common/rc help ?
>
> It can, I wasn't aware that helper existed. It's relatively new.
> Glad to see od is being preferred over hexdump, and I have always used it in
> tests over the years.
>
> Btw, _hexdump is asking od to output file offsets in hex.
> I find it a lot more friendly to read decimal values (maybe I'm weird), so
> I always pass '-A d' to od. Thoughts on that?
Same here, though my preference is eroding as we convert the xfs
tracepoints to report in hexadecimal. :)
--D
> Thanks.
>
> >
> > > +
> > > +_unmount_flakey
> > > +
> > > +# success, all done
> > > +status=0
> > > +exit
> > > diff --git a/tests/generic/694.out b/tests/generic/694.out
> > > new file mode 100644
> > > index 00000000..f55212f3
> > > --- /dev/null
> > > +++ b/tests/generic/694.out
> > > @@ -0,0 +1,15 @@
> > > +QA output created by 694
> > > +wrote 8388608/8388608 bytes at offset 2097152
> > > +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > > +File layout after power failure:
> > > +0: [0..8191]: hole
> > > +1: [8192..16383]: data
> > > +2: [16384..24575]: hole
> > > +File content after power failure:
> > > +0000000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > +*
> > > +4194304 ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab
> > > +*
> > > +8388608 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > +*
> > > +12582912
> > > --
> > > 2.35.1
> > >
> >
