[PATCHSET 0/2] fstests: new tests for kernel 5.19

[PATCHSET 0/2] fstests: new tests for kernel 5.19

[Darrick J. Wong]

Hi all,

Add new tests for xattr bugfixes merged during 5.19.  Specifically, we
add a new test to encode the desired behavior of the metadata verifiers,
the xattr code, and the fsck tools when the xattr leaf block header
count being zero; and fix a problem in an existing xattr test.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=xfs-merge-5.19

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=xfs-merge-5.19
---
 tests/xfs/288 |   32 ++++++--------
 tests/xfs/845 |  131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 144 insertions(+), 19 deletions(-)
 create mode 100755 tests/xfs/845

[PATCH 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Make sure that the kernel can handle empty xattr leaf blocks properly,
since we've screwed this up enough times.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/845 |  131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 131 insertions(+)
 create mode 100755 tests/xfs/845


diff --git a/tests/xfs/845 b/tests/xfs/845
new file mode 100755
index 00000000..4a846e57
--- /dev/null
+++ b/tests/xfs/845
@@ -0,0 +1,131 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test 845
+#
+# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
+# blocks can appear in files as a result of system crashes in the middle of
+# xattr operations, which means that we /must/ handle them gracefully.
+# Check that read and write verifiers won't trip, that the get/list/setxattr
+# operations don't stumble over them, and that xfs_repair will offer to remove
+# the entire xattr fork if the root xattr leaf block is empty.
+#
+# Regression test for kernel commit:
+#
+# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
+#
+. ./common/preamble
+_begin_fstest auto quick attr
+
+# Override the default cleanup function.
+# _cleanup()
+# {
+# 	cd /
+# 	rm -r -f $tmp.*
+# }
+
+# Import common functions.
+. ./common/filter
+. ./common/attr
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs
+_require_scratch
+_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
+
+_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
+cat $tmp.mkfs >> $seqres.full
+source $tmp.mkfs
+_scratch_mount
+
+$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
+$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
+
+smallfile_md5=$(md5sum < $SCRATCH_MNT/smallfile)
+largefile_md5=$(md5sum < $SCRATCH_MNT/largefile)
+
+# Try to force the creation of a single leaf block in each of three files.
+# The first one gets a local attr, the second a remote attr, and the third
+# is left for scrub and repair to find.
+touch $SCRATCH_MNT/e0
+touch $SCRATCH_MNT/e1
+touch $SCRATCH_MNT/e2
+
+$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
+$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
+$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
+
+e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
+e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
+e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
+
+_scratch_unmount
+
+# We used to think that it wasn't possible for empty xattr leaf blocks to
+# exist, but it turns out that setting a large xattr on a file that has no
+# xattrs can race with a log flush and crash, which results in an empty
+# leaf block being logged and recovered.  This is rather hard to trip, so we
+# use xfs_db to turn a regular leaf block into an empty one.
+make_empty_leaf() {
+	local inum="$1"
+
+	echo "editing inode $inum" >> $seqres.full
+
+	magic=$(_scratch_xfs_get_metadata_field hdr.info.magic "inode $inum" "ablock 0")
+	if [ "$magic" = "0xfbee" ]; then
+		_notrun "V4 filesystems deprecated"
+		return 1
+	fi
+
+	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
+	if [ "$magic" != "0x3bee" ]; then
+		echo "inode $inum ablock 0 is not a leaf block?"
+		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
+		return 1
+	fi
+
+	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
+
+	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
+		-c "write -d hdr.count 0" \
+		-c "write -d hdr.usedbytes 0" \
+		-c "write -d hdr.firstused $dbsize" \
+		-c "write -d hdr.freemap[0].size $((dbsize - base))" \
+		-c print >> $seqres.full
+}
+
+make_empty_leaf $e0_ino || exit
+make_empty_leaf $e1_ino
+make_empty_leaf $e2_ino
+
+_scratch_mount
+
+# Check that listxattr/getxattr/removexattr do nothing.
+$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
+$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
+$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
+
+# Add a small attr to e0
+$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
+$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
+small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | md5sum)"
+test "$small_md5" = "$smallfile_md5" || \
+	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
+
+# Add a large attr to e1
+$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
+$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
+large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | md5sum)"
+test "$large_md5" = "$largefile_md5" || \
+	echo "largefile $largefile_md5 does not match large attr $large_md5"
+
+
+# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
+# empty leaf blocks incorrectly too.
+
+# success, all done
+status=0
+exit

Re: [PATCH 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Zorro Lang]

On Tue, Jul 05, 2022 at 03:02:28PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Make sure that the kernel can handle empty xattr leaf blocks properly,
> since we've screwed this up enough times.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/xfs/845 |  131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 131 insertions(+)
>  create mode 100755 tests/xfs/845
> 
> 
> diff --git a/tests/xfs/845 b/tests/xfs/845
> new file mode 100755
> index 00000000..4a846e57
> --- /dev/null
> +++ b/tests/xfs/845
> @@ -0,0 +1,131 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test 845
> +#
> +# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
> +# blocks can appear in files as a result of system crashes in the middle of
> +# xattr operations, which means that we /must/ handle them gracefully.
> +# Check that read and write verifiers won't trip, that the get/list/setxattr
> +# operations don't stumble over them, and that xfs_repair will offer to remove
> +# the entire xattr fork if the root xattr leaf block is empty.
> +#
> +# Regression test for kernel commit:
> +#
> +# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
> +#
> +. ./common/preamble
> +_begin_fstest auto quick attr
> +
> +# Override the default cleanup function.
> +# _cleanup()
> +# {
> +# 	cd /
> +# 	rm -r -f $tmp.*
> +# }

Remove this part directly?

> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/attr
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
      ^^
      This comment is useless

> +_supported_fs xfs
> +_require_scratch
> +_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
> +
> +_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
> +cat $tmp.mkfs >> $seqres.full
> +source $tmp.mkfs
> +_scratch_mount
> +
> +$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
> +$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
> +
> +smallfile_md5=$(md5sum < $SCRATCH_MNT/smallfile)
> +largefile_md5=$(md5sum < $SCRATCH_MNT/largefile)

Hmm... is the tail '-' (printed by md5sum) as you wish? How about use the
_md5_checksum() in common/rc ? (Same as below small_md5 and large_md5 part)

> +
> +# Try to force the creation of a single leaf block in each of three files.
> +# The first one gets a local attr, the second a remote attr, and the third
> +# is left for scrub and repair to find.
> +touch $SCRATCH_MNT/e0
> +touch $SCRATCH_MNT/e1
> +touch $SCRATCH_MNT/e2
> +
> +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
> +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
> +$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
> +
> +e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
> +e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
> +e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
> +
> +_scratch_unmount
> +
> +# We used to think that it wasn't possible for empty xattr leaf blocks to
> +# exist, but it turns out that setting a large xattr on a file that has no
> +# xattrs can race with a log flush and crash, which results in an empty
> +# leaf block being logged and recovered.  This is rather hard to trip, so we
> +# use xfs_db to turn a regular leaf block into an empty one.
> +make_empty_leaf() {
> +	local inum="$1"
> +
> +	echo "editing inode $inum" >> $seqres.full
> +
> +	magic=$(_scratch_xfs_get_metadata_field hdr.info.magic "inode $inum" "ablock 0")
> +	if [ "$magic" = "0xfbee" ]; then
> +		_notrun "V4 filesystems deprecated"

Can _require_scratch_xfs_crc (at beginning) help that? Or is there a way to
get a v4 leaf block when crc isn't enabled?

> +		return 1

I think _notrun will exit directly, this "return 1" never be run.

> +	fi
> +
> +	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
> +	if [ "$magic" != "0x3bee" ]; then
> +		echo "inode $inum ablock 0 is not a leaf block?"
> +		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
> +		return 1
> +	fi
> +
> +	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
> +
> +	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
> +		-c "write -d hdr.count 0" \
> +		-c "write -d hdr.usedbytes 0" \
> +		-c "write -d hdr.firstused $dbsize" \
> +		-c "write -d hdr.freemap[0].size $((dbsize - base))" \

Nice trick:) Do we expect there's not corruption (can be found by xfs_repair)
at here? Or xfs_repair should blame the empty leaf block?

> +		-c print >> $seqres.full
> +}
> +
> +make_empty_leaf $e0_ino || exit

How about call _fail() directly in make_empty_leaf, if [ "$magic" != "0x3bee" ]

> +make_empty_leaf $e1_ino
> +make_empty_leaf $e2_ino
> +
> +_scratch_mount
> +
> +# Check that listxattr/getxattr/removexattr do nothing.
> +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> +$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> +$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> +
> +# Add a small attr to e0
> +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
> +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
> +small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | md5sum)"
> +test "$small_md5" = "$smallfile_md5" || \
> +	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
> +
> +# Add a large attr to e1
> +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
> +$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
> +large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | md5sum)"
> +test "$large_md5" = "$largefile_md5" || \
> +	echo "largefile $largefile_md5 does not match large attr $large_md5"
> +
> +
> +# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
> +# empty leaf blocks incorrectly too.

So we expect there's not corruption at here, and the empty leaf block can
be used properly?

Thanks,
Zorro

> +
> +# success, all done
> +status=0
> +exit
> 

Re: [PATCH 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Darrick J. Wong]

On Thu, Jul 07, 2022 at 08:06:39PM +0800, Zorro Lang wrote:
> On Tue, Jul 05, 2022 at 03:02:28PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Make sure that the kernel can handle empty xattr leaf blocks properly,
> > since we've screwed this up enough times.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  tests/xfs/845 |  131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 131 insertions(+)
> >  create mode 100755 tests/xfs/845
> > 
> > 
> > diff --git a/tests/xfs/845 b/tests/xfs/845
> > new file mode 100755
> > index 00000000..4a846e57
> > --- /dev/null
> > +++ b/tests/xfs/845
> > @@ -0,0 +1,131 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > +#
> > +# FS QA Test 845
> > +#
> > +# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
> > +# blocks can appear in files as a result of system crashes in the middle of
> > +# xattr operations, which means that we /must/ handle them gracefully.
> > +# Check that read and write verifiers won't trip, that the get/list/setxattr
> > +# operations don't stumble over them, and that xfs_repair will offer to remove
> > +# the entire xattr fork if the root xattr leaf block is empty.
> > +#
> > +# Regression test for kernel commit:
> > +#
> > +# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick attr
> > +
> > +# Override the default cleanup function.
> > +# _cleanup()
> > +# {
> > +# 	cd /
> > +# 	rm -r -f $tmp.*
> > +# }
> 
> Remove this part directly?

Ok.

> > +
> > +# Import common functions.
> > +. ./common/filter
> > +. ./common/attr
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
>       ^^
>       This comment is useless

Oops, will delete.

