Re: [PATCH 05/11] xfs: Check for extent overflow when adding/removing xattrs

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Nov 13, 2020 at 04:56:57PM +0530, Chandan Babu R wrote:
> This test verifies that XFS does not cause inode fork's extent count to
> overflow when adding/removing xattrs.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  tests/xfs/525     | 154 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/525.out |  16 +++++
>  tests/xfs/group   |   1 +
>  3 files changed, 171 insertions(+)
>  create mode 100755 tests/xfs/525
>  create mode 100644 tests/xfs/525.out
> 
> diff --git a/tests/xfs/525 b/tests/xfs/525
> new file mode 100755
> index 00000000..1d5d6e7c
> --- /dev/null
> +++ b/tests/xfs/525
> @@ -0,0 +1,154 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2020 Chandan Babu R.  All Rights Reserved.
> +#
> +# FS QA Test 525
> +#
> +# Verify that XFS does not cause inode fork's extent count to overflow when
> +# Adding/removing xattrs.
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/attr
> +. ./common/inject
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +_supported_fs xfs
> +_require_scratch
> +_require_attrs
> +_require_xfs_debug
> +_require_test_program "punch-alternating"
> +_require_xfs_io_error_injection "reduce_max_iextents"
> +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> +
> +attr_set()
> +{
> +	echo "* Set xattrs"
> +
> +	echo "Format and mount fs"
> +	_scratch_mkfs_sized $((1024 * 1024 * 1024)) >> $seqres.full
> +	_scratch_mount >> $seqres.full
> +
> +	bsize=$(_get_block_size $SCRATCH_MNT)
> +
> +	testfile=$SCRATCH_MNT/testfile
> +
> +	echo "Consume free space"
> +	dd if=/dev/zero of=${testfile} bs=${bsize} >> $seqres.full 2>&1
> +	sync
> +
> +	echo "Create fragmented filesystem"
> +	$here/src/punch-alternating $testfile >> $seqres.full
> +	sync
> +
> +	echo "Inject reduce_max_iextents error tag"
> +	xfs_io -x -c 'inject reduce_max_iextents' $SCRATCH_MNT
> +
> +	echo "Inject bmap_alloc_minlen_extent error tag"
> +	xfs_io -x -c 'inject bmap_alloc_minlen_extent' $SCRATCH_MNT
> +
> +	echo "Create xattrs"
> +
> +	attr_len=$(uuidgen | wc -c)
> +	nr_attrs=$((bsize * 20 / attr_len))
> +	for i in $(seq 1 $nr_attrs); do
> +		$SETFATTR_PROG -n "trusted.""$(uuidgen)" $testfile \

Does this test require UUIDs in the attr names?  Why wouldn't
$(printf "%037d" $i) suffice for this purpose?

Though if you insist upon using uuids, please call $UUIDGEN_PROG per
fstest custom.

> +			 >> $seqres.full 2>&1
> +		[[ $? != 0 ]] && break
> +	done
> +
> +	testino=$(stat -c "%i" $testfile)
> +
> +	_scratch_unmount >> $seqres.full
> +
> +	echo "Verify uquota inode's extent count"

Huh?  I thought we were testing file attrs?

> +	nextents=$(_scratch_get_iext_count $testino attr || \
> +			_fail "Unable to obtain inode fork's extent count")
> +	if (( $nextents > 10 )); then
> +		echo "Extent count overflow check failed: nextents = $nextents"
> +		exit 1
> +	fi
> +}
> +
> +attr_remove()
> +{
> +	echo "* Remove xattrs"
> +
> +	echo "Format and mount fs"
> +	_scratch_mkfs_sized $((1024 * 1024 * 1024)) >> $seqres.full
> +	_scratch_mount >> $seqres.full
> +
> +	bsize=$(_get_block_size $SCRATCH_MNT)
> +
> +	testfile=$SCRATCH_MNT/testfile
> +
> +	echo "Consume free space"
> +	dd if=/dev/zero of=${testfile} bs=${bsize} >> $seqres.full 2>&1
> +	sync
> +
> +	echo "Create fragmented filesystem"
> +	$here/src/punch-alternating $testfile >> $seqres.full
> +	sync
> +
> +	testino=$(stat -c "%i" $testfile)
> +
> +	naextents=0
> +	last=""
> +
> +	attr_len=$(uuidgen | wc -c)
> +	nr_attrs=$((bsize / attr_len))
> +
> +	echo "Create initial xattr extents"
> +	while (( $naextents < 4 )); do
> +		xfs_io -x -c 'inject bmap_alloc_minlen_extent' $SCRATCH_MNT
> +
> +		for i in $(seq 1 $nr_attrs); do
> +			last="trusted.""$(uuidgen)"
> +			$SETFATTR_PROG -n $last $testfile
> +		done
> +
> +		_scratch_unmount >> $seqres.full
> +
> +		naextents=$(_scratch_get_iext_count $testino attr || \
> +				_fail "Unable to obtain inode fork's extent count")
> +
> +		_scratch_mount >> $seqres.full
> +	done
> +
> +	echo "Inject reduce_max_iextents error tag"
> +	xfs_io -x -c 'inject reduce_max_iextents' $SCRATCH_MNT
> +
> +	echo "Remove xattr to trigger -EFBIG"
> +	$SETFATTR_PROG -x "$last" $testfile >> $seqres.full 2>&1
> +	if [[ $? == 0 ]]; then
> +		echo "Xattr removal succeeded; Should have failed "

So at this point the user has a file for which he can't ever remove the
xattrs for fear of overflowing naextents.  The only way to clear this is
to delete the file, so shouldn't you be testing that this succeeds?

--D

> +		exit 1
> +	fi
> +}
> +
> +attr_set
> +attr_remove
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/525.out b/tests/xfs/525.out
> new file mode 100644
> index 00000000..cc40e6e2
> --- /dev/null
> +++ b/tests/xfs/525.out
> @@ -0,0 +1,16 @@
> +QA output created by 525
> +* Set xattrs
> +Format and mount fs
> +Consume free space
> +Create fragmented filesystem
> +Inject reduce_max_iextents error tag
> +Inject bmap_alloc_minlen_extent error tag
> +Create xattrs
> +Verify uquota inode's extent count
> +* Remove xattrs
> +Format and mount fs
> +Consume free space
> +Create fragmented filesystem
> +Create initial xattr extents
> +Inject reduce_max_iextents error tag
> +Remove xattr to trigger -EFBIG
> diff --git a/tests/xfs/group b/tests/xfs/group
> index 3fa38c36..bd38aff0 100644
> --- a/tests/xfs/group
> +++ b/tests/xfs/group
> @@ -522,3 +522,4 @@
>  522 auto quick quota
>  523 auto quick realtime growfs
>  524 auto quick punch zero insert collapse
> +525 auto quick attr
> -- 
> 2.28.0
>