generic: test fsync after punching hole adjacent to an existing hole

Commit Message

Filipe Manana Aug. 8, 2022, 11:18 a.m. UTC

From: Filipe Manana <[e-mail]>

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"

Signed-off-by: Filipe Manana <[e-mail]>
---
 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

Comments

Zorro Lang Aug. 9, 2022, 3:35 a.m. UTC | #1

On Mon, Aug 08, 2022 at 12:18:58PM +0100, [e-mail] wrote:
> From: Filipe Manana <[e-mail]>
> 
> 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

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

And add _fixed_by_kernel_commit later, after that kernel patch is merged and
has a fixed commit id.

> +#
> +. ./common/preamble
> +_begin_fstest quick log punch

"auto" group?

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

> +
> +# 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 ?

> +
> +_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
> +*
> [phone] 00 00 00 00 00 00 00 00 00 00 00 00
> +*
> [phone]
> -- 
> 2.35.1
>

Filipe Manana Aug. 9, 2022, 7:29 a.m. UTC | #2

On Tue, Aug 09, 2022 at 11:35:51AM +0800, Zorro Lang wrote:
> On Mon, Aug 08, 2022 at 12:18:58PM +0100, [e-mail] wrote:
> > From: Filipe Manana <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> >  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.
Does that mean that tests get merged only after the corresponding kernel fix
is merged in Linus' tree?

> 
> > +#
> > +. ./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:

_require_congruent_file_oplen $((2 * 1024 * 1024))

> 
> > +
> > +# 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?

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
> > +*
> > [phone] 00 00 00 00 00 00 00 00 00 00 00 00
> > +*
> > [phone]
> > -- 
> > 2.35.1
> > 
>

Darrick J. Wong Aug. 9, 2022, 3:13 p.m. UTC | #3

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, [e-mail] wrote:
> > > From: Filipe Manana <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > ---
> > >  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
> > > +*
> > > [phone] 00 00 00 00 00 00 00 00 00 00 00 00
> > > +*
> > > [phone]
> > > -- 
> > > 2.35.1
> > > 
> >

Zorro Lang Aug. 11, 2022, 5:05 p.m. UTC | #4

On Tue, Aug 09, 2022 at 08:13:51AM -0700, Darrick J. Wong wrote:
> 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, [e-mail] wrote:
> > > > From: Filipe Manana <[e-mail]>
> > > > 
> > > > 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 <[e-mail]>
> > > > ---
> > > >  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?

No harm I think. When someone open and read this case, he'll know it's a
known regression test cases for a known patch fix, don't need to check
the original commit log.

> > 
> > > 
> > > 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 new, Amir talked about that with me, and brought in this helper to help
downstream kernel maintain.

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

No force ... but at least the kernel fix has been reviewed I think,
to make sure there won't be big changes, or even be denied.

> 
> I usually put in an obvious placeholder:
> 
> _fixed_by_kernel_commit XXXXXX "xfs: fix the frobnitech"
> 
> ...and hope someone remembers to clean it up.

You can refer to what Darrick does, or make up that later.

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

To replaced hexdump with od, we brought in _hexdump() and changed lots of old
cases. Due to old cases with hexdump output hexadecimal number in .out
file, so I tried keep same format in new _hexdump() function

If most of us prefer decimal values output, we can help _hexdump to accept
an argument to change the default format. Or write a _decdump, as the
current one calls *hex*dump :-P

Thanks,
Zorro

> 
> --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
> > > > +*
> > > > [phone] 00 00 00 00 00 00 00 00 00 00 00 00
> > > > +*
> > > > [phone]
> > > > -- 
> > > > 2.35.1
> > > > 
> > > 
>

Patch

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.
+#
+. ./common/preamble
+_begin_fstest quick log punch
+
+_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.
+_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
+
+# 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
+
+_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
+*
[phone] 00 00 00 00 00 00 00 00 00 00 00 00
+*
[phone]