> > +_supported_fs xfs
> > +_require_scratch
> > +_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
> > +
> > +_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
> > +cat $tmp.mkfs >> $seqres.full
> > +source $tmp.mkfs
> > +_scratch_mount
> > +
> > +$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
> > +$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
> > +
> > +smallfile_md5=$(md5sum < $SCRATCH_MNT/smallfile)
> > +largefile_md5=$(md5sum < $SCRATCH_MNT/largefile)
> 
> Hmm... is the tail '-' (printed by md5sum) as you wish? How about use the
> _md5_checksum() in common/rc ? (Same as below small_md5 and large_md5 part)

Ooh, nice suggestion.  I"ll convert the test.

> > +
> > +# Try to force the creation of a single leaf block in each of three files.
> > +# The first one gets a local attr, the second a remote attr, and the third
> > +# is left for scrub and repair to find.
> > +touch $SCRATCH_MNT/e0
> > +touch $SCRATCH_MNT/e1
> > +touch $SCRATCH_MNT/e2
> > +
> > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
> > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
> > +$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
> > +
> > +e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
> > +e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
> > +e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
> > +
> > +_scratch_unmount
> > +
> > +# We used to think that it wasn't possible for empty xattr leaf blocks to
> > +# exist, but it turns out that setting a large xattr on a file that has no
> > +# xattrs can race with a log flush and crash, which results in an empty
> > +# leaf block being logged and recovered.  This is rather hard to trip, so we
> > +# use xfs_db to turn a regular leaf block into an empty one.
> > +make_empty_leaf() {
> > +	local inum="$1"
> > +
> > +	echo "editing inode $inum" >> $seqres.full
> > +
> > +	magic=$(_scratch_xfs_get_metadata_field hdr.info.magic "inode $inum" "ablock 0")
> > +	if [ "$magic" = "0xfbee" ]; then
> > +		_notrun "V4 filesystems deprecated"
> 
> Can _require_scratch_xfs_crc (at beginning) help that? Or is there a way to
> get a v4 leaf block when crc isn't enabled?

Nope, that was an oversight on my part.  I'll add
_require_scratch_xfs_crc to the start of the test.

> > +		return 1
> 
> I think _notrun will exit directly, this "return 1" never be run.

Yep.

> > +	fi
> > +
> > +	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
> > +	if [ "$magic" != "0x3bee" ]; then
> > +		echo "inode $inum ablock 0 is not a leaf block?"
> > +		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
> > +		return 1
> > +	fi
> > +
> > +	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
> > +
> > +	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
> > +		-c "write -d hdr.count 0" \
> > +		-c "write -d hdr.usedbytes 0" \
> > +		-c "write -d hdr.firstused $dbsize" \
> > +		-c "write -d hdr.freemap[0].size $((dbsize - base))" \
> 
> Nice trick:) Do we expect there's not corruption (can be found by xfs_repair)
> at here?

Right.  We're setting up the empty leaf block 0 to check that the kernel
and fsck tools do /not/ flag this as corruption.

> Or xfs_repair should blame the empty leaf block?

...though repair run in not-dryrun mode will free the empty leaf block 0
if it finds one.

> > +		-c print >> $seqres.full
> > +}
> > +
> > +make_empty_leaf $e0_ino || exit
> 
> How about call _fail() directly in make_empty_leaf, if [ "$magic" != "0x3bee" ]

_require_scratch_xfs_crc should take care of that.

> > +make_empty_leaf $e1_ino
> > +make_empty_leaf $e2_ino
> > +
> > +_scratch_mount
> > +
> > +# Check that listxattr/getxattr/removexattr do nothing.
> > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > +$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > +$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > +
> > +# Add a small attr to e0
> > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
> > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
> > +small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | md5sum)"
> > +test "$small_md5" = "$smallfile_md5" || \
> > +	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
> > +
> > +# Add a large attr to e1
> > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
> > +$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
> > +large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | md5sum)"
> > +test "$large_md5" = "$largefile_md5" || \
> > +	echo "largefile $largefile_md5 does not match large attr $large_md5"
> > +
> > +
> > +# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
> > +# empty leaf blocks incorrectly too.
> 
> So we expect there's not corruption at here, and the empty leaf block can
> be used properly?

Correct.

--D

> Thanks,
> Zorro
> 
> > +
> > +# success, all done
> > +status=0
> > +exit
> > 
> 

Re: [PATCH 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Zorro Lang]

On Thu, Jul 07, 2022 at 11:22:14AM -0700, Darrick J. Wong wrote:
> On Thu, Jul 07, 2022 at 08:06:39PM +0800, Zorro Lang wrote:
> > On Tue, Jul 05, 2022 at 03:02:28PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Make sure that the kernel can handle empty xattr leaf blocks properly,
> > > since we've screwed this up enough times.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  tests/xfs/845 |  131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 131 insertions(+)
> > >  create mode 100755 tests/xfs/845
> > > 
> > > 
> > > diff --git a/tests/xfs/845 b/tests/xfs/845
> > > new file mode 100755
> > > index 00000000..4a846e57
> > > --- /dev/null
> > > +++ b/tests/xfs/845
> > > @@ -0,0 +1,131 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > > +#
> > > +# FS QA Test 845
> > > +#
> > > +# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
> > > +# blocks can appear in files as a result of system crashes in the middle of
> > > +# xattr operations, which means that we /must/ handle them gracefully.
> > > +# Check that read and write verifiers won't trip, that the get/list/setxattr
> > > +# operations don't stumble over them, and that xfs_repair will offer to remove
> > > +# the entire xattr fork if the root xattr leaf block is empty.
> > > +#
> > > +# Regression test for kernel commit:
> > > +#
> > > +# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick attr
> > > +
> > > +# Override the default cleanup function.
> > > +# _cleanup()
> > > +# {
> > > +# 	cd /
> > > +# 	rm -r -f $tmp.*
> > > +# }
> > 
> > Remove this part directly?
> 
> Ok.
> 
> > > +
> > > +# Import common functions.
> > > +. ./common/filter
> > > +. ./common/attr
> > > +
> > > +# real QA test starts here
> > > +
> > > +# Modify as appropriate.
> >       ^^
> >       This comment is useless
> 
> Oops, will delete.
> 
> > > +_supported_fs xfs
> > > +_require_scratch
> > > +_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
> > > +
> > > +_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
> > > +cat $tmp.mkfs >> $seqres.full
> > > +source $tmp.mkfs
> > > +_scratch_mount
> > > +
> > > +$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
> > > +$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
> > > +
> > > +smallfile_md5=$(md5sum < $SCRATCH_MNT/smallfile)
> > > +largefile_md5=$(md5sum < $SCRATCH_MNT/largefile)
> > 
> > Hmm... is the tail '-' (printed by md5sum) as you wish? How about use the
> > _md5_checksum() in common/rc ? (Same as below small_md5 and large_md5 part)
> 
> Ooh, nice suggestion.  I"ll convert the test.
> 
> > > +
> > > +# Try to force the creation of a single leaf block in each of three files.
> > > +# The first one gets a local attr, the second a remote attr, and the third
> > > +# is left for scrub and repair to find.
> > > +touch $SCRATCH_MNT/e0
> > > +touch $SCRATCH_MNT/e1
> > > +touch $SCRATCH_MNT/e2
> > > +
> > > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
> > > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
> > > +$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
> > > +
> > > +e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
> > > +e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
> > > +e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
> > > +
> > > +_scratch_unmount
> > > +
> > > +# We used to think that it wasn't possible for empty xattr leaf blocks to
> > > +# exist, but it turns out that setting a large xattr on a file that has no
> > > +# xattrs can race with a log flush and crash, which results in an empty
> > > +# leaf block being logged and recovered.  This is rather hard to trip, so we
> > > +# use xfs_db to turn a regular leaf block into an empty one.
> > > +make_empty_leaf() {
> > > +	local inum="$1"
> > > +
> > > +	echo "editing inode $inum" >> $seqres.full
> > > +
> > > +	magic=$(_scratch_xfs_get_metadata_field hdr.info.magic "inode $inum" "ablock 0")
> > > +	if [ "$magic" = "0xfbee" ]; then
> > > +		_notrun "V4 filesystems deprecated"
> > 
> > Can _require_scratch_xfs_crc (at beginning) help that? Or is there a way to
> > get a v4 leaf block when crc isn't enabled?
> 
> Nope, that was an oversight on my part.  I'll add
> _require_scratch_xfs_crc to the start of the test.
> 
> > > +		return 1
> > 
> > I think _notrun will exit directly, this "return 1" never be run.
> 
> Yep.
> 
> > > +	fi
> > > +
> > > +	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
> > > +	if [ "$magic" != "0x3bee" ]; then
> > > +		echo "inode $inum ablock 0 is not a leaf block?"
> > > +		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
> > > +		return 1
> > > +	fi
> > > +
> > > +	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
> > > +
> > > +	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
> > > +		-c "write -d hdr.count 0" \
> > > +		-c "write -d hdr.usedbytes 0" \
> > > +		-c "write -d hdr.firstused $dbsize" \
> > > +		-c "write -d hdr.freemap[0].size $((dbsize - base))" \
> > 
> > Nice trick:) Do we expect there's not corruption (can be found by xfs_repair)
> > at here?
> 
> Right.  We're setting up the empty leaf block 0 to check that the kernel
> and fsck tools do /not/ flag this as corruption.
> 
> > Or xfs_repair should blame the empty leaf block?
> 
> ...though repair run in not-dryrun mode will free the empty leaf block 0
> if it finds one.
> 
> > > +		-c print >> $seqres.full
> > > +}
> > > +
> > > +make_empty_leaf $e0_ino || exit
> > 
> > How about call _fail() directly in make_empty_leaf, if [ "$magic" != "0x3bee" ]
> 
> _require_scratch_xfs_crc should take care of that.

Hmm... I think _require_scratch_xfs_crc will help this:

  magic=$(_scratch_xfs_get_metadata_field hdr.info.magic "inode $inum" "ablock 0")
  if [ "$magic" = "0xfbee" ]; then

but can't help this part:

 magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
 if [ "$magic" != "0x3bee" ]; then

due to ablock 0 might be 0x3ebe (XFS_DA3_NODE_MAGIC) besides 0x3bee, although
it looks nearly not impossible in this test, as you only create one attr equal
to inode size :) So if we make sure it never be a node block, it's fine to
remove this judgement, or you can keep it for sure.

Thanks,
Zorro

> 
> > > +make_empty_leaf $e1_ino
> > > +make_empty_leaf $e2_ino
> > > +
> > > +_scratch_mount
> > > +
> > > +# Check that listxattr/getxattr/removexattr do nothing.
> > > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > > +$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > > +$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > > +
> > > +# Add a small attr to e0
> > > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
> > > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
> > > +small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | md5sum)"
> > > +test "$small_md5" = "$smallfile_md5" || \
> > > +	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
> > > +
> > > +# Add a large attr to e1
> > > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
> > > +$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
> > > +large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | md5sum)"
> > > +test "$large_md5" = "$largefile_md5" || \
> > > +	echo "largefile $largefile_md5 does not match large attr $large_md5"
> > > +
> > > +
> > > +# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
> > > +# empty leaf blocks incorrectly too.
> > 
> > So we expect there's not corruption at here, and the empty leaf block can
> > be used properly?
> 
> Correct.
> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > > +
> > > +# success, all done
> > > +status=0
> > > +exit
> > > 
> > 
> 

Re: [PATCH 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Darrick J. Wong]

On Fri, Jul 08, 2022 at 08:49:16AM +0800, Zorro Lang wrote:
> On Thu, Jul 07, 2022 at 11:22:14AM -0700, Darrick J. Wong wrote:
> > On Thu, Jul 07, 2022 at 08:06:39PM +0800, Zorro Lang wrote:
> > > On Tue, Jul 05, 2022 at 03:02:28PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Make sure that the kernel can handle empty xattr leaf blocks properly,
> > > > since we've screwed this up enough times.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  tests/xfs/845 |  131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 131 insertions(+)
> > > >  create mode 100755 tests/xfs/845
> > > > 
> > > > 
> > > > diff --git a/tests/xfs/845 b/tests/xfs/845
> > > > new file mode 100755
> > > > index 00000000..4a846e57
> > > > --- /dev/null
> > > > +++ b/tests/xfs/845
> > > > @@ -0,0 +1,131 @@
> > > > +#! /bin/bash
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > > > +#
> > > > +# FS QA Test 845
> > > > +#
> > > > +# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
> > > > +# blocks can appear in files as a result of system crashes in the middle of
> > > > +# xattr operations, which means that we /must/ handle them gracefully.
> > > > +# Check that read and write verifiers won't trip, that the get/list/setxattr
> > > > +# operations don't stumble over them, and that xfs_repair will offer to remove
> > > > +# the entire xattr fork if the root xattr leaf block is empty.
> > > > +#
> > > > +# Regression test for kernel commit:
> > > > +#
> > > > +# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
> > > > +#
> > > > +. ./common/preamble
> > > > +_begin_fstest auto quick attr
> > > > +
> > > > +# Override the default cleanup function.
> > > > +# _cleanup()
> > > > +# {
> > > > +# 	cd /
> > > > +# 	rm -r -f $tmp.*
> > > > +# }
> > > 
> > > Remove this part directly?
> > 
> > Ok.
> > 
> > > > +
> > > > +# Import common functions.
> > > > +. ./common/filter
> > > > +. ./common/attr
> > > > +
> > > > +# real QA test starts here
> > > > +
> > > > +# Modify as appropriate.
> > >       ^^
> > >       This comment is useless
> > 
> > Oops, will delete.
> > 
> > > > +_supported_fs xfs
> > > > +_require_scratch
> > > > +_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
> > > > +
> > > > +_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
> > > > +cat $tmp.mkfs >> $seqres.full
> > > > +source $tmp.mkfs
> > > > +_scratch_mount
> > > > +
> > > > +$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
> > > > +$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
> > > > +
> > > > +smallfile_md5=$(md5sum < $SCRATCH_MNT/smallfile)
> > > > +largefile_md5=$(md5sum < $SCRATCH_MNT/largefile)
> > > 
> > > Hmm... is the tail '-' (printed by md5sum) as you wish? How about use the
> > > _md5_checksum() in common/rc ? (Same as below small_md5 and large_md5 part)
> > 
> > Ooh, nice suggestion.  I"ll convert the test.
> > 
> > > > +
> > > > +# Try to force the creation of a single leaf block in each of three files.
> > > > +# The first one gets a local attr, the second a remote attr, and the third
> > > > +# is left for scrub and repair to find.
> > > > +touch $SCRATCH_MNT/e0
> > > > +touch $SCRATCH_MNT/e1
> > > > +touch $SCRATCH_MNT/e2
> > > > +
> > > > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
> > > > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
> > > > +$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
> > > > +
> > > > +e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
> > > > +e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
> > > > +e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
> > > > +
> > > > +_scratch_unmount
> > > > +
> > > > +# We used to think that it wasn't possible for empty xattr leaf blocks to
> > > > +# exist, but it turns out that setting a large xattr on a file that has no
> > > > +# xattrs can race with a log flush and crash, which results in an empty
> > > > +# leaf block being logged and recovered.  This is rather hard to trip, so we
> > > > +# use xfs_db to turn a regular leaf block into an empty one.
> > > > +make_empty_leaf() {
> > > > +	local inum="$1"
> > > > +
> > > > +	echo "editing inode $inum" >> $seqres.full
> > > > +
> > > > +	magic=$(_scratch_xfs_get_metadata_field hdr.info.magic "inode $inum" "ablock 0")
> > > > +	if [ "$magic" = "0xfbee" ]; then
> > > > +		_notrun "V4 filesystems deprecated"
> > > 
> > > Can _require_scratch_xfs_crc (at beginning) help that? Or is there a way to
> > > get a v4 leaf block when crc isn't enabled?
> > 
> > Nope, that was an oversight on my part.  I'll add
> > _require_scratch_xfs_crc to the start of the test.
> > 
> > > > +		return 1
> > > 
> > > I think _notrun will exit directly, this "return 1" never be run.
> > 
> > Yep.
> > 
> > > > +	fi
> > > > +
> > > > +	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
> > > > +	if [ "$magic" != "0x3bee" ]; then
> > > > +		echo "inode $inum ablock 0 is not a leaf block?"
> > > > +		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
> > > > +		return 1
> > > > +	fi
> > > > +
> > > > +	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
> > > > +
> > > > +	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
> > > > +		-c "write -d hdr.count 0" \
> > > > +		-c "write -d hdr.usedbytes 0" \
> > > > +		-c "write -d hdr.firstused $dbsize" \
> > > > +		-c "write -d hdr.freemap[0].size $((dbsize - base))" \
> > > 
> > > Nice trick:) Do we expect there's not corruption (can be found by xfs_repair)
> > > at here?
> > 
> > Right.  We're setting up the empty leaf block 0 to check that the kernel
> > and fsck tools do /not/ flag this as corruption.
> > 
> > > Or xfs_repair should blame the empty leaf block?
> > 
> > ...though repair run in not-dryrun mode will free the empty leaf block 0
> > if it finds one.
> > 
> > > > +		-c print >> $seqres.full
> > > > +}
> > > > +
> > > > +make_empty_leaf $e0_ino || exit
> > > 
> > > How about call _fail() directly in make_empty_leaf, if [ "$magic" != "0x3bee" ]
> > 
> > _require_scratch_xfs_crc should take care of that.
> 
> Hmm... I think _require_scratch_xfs_crc will help this:
> 
>   magic=$(_scratch_xfs_get_metadata_field hdr.info.magic "inode $inum" "ablock 0")
>   if [ "$magic" = "0xfbee" ]; then
> 
> but can't help this part:
> 
>  magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
>  if [ "$magic" != "0x3bee" ]; then
> 
> due to ablock 0 might be 0x3ebe (XFS_DA3_NODE_MAGIC) besides 0x3bee, although
> it looks nearly not impossible in this test, as you only create one attr equal
> to inode size :) So if we make sure it never be a node block, it's fine to
> remove this judgement, or you can keep it for sure.

That's actually a good point, I should print out something if the magic
number isn't 0x3bee, because that means a test precondition was not
satisfied.

(Granted, anything other than a leaf block and the _set commands will
spray errors, but we could be explicit about not finding the leaf block
we expected to see.)

--D

> Thanks,
> Zorro
> 
> > 
> > > > +make_empty_leaf $e1_ino
> > > > +make_empty_leaf $e2_ino
> > > > +
> > > > +_scratch_mount
> > > > +
> > > > +# Check that listxattr/getxattr/removexattr do nothing.
> > > > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > > > +$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > > > +$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > > > +
> > > > +# Add a small attr to e0
> > > > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
> > > > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
> > > > +small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | md5sum)"
> > > > +test "$small_md5" = "$smallfile_md5" || \
> > > > +	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
> > > > +
> > > > +# Add a large attr to e1
> > > > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
> > > > +$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
> > > > +large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | md5sum)"
> > > > +test "$large_md5" = "$largefile_md5" || \
> > > > +	echo "largefile $largefile_md5 does not match large attr $large_md5"
> > > > +
> > > > +
> > > > +# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
> > > > +# empty leaf blocks incorrectly too.
> > > 
> > > So we expect there's not corruption at here, and the empty leaf block can
> > > be used properly?
> > 
> > Correct.
> > 
> > --D
> > 
> > > Thanks,
> > > Zorro
> > > 
> > > > +
> > > > +# success, all done
> > > > +status=0
> > > > +exit
> > > > 
> > > 
> > 
> 

[PATCH v1.1 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Make sure that the kernel can handle empty xattr leaf blocks properly,
since we've screwed this up enough times.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v1.1: adopt maintainer's refactoring suggestions, skip v4 filesystems
from the start, and check that we really get an attr leaf block.
---
 tests/xfs/845 |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)
 create mode 100755 tests/xfs/845

diff --git a/tests/xfs/845 b/tests/xfs/845
new file mode 100755
index 00000000..0b7f4bff
--- /dev/null
+++ b/tests/xfs/845
@@ -0,0 +1,123 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test 845
+#
+# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
+# blocks can appear in files as a result of system crashes in the middle of
+# xattr operations, which means that we /must/ handle them gracefully.
+# Check that read and write verifiers won't trip, that the get/list/setxattr
+# operations don't stumble over them, and that xfs_repair will offer to remove
+# the entire xattr fork if the root xattr leaf block is empty.
+#
+# Regression test for kernel commit:
+#
+# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
+#
+. ./common/preamble
+_begin_fstest auto quick attr
+
+# Import common functions.
+. ./common/filter
+. ./common/attr
+
+# real QA test starts here
+
+_supported_fs xfs
+_require_scratch
+_require_scratch_xfs_crc # V4 is deprecated
+_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
+
+_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
+cat $tmp.mkfs >> $seqres.full
+source $tmp.mkfs
+_scratch_mount
+
+$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
+$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
+
+smallfile_md5=$(_md5_checksum $SCRATCH_MNT/smallfile)
+largefile_md5=$(_md5_checksum $SCRATCH_MNT/largefile)
+
+# Try to force the creation of a single leaf block in each of three files.
+# The first one gets a local attr, the second a remote attr, and the third
+# is left for scrub and repair to find.
+touch $SCRATCH_MNT/e0
+touch $SCRATCH_MNT/e1
+touch $SCRATCH_MNT/e2
+
+$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
+$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
+$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
+
+e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
+e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
+e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
+
+_scratch_unmount
+
+# We used to think that it wasn't possible for empty xattr leaf blocks to
+# exist, but it turns out that setting a large xattr on a file that has no
+# xattrs can race with a log flush and crash, which results in an empty
+# leaf block being logged and recovered.  This is rather hard to trip, so we
+# use xfs_db to turn a regular leaf block into an empty one.
+make_empty_leaf() {
+	local inum="$1"
+
+	echo "editing inode $inum" >> $seqres.full
+
+	magic=$(_scratch_xfs_get_metadata_field hdr.info.magic "inode $inum" "ablock 0")
+	if [ "$magic" != "0x3bee" ]; then
+		echo "attr block 0 expected magic 0x3bee, got $magic??"
+	fi
+
+	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
+	if [ "$magic" != "0x3bee" ]; then
+		echo "inode $inum ablock 0 is not a leaf block?"
+		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
+		return 1
+	fi
+
+	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
+
+	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
+		-c "write -d hdr.count 0" \
+		-c "write -d hdr.usedbytes 0" \
+		-c "write -d hdr.firstused $dbsize" \
+		-c "write -d hdr.freemap[0].size $((dbsize - base))" \
+		-c print >> $seqres.full
+}
+
+make_empty_leaf $e0_ino
+make_empty_leaf $e1_ino
+make_empty_leaf $e2_ino
+
+_scratch_mount
+
+# Check that listxattr/getxattr/removexattr do nothing.
+$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
+$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
+$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
+
+# Add a small attr to e0
+$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
+$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
+small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | _md5_checksum)"
+test "$small_md5" = "$smallfile_md5" || \
+	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
+
+# Add a large attr to e1
+$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
+$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
+large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | _md5_checksum)"
+test "$large_md5" = "$largefile_md5" || \
+	echo "largefile $largefile_md5 does not match large attr $large_md5"
+
+
+# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
+# empty leaf blocks incorrectly too.
+
+# success, all done
+status=0
+exit

Re: [PATCH v1.1 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Zorro Lang]

On Fri, Jul 08, 2022 at 08:32:01AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Make sure that the kernel can handle empty xattr leaf blocks properly,
> since we've screwed this up enough times.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v1.1: adopt maintainer's refactoring suggestions, skip v4 filesystems
> from the start, and check that we really get an attr leaf block.
> ---
>  tests/xfs/845 |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 123 insertions(+)
>  create mode 100755 tests/xfs/845
> 
> diff --git a/tests/xfs/845 b/tests/xfs/845
> new file mode 100755
> index 00000000..0b7f4bff
> --- /dev/null
> +++ b/tests/xfs/845
> @@ -0,0 +1,123 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test 845
> +#
> +# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
> +# blocks can appear in files as a result of system crashes in the middle of
> +# xattr operations, which means that we /must/ handle them gracefully.
> +# Check that read and write verifiers won't trip, that the get/list/setxattr
> +# operations don't stumble over them, and that xfs_repair will offer to remove
> +# the entire xattr fork if the root xattr leaf block is empty.
> +#
> +# Regression test for kernel commit:
> +#
> +# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
> +#
> +. ./common/preamble
> +_begin_fstest auto quick attr
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/attr
> +
> +# real QA test starts here
> +
> +_supported_fs xfs
> +_require_scratch
> +_require_scratch_xfs_crc # V4 is deprecated
> +_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
> +
> +_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
> +cat $tmp.mkfs >> $seqres.full
> +source $tmp.mkfs
> +_scratch_mount
> +
> +$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
> +$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
> +
> +smallfile_md5=$(_md5_checksum $SCRATCH_MNT/smallfile)
> +largefile_md5=$(_md5_checksum $SCRATCH_MNT/largefile)
> +
> +# Try to force the creation of a single leaf block in each of three files.
> +# The first one gets a local attr, the second a remote attr, and the third
> +# is left for scrub and repair to find.
> +touch $SCRATCH_MNT/e0
> +touch $SCRATCH_MNT/e1
> +touch $SCRATCH_MNT/e2
> +
> +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
> +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
> +$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
> +
> +e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
> +e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
> +e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
> +
> +_scratch_unmount
> +
> +# We used to think that it wasn't possible for empty xattr leaf blocks to
> +# exist, but it turns out that setting a large xattr on a file that has no
> +# xattrs can race with a log flush and crash, which results in an empty
> +# leaf block being logged and recovered.  This is rather hard to trip, so we
> +# use xfs_db to turn a regular leaf block into an empty one.
> +make_empty_leaf() {
> +	local inum="$1"
> +
> +	echo "editing inode $inum" >> $seqres.full
> +
> +	magic=$(_scratch_xfs_get_metadata_field hdr.info.magic "inode $inum" "ablock 0")
> +	if [ "$magic" != "0x3bee" ]; then
> +		echo "attr block 0 expected magic 0x3bee, got $magic??"
> +	fi

Hmm... sorry I'm a little confused, can we get "hdr.info.magic = 0x3bee"
from a V5 xfs ablock 0? If it's V5 XFS, I think the magic should be in
the "info.hdr" field:

  struct xfs_da3_blkinfo {
      struct xfs_da_blkinfo   hdr;  <== magic in it.
      ...
  }

And is it a duplicate checking with below?

> +
> +	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
> +	if [ "$magic" != "0x3bee" ]; then
> +		echo "inode $inum ablock 0 is not a leaf block?"
> +		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
> +		return 1

How about:

  if [ "$magic" != "0x3bee" ]; then
      _scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
      _fail "inode $inum ablock 0 is not a leaf block?"
  fi

> +	fi
> +
> +	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
> +
> +	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
> +		-c "write -d hdr.count 0" \
> +		-c "write -d hdr.usedbytes 0" \
> +		-c "write -d hdr.firstused $dbsize" \
> +		-c "write -d hdr.freemap[0].size $((dbsize - base))" \
> +		-c print >> $seqres.full
> +}
> +
> +make_empty_leaf $e0_ino
> +make_empty_leaf $e1_ino
> +make_empty_leaf $e2_ino
> +
> +_scratch_mount
> +
> +# Check that listxattr/getxattr/removexattr do nothing.
> +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> +$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> +$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> +
> +# Add a small attr to e0
> +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
> +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
> +small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | _md5_checksum)"
> +test "$small_md5" = "$smallfile_md5" || \
> +	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
> +
> +# Add a large attr to e1
> +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
> +$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
> +large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | _md5_checksum)"
> +test "$large_md5" = "$largefile_md5" || \
> +	echo "largefile $largefile_md5 does not match large attr $large_md5"
> +
> +
> +# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
> +# empty leaf blocks incorrectly too.
> +
> +# success, all done
> +status=0
> +exit
> 

Re: [PATCH v1.1 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Darrick J. Wong]

On Sat, Jul 09, 2022 at 12:30:58AM +0800, Zorro Lang wrote:
> On Fri, Jul 08, 2022 at 08:32:01AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Make sure that the kernel can handle empty xattr leaf blocks properly,
> > since we've screwed this up enough times.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > v1.1: adopt maintainer's refactoring suggestions, skip v4 filesystems
> > from the start, and check that we really get an attr leaf block.
> > ---
> >  tests/xfs/845 |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 123 insertions(+)
> >  create mode 100755 tests/xfs/845
> > 
> > diff --git a/tests/xfs/845 b/tests/xfs/845
> > new file mode 100755
> > index 00000000..0b7f4bff
> > --- /dev/null
> > +++ b/tests/xfs/845
> > @@ -0,0 +1,123 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > +#
> > +# FS QA Test 845
> > +#
> > +# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
> > +# blocks can appear in files as a result of system crashes in the middle of
> > +# xattr operations, which means that we /must/ handle them gracefully.
> > +# Check that read and write verifiers won't trip, that the get/list/setxattr
> > +# operations don't stumble over them, and that xfs_repair will offer to remove
> > +# the entire xattr fork if the root xattr leaf block is empty.
> > +#
> > +# Regression test for kernel commit:
> > +#
> > +# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick attr
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +. ./common/attr
> > +
> > +# real QA test starts here
> > +
> > +_supported_fs xfs
> > +_require_scratch
> > +_require_scratch_xfs_crc # V4 is deprecated
> > +_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
> > +
> > +_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
> > +cat $tmp.mkfs >> $seqres.full
> > +source $tmp.mkfs
> > +_scratch_mount
> > +
> > +$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
> > +$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
> > +
> > +smallfile_md5=$(_md5_checksum $SCRATCH_MNT/smallfile)
> > +largefile_md5=$(_md5_checksum $SCRATCH_MNT/largefile)
> > +
> > +# Try to force the creation of a single leaf block in each of three files.
> > +# The first one gets a local attr, the second a remote attr, and the third
> > +# is left for scrub and repair to find.
> > +touch $SCRATCH_MNT/e0
> > +touch $SCRATCH_MNT/e1
> > +touch $SCRATCH_MNT/e2
> > +
> > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
> > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
> > +$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
> > +
> > +e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
> > +e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
> > +e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
> > +
> > +_scratch_unmount
> > +
> > +# We used to think that it wasn't possible for empty xattr leaf blocks to
> > +# exist, but it turns out that setting a large xattr on a file that has no
> > +# xattrs can race with a log flush and crash, which results in an empty
> > +# leaf block being logged and recovered.  This is rather hard to trip, so we
> > +# use xfs_db to turn a regular leaf block into an empty one.
> > +make_empty_leaf() {
> > +	local inum="$1"
> > +
> > +	echo "editing inode $inum" >> $seqres.full
> > +
> > +	magic=$(_scratch_xfs_get_metadata_field hdr.info.magic "inode $inum" "ablock 0")
> > +	if [ "$magic" != "0x3bee" ]; then
> > +		echo "attr block 0 expected magic 0x3bee, got $magic??"
> > +	fi
> 
> Hmm... sorry I'm a little confused, can we get "hdr.info.magic = 0x3bee"
> from a V5 xfs ablock 0? If it's V5 XFS, I think the magic should be in
> the "info.hdr" field:

<sigh> yes, trying to get things done in too short a time.  This isn't
needed at all anymore.

>   struct xfs_da3_blkinfo {
>       struct xfs_da_blkinfo   hdr;  <== magic in it.
>       ...
>   }
> 
> And is it a duplicate checking with below?

Nope, the second check is fine on its own.

> > +
> > +	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
> > +	if [ "$magic" != "0x3bee" ]; then
> > +		echo "inode $inum ablock 0 is not a leaf block?"
> > +		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
> > +		return 1
> 
> How about:
> 
>   if [ "$magic" != "0x3bee" ]; then
>       _scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
>       _fail "inode $inum ablock 0 is not a leaf block?"
>   fi

Ok.

--D

> 
> > +	fi
> > +
> > +	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
> > +
> > +	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
> > +		-c "write -d hdr.count 0" \
> > +		-c "write -d hdr.usedbytes 0" \
> > +		-c "write -d hdr.firstused $dbsize" \
> > +		-c "write -d hdr.freemap[0].size $((dbsize - base))" \
> > +		-c print >> $seqres.full
> > +}
> > +
> > +make_empty_leaf $e0_ino
> > +make_empty_leaf $e1_ino
> > +make_empty_leaf $e2_ino
> > +
> > +_scratch_mount
> > +
> > +# Check that listxattr/getxattr/removexattr do nothing.
> > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > +$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > +$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > +
> > +# Add a small attr to e0
> > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
> > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
> > +small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | _md5_checksum)"
> > +test "$small_md5" = "$smallfile_md5" || \
> > +	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
> > +
> > +# Add a large attr to e1
> > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
> > +$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
> > +large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | _md5_checksum)"
> > +test "$large_md5" = "$largefile_md5" || \
> > +	echo "largefile $largefile_md5 does not match large attr $large_md5"
> > +
> > +
> > +# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
> > +# empty leaf blocks incorrectly too.
> > +
> > +# success, all done
> > +status=0
> > +exit
> > 
> 

Re: [PATCH 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Make sure that the kernel can handle empty xattr leaf blocks properly,
since we've screwed this up enough times.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v1.1: adopt maintainer's refactoring suggestions, skip v4 filesystems
from the start, and check that we really get an attr leaf block.

v1.2: eliminate dead code
---
 tests/xfs/845 |  117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)
 create mode 100755 tests/xfs/845

diff --git a/tests/xfs/845 b/tests/xfs/845
new file mode 100755
index 00000000..c142fba1
--- /dev/null
+++ b/tests/xfs/845
@@ -0,0 +1,117 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test 845
+#
+# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
+# blocks can appear in files as a result of system crashes in the middle of
+# xattr operations, which means that we /must/ handle them gracefully.
+# Check that read and write verifiers won't trip, that the get/list/setxattr
+# operations don't stumble over them, and that xfs_repair will offer to remove
+# the entire xattr fork if the root xattr leaf block is empty.
+#
+# Regression test for kernel commit:
+#
+# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
+#
+. ./common/preamble
+_begin_fstest auto quick attr
+
+# Import common functions.
+. ./common/filter
+. ./common/attr
+
+# real QA test starts here
+
+_supported_fs xfs
+_require_scratch
+_require_scratch_xfs_crc # V4 is deprecated
+_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
+
+_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
+cat $tmp.mkfs >> $seqres.full
+source $tmp.mkfs
+_scratch_mount
+
+$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
+$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
+
+smallfile_md5=$(_md5_checksum $SCRATCH_MNT/smallfile)
+largefile_md5=$(_md5_checksum $SCRATCH_MNT/largefile)
+
+# Try to force the creation of a single leaf block in each of three files.
+# The first one gets a local attr, the second a remote attr, and the third
+# is left for scrub and repair to find.
+touch $SCRATCH_MNT/e0
+touch $SCRATCH_MNT/e1
+touch $SCRATCH_MNT/e2
+
+$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
+$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
+$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
+
+e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
+e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
+e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
+
+_scratch_unmount
+
+# We used to think that it wasn't possible for empty xattr leaf blocks to
+# exist, but it turns out that setting a large xattr on a file that has no
+# xattrs can race with a log flush and crash, which results in an empty
+# leaf block being logged and recovered.  This is rather hard to trip, so we
+# use xfs_db to turn a regular leaf block into an empty one.
+make_empty_leaf() {
+	local inum="$1"
+
+	echo "editing inode $inum" >> $seqres.full
+
+	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
+	if [ "$magic" != "0x3bee" ]; then
+		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
+		_fail "inode $inum ablock 0 is not a leaf block?"
+	fi
+
+	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
+
+	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
+		-c "write -d hdr.count 0" \
+		-c "write -d hdr.usedbytes 0" \
+		-c "write -d hdr.firstused $dbsize" \
+		-c "write -d hdr.freemap[0].size $((dbsize - base))" \
+		-c print >> $seqres.full
+}
+
+make_empty_leaf $e0_ino
+make_empty_leaf $e1_ino
+make_empty_leaf $e2_ino
+
+_scratch_mount
+
+# Check that listxattr/getxattr/removexattr do nothing.
+$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
+$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
+$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
+
+# Add a small attr to e0
+$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
+$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
+small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | _md5_checksum)"
+test "$small_md5" = "$smallfile_md5" || \
+	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
+
+# Add a large attr to e1
+$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
+$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
+large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | _md5_checksum)"
+test "$large_md5" = "$largefile_md5" || \
+	echo "largefile $largefile_md5 does not match large attr $large_md5"
+
+
+# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
+# empty leaf blocks incorrectly too.
+
+# success, all done
+status=0
+exit

Re: [PATCH 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Zorro Lang]

On Fri, Jul 08, 2022 at 10:02:04AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Make sure that the kernel can handle empty xattr leaf blocks properly,
> since we've screwed this up enough times.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

This version looks good to me, thanks for this new case!

Reviewed-by: Zorro Lang <zlang@redhat.com>

> v1.1: adopt maintainer's refactoring suggestions, skip v4 filesystems
> from the start, and check that we really get an attr leaf block.
> 
> v1.2: eliminate dead code
> ---
>  tests/xfs/845 |  117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 117 insertions(+)
>  create mode 100755 tests/xfs/845
> 
> diff --git a/tests/xfs/845 b/tests/xfs/845
> new file mode 100755
> index 00000000..c142fba1
> --- /dev/null
> +++ b/tests/xfs/845
> @@ -0,0 +1,117 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test 845
> +#
> +# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
> +# blocks can appear in files as a result of system crashes in the middle of
> +# xattr operations, which means that we /must/ handle them gracefully.
> +# Check that read and write verifiers won't trip, that the get/list/setxattr
> +# operations don't stumble over them, and that xfs_repair will offer to remove
> +# the entire xattr fork if the root xattr leaf block is empty.
> +#
> +# Regression test for kernel commit:
> +#
> +# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
> +#
> +. ./common/preamble
> +_begin_fstest auto quick attr
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/attr
> +
> +# real QA test starts here
> +
> +_supported_fs xfs
> +_require_scratch
> +_require_scratch_xfs_crc # V4 is deprecated
> +_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
> +
> +_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
> +cat $tmp.mkfs >> $seqres.full
> +source $tmp.mkfs
> +_scratch_mount
> +
> +$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
> +$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
> +
> +smallfile_md5=$(_md5_checksum $SCRATCH_MNT/smallfile)
> +largefile_md5=$(_md5_checksum $SCRATCH_MNT/largefile)
> +
> +# Try to force the creation of a single leaf block in each of three files.
> +# The first one gets a local attr, the second a remote attr, and the third
> +# is left for scrub and repair to find.
> +touch $SCRATCH_MNT/e0
> +touch $SCRATCH_MNT/e1
> +touch $SCRATCH_MNT/e2
> +
> +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
> +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
> +$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
> +
> +e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
> +e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
> +e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
> +
> +_scratch_unmount
> +
> +# We used to think that it wasn't possible for empty xattr leaf blocks to
> +# exist, but it turns out that setting a large xattr on a file that has no
> +# xattrs can race with a log flush and crash, which results in an empty
> +# leaf block being logged and recovered.  This is rather hard to trip, so we
> +# use xfs_db to turn a regular leaf block into an empty one.
> +make_empty_leaf() {
> +	local inum="$1"
> +
> +	echo "editing inode $inum" >> $seqres.full
> +
> +	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
> +	if [ "$magic" != "0x3bee" ]; then
> +		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
> +		_fail "inode $inum ablock 0 is not a leaf block?"
> +	fi
> +
> +	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
> +
> +	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
> +		-c "write -d hdr.count 0" \
> +		-c "write -d hdr.usedbytes 0" \
> +		-c "write -d hdr.firstused $dbsize" \
> +		-c "write -d hdr.freemap[0].size $((dbsize - base))" \
> +		-c print >> $seqres.full
> +}
> +
> +make_empty_leaf $e0_ino
> +make_empty_leaf $e1_ino
> +make_empty_leaf $e2_ino
> +
> +_scratch_mount
> +
> +# Check that listxattr/getxattr/removexattr do nothing.
> +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> +$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> +$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> +
> +# Add a small attr to e0
> +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
> +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
> +small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | _md5_checksum)"
> +test "$small_md5" = "$smallfile_md5" || \
> +	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
> +
> +# Add a large attr to e1
> +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
> +$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
> +large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | _md5_checksum)"
> +test "$large_md5" = "$largefile_md5" || \
> +	echo "largefile $largefile_md5 does not match large attr $large_md5"
> +
> +
> +# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
> +# empty leaf blocks incorrectly too.
> +
> +# success, all done
> +status=0
> +exit
> 

Re: [PATCH 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Zorro Lang]

On Sat, Jul 09, 2022 at 01:08:27AM +0800, Zorro Lang wrote:
> On Fri, Jul 08, 2022 at 10:02:04AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Make sure that the kernel can handle empty xattr leaf blocks properly,
> > since we've screwed this up enough times.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> 
> This version looks good to me, thanks for this new case!
> 
> Reviewed-by: Zorro Lang <zlang@redhat.com>

Errr.. I just noticed that there's not .out file in this patch, and it's not
simple "Silence is golden" I think.

Thanks,
Zorro

> 
> > v1.1: adopt maintainer's refactoring suggestions, skip v4 filesystems
> > from the start, and check that we really get an attr leaf block.
> > 
> > v1.2: eliminate dead code
> > ---
> >  tests/xfs/845 |  117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 117 insertions(+)
> >  create mode 100755 tests/xfs/845
> > 
> > diff --git a/tests/xfs/845 b/tests/xfs/845
> > new file mode 100755
> > index 00000000..c142fba1
> > --- /dev/null
> > +++ b/tests/xfs/845
> > @@ -0,0 +1,117 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > +#
> > +# FS QA Test 845
> > +#
> > +# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
> > +# blocks can appear in files as a result of system crashes in the middle of
> > +# xattr operations, which means that we /must/ handle them gracefully.
> > +# Check that read and write verifiers won't trip, that the get/list/setxattr
> > +# operations don't stumble over them, and that xfs_repair will offer to remove
> > +# the entire xattr fork if the root xattr leaf block is empty.
> > +#
> > +# Regression test for kernel commit:
> > +#
> > +# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick attr
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +. ./common/attr
> > +
> > +# real QA test starts here
> > +
> > +_supported_fs xfs
> > +_require_scratch
> > +_require_scratch_xfs_crc # V4 is deprecated
> > +_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
> > +
> > +_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
> > +cat $tmp.mkfs >> $seqres.full
> > +source $tmp.mkfs
> > +_scratch_mount
> > +
> > +$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
> > +$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
> > +
> > +smallfile_md5=$(_md5_checksum $SCRATCH_MNT/smallfile)
> > +largefile_md5=$(_md5_checksum $SCRATCH_MNT/largefile)
> > +
> > +# Try to force the creation of a single leaf block in each of three files.
> > +# The first one gets a local attr, the second a remote attr, and the third
> > +# is left for scrub and repair to find.
> > +touch $SCRATCH_MNT/e0
> > +touch $SCRATCH_MNT/e1
> > +touch $SCRATCH_MNT/e2
> > +
> > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
> > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
> > +$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
> > +
> > +e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
> > +e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
> > +e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
> > +
> > +_scratch_unmount
> > +
> > +# We used to think that it wasn't possible for empty xattr leaf blocks to
> > +# exist, but it turns out that setting a large xattr on a file that has no
> > +# xattrs can race with a log flush and crash, which results in an empty
> > +# leaf block being logged and recovered.  This is rather hard to trip, so we
> > +# use xfs_db to turn a regular leaf block into an empty one.
> > +make_empty_leaf() {
> > +	local inum="$1"
> > +
> > +	echo "editing inode $inum" >> $seqres.full
> > +
> > +	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
> > +	if [ "$magic" != "0x3bee" ]; then
> > +		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
> > +		_fail "inode $inum ablock 0 is not a leaf block?"
> > +	fi
> > +
> > +	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
> > +
> > +	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
> > +		-c "write -d hdr.count 0" \
> > +		-c "write -d hdr.usedbytes 0" \
> > +		-c "write -d hdr.firstused $dbsize" \
> > +		-c "write -d hdr.freemap[0].size $((dbsize - base))" \
> > +		-c print >> $seqres.full
> > +}
> > +
> > +make_empty_leaf $e0_ino
> > +make_empty_leaf $e1_ino
> > +make_empty_leaf $e2_ino
> > +
> > +_scratch_mount
> > +
> > +# Check that listxattr/getxattr/removexattr do nothing.
> > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > +$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > +$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > +
> > +# Add a small attr to e0
> > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
> > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
> > +small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | _md5_checksum)"
> > +test "$small_md5" = "$smallfile_md5" || \
> > +	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
> > +
> > +# Add a large attr to e1
> > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
> > +$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
> > +large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | _md5_checksum)"
> > +test "$large_md5" = "$largefile_md5" || \
> > +	echo "largefile $largefile_md5 does not match large attr $large_md5"
> > +
> > +
> > +# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
> > +# empty leaf blocks incorrectly too.
> > +
> > +# success, all done
> > +status=0
> > +exit
> > 

Re: [PATCH 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Darrick J. Wong]

On Sat, Jul 09, 2022 at 01:27:20AM +0800, Zorro Lang wrote:
> On Sat, Jul 09, 2022 at 01:08:27AM +0800, Zorro Lang wrote:
> > On Fri, Jul 08, 2022 at 10:02:04AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Make sure that the kernel can handle empty xattr leaf blocks properly,
> > > since we've screwed this up enough times.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > 
> > This version looks good to me, thanks for this new case!
> > 
> > Reviewed-by: Zorro Lang <zlang@redhat.com>
> 
> Errr.. I just noticed that there's not .out file in this patch, and it's not
> simple "Silence is golden" I think.

<sigh> guess who forgot to git add? :(

--D

> Thanks,
> Zorro
> 
> > 
> > > v1.1: adopt maintainer's refactoring suggestions, skip v4 filesystems
> > > from the start, and check that we really get an attr leaf block.
> > > 
> > > v1.2: eliminate dead code
> > > ---
> > >  tests/xfs/845 |  117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 117 insertions(+)
> > >  create mode 100755 tests/xfs/845
> > > 
> > > diff --git a/tests/xfs/845 b/tests/xfs/845
> > > new file mode 100755
> > > index 00000000..c142fba1
> > > --- /dev/null
> > > +++ b/tests/xfs/845
> > > @@ -0,0 +1,117 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > > +#
> > > +# FS QA Test 845
> > > +#
> > > +# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
> > > +# blocks can appear in files as a result of system crashes in the middle of
> > > +# xattr operations, which means that we /must/ handle them gracefully.
> > > +# Check that read and write verifiers won't trip, that the get/list/setxattr
> > > +# operations don't stumble over them, and that xfs_repair will offer to remove
> > > +# the entire xattr fork if the root xattr leaf block is empty.
> > > +#
> > > +# Regression test for kernel commit:
> > > +#
> > > +# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick attr
> > > +
> > > +# Import common functions.
> > > +. ./common/filter
> > > +. ./common/attr
> > > +
> > > +# real QA test starts here
> > > +
> > > +_supported_fs xfs
> > > +_require_scratch
> > > +_require_scratch_xfs_crc # V4 is deprecated
> > > +_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
> > > +
> > > +_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
> > > +cat $tmp.mkfs >> $seqres.full
> > > +source $tmp.mkfs
> > > +_scratch_mount
> > > +
> > > +$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
> > > +$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
> > > +
> > > +smallfile_md5=$(_md5_checksum $SCRATCH_MNT/smallfile)
> > > +largefile_md5=$(_md5_checksum $SCRATCH_MNT/largefile)
> > > +
> > > +# Try to force the creation of a single leaf block in each of three files.
> > > +# The first one gets a local attr, the second a remote attr, and the third
> > > +# is left for scrub and repair to find.
> > > +touch $SCRATCH_MNT/e0
> > > +touch $SCRATCH_MNT/e1
> > > +touch $SCRATCH_MNT/e2
> > > +
> > > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
> > > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
> > > +$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
> > > +
> > > +e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
> > > +e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
> > > +e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
> > > +
> > > +_scratch_unmount
> > > +
> > > +# We used to think that it wasn't possible for empty xattr leaf blocks to
> > > +# exist, but it turns out that setting a large xattr on a file that has no
> > > +# xattrs can race with a log flush and crash, which results in an empty
> > > +# leaf block being logged and recovered.  This is rather hard to trip, so we
> > > +# use xfs_db to turn a regular leaf block into an empty one.
> > > +make_empty_leaf() {
> > > +	local inum="$1"
> > > +
> > > +	echo "editing inode $inum" >> $seqres.full
> > > +
> > > +	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
> > > +	if [ "$magic" != "0x3bee" ]; then
> > > +		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
> > > +		_fail "inode $inum ablock 0 is not a leaf block?"
> > > +	fi
> > > +
> > > +	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
> > > +
> > > +	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
> > > +		-c "write -d hdr.count 0" \
> > > +		-c "write -d hdr.usedbytes 0" \
> > > +		-c "write -d hdr.firstused $dbsize" \
> > > +		-c "write -d hdr.freemap[0].size $((dbsize - base))" \
> > > +		-c print >> $seqres.full
> > > +}
> > > +
> > > +make_empty_leaf $e0_ino
> > > +make_empty_leaf $e1_ino
> > > +make_empty_leaf $e2_ino
> > > +
> > > +_scratch_mount
> > > +
> > > +# Check that listxattr/getxattr/removexattr do nothing.
> > > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > > +$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > > +$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > > +
> > > +# Add a small attr to e0
> > > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
> > > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
> > > +small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | _md5_checksum)"
> > > +test "$small_md5" = "$smallfile_md5" || \
> > > +	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
> > > +
> > > +# Add a large attr to e1
> > > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
> > > +$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
> > > +large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | _md5_checksum)"
> > > +test "$large_md5" = "$largefile_md5" || \
> > > +	echo "largefile $largefile_md5 does not match large attr $large_md5"
> > > +
> > > +
> > > +# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
> > > +# empty leaf blocks incorrectly too.
> > > +
> > > +# success, all done
> > > +status=0
> > > +exit
> > > 
> 

Re: [PATCH 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Zorro Lang]

On Fri, Jul 08, 2022 at 10:46:38AM -0700, Darrick J. Wong wrote:
> On Sat, Jul 09, 2022 at 01:27:20AM +0800, Zorro Lang wrote:
> > On Sat, Jul 09, 2022 at 01:08:27AM +0800, Zorro Lang wrote:
> > > On Fri, Jul 08, 2022 at 10:02:04AM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Make sure that the kernel can handle empty xattr leaf blocks properly,
> > > > since we've screwed this up enough times.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > 
> > > This version looks good to me, thanks for this new case!
> > > 
> > > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > 
> > Errr.. I just noticed that there's not .out file in this patch, and it's not
> > simple "Silence is golden" I think.
> 
> <sigh> guess who forgot to git add? :(

Thanks for bringing the .out file back:)

BTW, I changed commit id from af866926d865 to 7be3bd8856fb, due to I can't find
af866926d865, but found:
  7be3bd8856fb xfs: empty xattr leaf header blocks are not corruption

Thanks,
Zorro

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > > 
> > > > v1.1: adopt maintainer's refactoring suggestions, skip v4 filesystems
> > > > from the start, and check that we really get an attr leaf block.
> > > > 
> > > > v1.2: eliminate dead code
> > > > ---
> > > >  tests/xfs/845 |  117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 117 insertions(+)
> > > >  create mode 100755 tests/xfs/845
> > > > 
> > > > diff --git a/tests/xfs/845 b/tests/xfs/845
> > > > new file mode 100755
> > > > index 00000000..c142fba1
> > > > --- /dev/null
> > > > +++ b/tests/xfs/845
> > > > @@ -0,0 +1,117 @@
> > > > +#! /bin/bash
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > > > +#
> > > > +# FS QA Test 845
> > > > +#
> > > > +# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
> > > > +# blocks can appear in files as a result of system crashes in the middle of
> > > > +# xattr operations, which means that we /must/ handle them gracefully.
> > > > +# Check that read and write verifiers won't trip, that the get/list/setxattr
> > > > +# operations don't stumble over them, and that xfs_repair will offer to remove
> > > > +# the entire xattr fork if the root xattr leaf block is empty.
> > > > +#
> > > > +# Regression test for kernel commit:
> > > > +#
> > > > +# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
> > > > +#
> > > > +. ./common/preamble
> > > > +_begin_fstest auto quick attr
> > > > +
> > > > +# Import common functions.
> > > > +. ./common/filter
> > > > +. ./common/attr
> > > > +
> > > > +# real QA test starts here
> > > > +
> > > > +_supported_fs xfs
> > > > +_require_scratch
> > > > +_require_scratch_xfs_crc # V4 is deprecated
> > > > +_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
> > > > +
> > > > +_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
> > > > +cat $tmp.mkfs >> $seqres.full
> > > > +source $tmp.mkfs
> > > > +_scratch_mount
> > > > +
> > > > +$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
> > > > +$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
> > > > +
> > > > +smallfile_md5=$(_md5_checksum $SCRATCH_MNT/smallfile)
> > > > +largefile_md5=$(_md5_checksum $SCRATCH_MNT/largefile)
> > > > +
> > > > +# Try to force the creation of a single leaf block in each of three files.
> > > > +# The first one gets a local attr, the second a remote attr, and the third
> > > > +# is left for scrub and repair to find.
> > > > +touch $SCRATCH_MNT/e0
> > > > +touch $SCRATCH_MNT/e1
> > > > +touch $SCRATCH_MNT/e2
> > > > +
> > > > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
> > > > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
> > > > +$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
> > > > +
> > > > +e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
> > > > +e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
> > > > +e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
> > > > +
> > > > +_scratch_unmount
> > > > +
> > > > +# We used to think that it wasn't possible for empty xattr leaf blocks to
> > > > +# exist, but it turns out that setting a large xattr on a file that has no
> > > > +# xattrs can race with a log flush and crash, which results in an empty
> > > > +# leaf block being logged and recovered.  This is rather hard to trip, so we
> > > > +# use xfs_db to turn a regular leaf block into an empty one.
> > > > +make_empty_leaf() {
> > > > +	local inum="$1"
> > > > +
> > > > +	echo "editing inode $inum" >> $seqres.full
> > > > +
> > > > +	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
> > > > +	if [ "$magic" != "0x3bee" ]; then
> > > > +		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
> > > > +		_fail "inode $inum ablock 0 is not a leaf block?"
> > > > +	fi
> > > > +
> > > > +	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
> > > > +
> > > > +	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
> > > > +		-c "write -d hdr.count 0" \
> > > > +		-c "write -d hdr.usedbytes 0" \
> > > > +		-c "write -d hdr.firstused $dbsize" \
> > > > +		-c "write -d hdr.freemap[0].size $((dbsize - base))" \
> > > > +		-c print >> $seqres.full
> > > > +}
> > > > +
> > > > +make_empty_leaf $e0_ino
> > > > +make_empty_leaf $e1_ino
> > > > +make_empty_leaf $e2_ino
> > > > +
> > > > +_scratch_mount
> > > > +
> > > > +# Check that listxattr/getxattr/removexattr do nothing.
> > > > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > > > +$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > > > +$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
> > > > +
> > > > +# Add a small attr to e0
> > > > +$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
> > > > +$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
> > > > +small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | _md5_checksum)"
> > > > +test "$small_md5" = "$smallfile_md5" || \
> > > > +	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
> > > > +
> > > > +# Add a large attr to e1
> > > > +$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
> > > > +$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
> > > > +large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | _md5_checksum)"
> > > > +test "$large_md5" = "$largefile_md5" || \
> > > > +	echo "largefile $largefile_md5 does not match large attr $large_md5"
> > > > +
> > > > +
> > > > +# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
> > > > +# empty leaf blocks incorrectly too.
> > > > +
> > > > +# success, all done
> > > > +status=0
> > > > +exit
> > > > 
> > 
> 

[PATCH v1.3 1/2] xfs: make sure that we handle empty xattr leaf blocks ok

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Make sure that the kernel can handle empty xattr leaf blocks properly,
since we've screwed this up enough times.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v1.1: adopt maintainer's refactoring suggestions, skip v4 filesystems
from the start, and check that we really get an attr leaf block.
v1.2: eliminate dead code
v1.3: actually add golden output
---
 tests/xfs/845     |  117 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/845.out |    7 +++
 2 files changed, 124 insertions(+)
 create mode 100755 tests/xfs/845
 create mode 100644 tests/xfs/845.out

diff --git a/tests/xfs/845 b/tests/xfs/845
new file mode 100755
index 00000000..c142fba1
--- /dev/null
+++ b/tests/xfs/845
@@ -0,0 +1,117 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test 845
+#
+# Make sure that XFS can handle empty leaf xattr blocks correctly.  These
+# blocks can appear in files as a result of system crashes in the middle of
+# xattr operations, which means that we /must/ handle them gracefully.
+# Check that read and write verifiers won't trip, that the get/list/setxattr
+# operations don't stumble over them, and that xfs_repair will offer to remove
+# the entire xattr fork if the root xattr leaf block is empty.
+#
+# Regression test for kernel commit:
+#
+# af866926d865 ("xfs: empty xattr leaf header blocks are not corruption")
+#
+. ./common/preamble
+_begin_fstest auto quick attr
+
+# Import common functions.
+. ./common/filter
+. ./common/attr
+
+# real QA test starts here
+
+_supported_fs xfs
+_require_scratch
+_require_scratch_xfs_crc # V4 is deprecated
+_fixed_by_kernel_commit af866926d865 "xfs: empty xattr leaf header blocks are not corruption"
+
+_scratch_mkfs_xfs | _filter_mkfs >$seqres.full 2>$tmp.mkfs
+cat $tmp.mkfs >> $seqres.full
+source $tmp.mkfs
+_scratch_mount
+
+$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 64k' $SCRATCH_MNT/largefile >> $seqres.full
+$XFS_IO_PROG -f -c "pwrite -S 0x58 0 $isize" $SCRATCH_MNT/smallfile >> $seqres.full
+
+smallfile_md5=$(_md5_checksum $SCRATCH_MNT/smallfile)
+largefile_md5=$(_md5_checksum $SCRATCH_MNT/largefile)
+
+# Try to force the creation of a single leaf block in each of three files.
+# The first one gets a local attr, the second a remote attr, and the third
+# is left for scrub and repair to find.
+touch $SCRATCH_MNT/e0
+touch $SCRATCH_MNT/e1
+touch $SCRATCH_MNT/e2
+
+$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile >> $seqres.full
+$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/smallfile >> $seqres.full
+$ATTR_PROG -s x $SCRATCH_MNT/e2 < $SCRATCH_MNT/smallfile >> $seqres.full
+
+e0_ino=$(stat -c '%i' $SCRATCH_MNT/e0)
+e1_ino=$(stat -c '%i' $SCRATCH_MNT/e1)
+e2_ino=$(stat -c '%i' $SCRATCH_MNT/e2)
+
+_scratch_unmount
+
+# We used to think that it wasn't possible for empty xattr leaf blocks to
+# exist, but it turns out that setting a large xattr on a file that has no
+# xattrs can race with a log flush and crash, which results in an empty
+# leaf block being logged and recovered.  This is rather hard to trip, so we
+# use xfs_db to turn a regular leaf block into an empty one.
+make_empty_leaf() {
+	local inum="$1"
+
+	echo "editing inode $inum" >> $seqres.full
+
+	magic=$(_scratch_xfs_get_metadata_field hdr.info.hdr.magic "inode $inum" "ablock 0")
+	if [ "$magic" != "0x3bee" ]; then
+		_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" -c print >> $seqres.full
+		_fail "inode $inum ablock 0 is not a leaf block?"
+	fi
+
+	base=$(_scratch_xfs_get_metadata_field "hdr.freemap[0].base" "inode $inum" "ablock 0")
+
+	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" \
+		-c "write -d hdr.count 0" \
+		-c "write -d hdr.usedbytes 0" \
+		-c "write -d hdr.firstused $dbsize" \
+		-c "write -d hdr.freemap[0].size $((dbsize - base))" \
+		-c print >> $seqres.full
+}
+
+make_empty_leaf $e0_ino
+make_empty_leaf $e1_ino
+make_empty_leaf $e2_ino
+
+_scratch_mount
+
+# Check that listxattr/getxattr/removexattr do nothing.
+$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | _filter_scratch
+$ATTR_PROG -g x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
+$ATTR_PROG -r x $SCRATCH_MNT/e0 2>&1 | _filter_scratch
+
+# Add a small attr to e0
+$ATTR_PROG -s x $SCRATCH_MNT/e0 < $SCRATCH_MNT/smallfile > /dev/null
+$ATTR_PROG -l $SCRATCH_MNT/e0 2>&1 | sed -e 's/\([0-9]*\) byte/XXX byte/g' | _filter_scratch
+small_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e0 | _md5_checksum)"
+test "$small_md5" = "$smallfile_md5" || \
+	echo "smallfile $smallfile_md5 does not match small attr $small_md5"
+
+# Add a large attr to e1
+$ATTR_PROG -s x $SCRATCH_MNT/e1 < $SCRATCH_MNT/largefile > /dev/null
+$ATTR_PROG -l $SCRATCH_MNT/e1 2>&1 | _filter_scratch
+large_md5="$($GETFATTR_PROG -n user.x --absolute-names --only-values $SCRATCH_MNT/e1 | _md5_checksum)"
+test "$large_md5" = "$largefile_md5" || \
+	echo "largefile $largefile_md5 does not match large attr $large_md5"
+
+
+# Leave e2 to try to trip the repair tools, since xfs_repair used to flag
+# empty leaf blocks incorrectly too.
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/845.out b/tests/xfs/845.out
new file mode 100644
index 00000000..be4e5b7c
--- /dev/null
+++ b/tests/xfs/845.out
@@ -0,0 +1,7 @@
+QA output created by 845
+attr_get: No data available
+Could not get "x" for SCRATCH_MNT/e0
+attr_remove: No data available
+Could not remove "x" for SCRATCH_MNT/e0
+Attribute "x" has a XXX byte value for SCRATCH_MNT/e0
+Attribute "x" has a 65536 byte value for SCRATCH_MNT/e1

[PATCH 2/2] xfs/288: skip repair -n when checking empty root leaf block behavior

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Update this test to reflect the (once again) corrected behavior of the
xattr leaf block verifiers.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/288 |   32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)


diff --git a/tests/xfs/288 b/tests/xfs/288
index e3d230e9..aa664a26 100755
--- a/tests/xfs/288
+++ b/tests/xfs/288
@@ -8,7 +8,7 @@
 # that leaf directly (as xfsprogs commit f714016).
 #
 . ./common/preamble
-_begin_fstest auto quick repair fuzzers
+_begin_fstest auto quick repair fuzzers attr
 
 # Import common functions.
 . ./common/filter
@@ -50,25 +50,19 @@ if [ "$count" != "0" ]; then
 	_notrun "xfs_db can't set attr hdr.count to 0"
 fi
 
-# make sure xfs_repair can find above corruption. If it can't, that
-# means we need to fix this bug on current xfs_repair
-_scratch_xfs_repair -n >> $seqres.full 2>&1
-if [ $? -eq 0 ];then
-	_fail "xfs_repair can't find the corruption"
-else
-	# If xfs_repair can find this corruption, then this repair
-	# should junk above leaf attribute and fix this XFS.
-	_scratch_xfs_repair >> $seqres.full 2>&1
+# Check that xfs_repair discards the attr fork if block 0 is an empty leaf
+# block.  Empty leaf blocks at the start of the xattr data can be a byproduct
+# of a shutdown race, and hence are not a corruption.
+_scratch_xfs_repair >> $seqres.full 2>&1
 
-	# Old xfs_repair maybe find and fix this corruption by
-	# reset the first used heap value and the usedbytes cnt
-	# in ablock 0. That's not what we want. So check if
-	# xfs_repair has junked the whole ablock 0 by xfs_db.
-	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" | \
-		grep -q "no attribute data"
-	if [ $? -ne 0 ]; then
-		_fail "xfs_repair didn't junk the empty attr leaf"
-	fi
+# Old xfs_repair maybe find and fix this corruption by
+# reset the first used heap value and the usedbytes cnt
+# in ablock 0. That's not what we want. So check if
+# xfs_repair has junked the whole ablock 0 by xfs_db.
+_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" | \
+	grep -q "no attribute data"
+if [ $? -ne 0 ]; then
+	_fail "xfs_repair didn't junk the empty attr leaf"
 fi
 
 echo "Silence is golden"

Re: [PATCH 2/2] xfs/288: skip repair -n when checking empty root leaf block behavior

[Zorro Lang]

On Tue, Jul 05, 2022 at 03:02:34PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Update this test to reflect the (once again) corrected behavior of the
> xattr leaf block verifiers.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/xfs/288 |   32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)
> 
> 
> diff --git a/tests/xfs/288 b/tests/xfs/288
> index e3d230e9..aa664a26 100755
> --- a/tests/xfs/288
> +++ b/tests/xfs/288
> @@ -8,7 +8,7 @@
>  # that leaf directly (as xfsprogs commit f714016).
>  #
>  . ./common/preamble
> -_begin_fstest auto quick repair fuzzers
> +_begin_fstest auto quick repair fuzzers attr
>  
>  # Import common functions.
>  . ./common/filter
> @@ -50,25 +50,19 @@ if [ "$count" != "0" ]; then
>  	_notrun "xfs_db can't set attr hdr.count to 0"
>  fi
>  
> -# make sure xfs_repair can find above corruption. If it can't, that
> -# means we need to fix this bug on current xfs_repair
> -_scratch_xfs_repair -n >> $seqres.full 2>&1

So we drop the `xfs_repair -n` test.

Will the latest xfs_repair fail or pass on that? I'm wondering what's the expect
result of `xfs_repair -n` on a xfs with empty leaf? Should it report errors,
or nothing wrong?

Thanks,
Zorro

> -if [ $? -eq 0 ];then
> -	_fail "xfs_repair can't find the corruption"
> -else
> -	# If xfs_repair can find this corruption, then this repair
> -	# should junk above leaf attribute and fix this XFS.
> -	_scratch_xfs_repair >> $seqres.full 2>&1
> +# Check that xfs_repair discards the attr fork if block 0 is an empty leaf
> +# block.  Empty leaf blocks at the start of the xattr data can be a byproduct
> +# of a shutdown race, and hence are not a corruption.
> +_scratch_xfs_repair >> $seqres.full 2>&1
>  
> -	# Old xfs_repair maybe find and fix this corruption by
> -	# reset the first used heap value and the usedbytes cnt
> -	# in ablock 0. That's not what we want. So check if
> -	# xfs_repair has junked the whole ablock 0 by xfs_db.
> -	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" | \
> -		grep -q "no attribute data"
> -	if [ $? -ne 0 ]; then
> -		_fail "xfs_repair didn't junk the empty attr leaf"
> -	fi
> +# Old xfs_repair maybe find and fix this corruption by
> +# reset the first used heap value and the usedbytes cnt
> +# in ablock 0. That's not what we want. So check if
> +# xfs_repair has junked the whole ablock 0 by xfs_db.
> +_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" | \
> +	grep -q "no attribute data"
> +if [ $? -ne 0 ]; then
> +	_fail "xfs_repair didn't junk the empty attr leaf"
>  fi
>  
>  echo "Silence is golden"
> 

Re: [PATCH 2/2] xfs/288: skip repair -n when checking empty root leaf block behavior

[Darrick J. Wong]

On Thu, Jul 07, 2022 at 08:25:25PM +0800, Zorro Lang wrote:
> On Tue, Jul 05, 2022 at 03:02:34PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Update this test to reflect the (once again) corrected behavior of the
> > xattr leaf block verifiers.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  tests/xfs/288 |   32 +++++++++++++-------------------
> >  1 file changed, 13 insertions(+), 19 deletions(-)
> > 
> > 
> > diff --git a/tests/xfs/288 b/tests/xfs/288
> > index e3d230e9..aa664a26 100755
> > --- a/tests/xfs/288
> > +++ b/tests/xfs/288
> > @@ -8,7 +8,7 @@
> >  # that leaf directly (as xfsprogs commit f714016).
> >  #
> >  . ./common/preamble
> > -_begin_fstest auto quick repair fuzzers
> > +_begin_fstest auto quick repair fuzzers attr
> >  
> >  # Import common functions.
> >  . ./common/filter
> > @@ -50,25 +50,19 @@ if [ "$count" != "0" ]; then
> >  	_notrun "xfs_db can't set attr hdr.count to 0"
> >  fi
> >  
> > -# make sure xfs_repair can find above corruption. If it can't, that
> > -# means we need to fix this bug on current xfs_repair
> > -_scratch_xfs_repair -n >> $seqres.full 2>&1
> 
> So we drop the `xfs_repair -n` test.

Yep.

> Will the latest xfs_repair fail or pass on that? I'm wondering what's the expect
> result of `xfs_repair -n` on a xfs with empty leaf? Should it report errors,
> or nothing wrong?

xfs_repair -n no longer fails on attr block 0 being an empty leaf block
since those are part of the ondisk format and are not a corruption.

xfs_repair (without the -n) will clear the attr fork since there aren't
any xattrs if attr block 0 is empty.

--D

> Thanks,
> Zorro
> 
> > -if [ $? -eq 0 ];then
> > -	_fail "xfs_repair can't find the corruption"
> > -else
> > -	# If xfs_repair can find this corruption, then this repair
> > -	# should junk above leaf attribute and fix this XFS.
> > -	_scratch_xfs_repair >> $seqres.full 2>&1
> > +# Check that xfs_repair discards the attr fork if block 0 is an empty leaf
> > +# block.  Empty leaf blocks at the start of the xattr data can be a byproduct
> > +# of a shutdown race, and hence are not a corruption.
> > +_scratch_xfs_repair >> $seqres.full 2>&1
> >  
> > -	# Old xfs_repair maybe find and fix this corruption by
> > -	# reset the first used heap value and the usedbytes cnt
> > -	# in ablock 0. That's not what we want. So check if
> > -	# xfs_repair has junked the whole ablock 0 by xfs_db.
> > -	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" | \
> > -		grep -q "no attribute data"
> > -	if [ $? -ne 0 ]; then
> > -		_fail "xfs_repair didn't junk the empty attr leaf"
> > -	fi
> > +# Old xfs_repair maybe find and fix this corruption by
> > +# reset the first used heap value and the usedbytes cnt
> > +# in ablock 0. That's not what we want. So check if
> > +# xfs_repair has junked the whole ablock 0 by xfs_db.
> > +_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" | \
> > +	grep -q "no attribute data"
> > +if [ $? -ne 0 ]; then
> > +	_fail "xfs_repair didn't junk the empty attr leaf"
> >  fi
> >  
> >  echo "Silence is golden"
> > 
> 

Re: [PATCH 2/2] xfs/288: skip repair -n when checking empty root leaf block behavior

[Zorro Lang]

On Thu, Jul 07, 2022 at 11:08:20AM -0700, Darrick J. Wong wrote:
> On Thu, Jul 07, 2022 at 08:25:25PM +0800, Zorro Lang wrote:
> > On Tue, Jul 05, 2022 at 03:02:34PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Update this test to reflect the (once again) corrected behavior of the
> > > xattr leaf block verifiers.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  tests/xfs/288 |   32 +++++++++++++-------------------
> > >  1 file changed, 13 insertions(+), 19 deletions(-)
> > > 
> > > 
> > > diff --git a/tests/xfs/288 b/tests/xfs/288
> > > index e3d230e9..aa664a26 100755
> > > --- a/tests/xfs/288
> > > +++ b/tests/xfs/288
> > > @@ -8,7 +8,7 @@
> > >  # that leaf directly (as xfsprogs commit f714016).
> > >  #
> > >  . ./common/preamble
> > > -_begin_fstest auto quick repair fuzzers
> > > +_begin_fstest auto quick repair fuzzers attr
> > >  
> > >  # Import common functions.
> > >  . ./common/filter
> > > @@ -50,25 +50,19 @@ if [ "$count" != "0" ]; then
> > >  	_notrun "xfs_db can't set attr hdr.count to 0"
> > >  fi
> > >  
> > > -# make sure xfs_repair can find above corruption. If it can't, that
> > > -# means we need to fix this bug on current xfs_repair
> > > -_scratch_xfs_repair -n >> $seqres.full 2>&1
> > 
> > So we drop the `xfs_repair -n` test.
> 
> Yep.
> 
> > Will the latest xfs_repair fail or pass on that? I'm wondering what's the expect
> > result of `xfs_repair -n` on a xfs with empty leaf? Should it report errors,
> > or nothing wrong?
> 
> xfs_repair -n no longer fails on attr block 0 being an empty leaf block
> since those are part of the ondisk format and are not a corruption.
> 
> xfs_repair (without the -n) will clear the attr fork since there aren't
> any xattrs if attr block 0 is empty.

Thanks for the explain.

Reviewed-by: Zorro Lang <zlang@redhat.com>

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > > -if [ $? -eq 0 ];then
> > > -	_fail "xfs_repair can't find the corruption"
> > > -else
> > > -	# If xfs_repair can find this corruption, then this repair
> > > -	# should junk above leaf attribute and fix this XFS.
> > > -	_scratch_xfs_repair >> $seqres.full 2>&1
> > > +# Check that xfs_repair discards the attr fork if block 0 is an empty leaf
> > > +# block.  Empty leaf blocks at the start of the xattr data can be a byproduct
> > > +# of a shutdown race, and hence are not a corruption.
> > > +_scratch_xfs_repair >> $seqres.full 2>&1
> > >  
> > > -	# Old xfs_repair maybe find and fix this corruption by
> > > -	# reset the first used heap value and the usedbytes cnt
> > > -	# in ablock 0. That's not what we want. So check if
> > > -	# xfs_repair has junked the whole ablock 0 by xfs_db.
> > > -	_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" | \
> > > -		grep -q "no attribute data"
> > > -	if [ $? -ne 0 ]; then
> > > -		_fail "xfs_repair didn't junk the empty attr leaf"
> > > -	fi
> > > +# Old xfs_repair maybe find and fix this corruption by
> > > +# reset the first used heap value and the usedbytes cnt
> > > +# in ablock 0. That's not what we want. So check if
> > > +# xfs_repair has junked the whole ablock 0 by xfs_db.
> > > +_scratch_xfs_db -x -c "inode $inum" -c "ablock 0" | \
> > > +	grep -q "no attribute data"
> > > +if [ $? -ne 0 ]; then
> > > +	_fail "xfs_repair didn't junk the empty attr leaf"
> > >  fi
> > >  
> > >  echo "Silence is golden"
> > > 
> > 
>