Re: [PATCH] common/populate: Ensure that S_IFDIR.FMT_BTREE is in btree format

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Dec 01, 2022 at 04:12:08PM +0800, Gao Xiang wrote:
> Sometimes "$((128 * dblksz / 40))" dirents cannot make sure that
> S_IFDIR.FMT_BTREE could become btree format for its DATA fork.
> 
> Actually we just observed it can fail after apply our inode
> extent-to-btree workaround. The root cause is that the kernel may be
> too good at allocating consecutive blocks so that the data fork is
> still in extents format.
> 
> Therefore instead of using a fixed number, let's make sure the number
> of extents is large enough than (inode size - inode core size) /
> sizeof(xfs_bmbt_rec_t).
> 
> Suggested-by: "Darrick J. Wong" <djwong@kernel.org>
> Cc: Ziyang Zhang <ZiyangZhang@linux.alibaba.com>
> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> ---
>  common/populate | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/common/populate b/common/populate
> index 6e004997..e179a300 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -71,6 +71,25 @@ __populate_create_dir() {
>  	done
>  }
>  
> +# Create a large directory and ensure that it's a btree format
> +__populate_create_btree_dir() {

Since this encodes behavior specific to xfs, this ought to be called

__populate_xfs_create_btree_dir

or something like that.

> +	name="$1"
> +	isize="$2"

These ought to be local variables, e.g.

	local name="$1"
	local isize="$2"

So that they don't pollute the global name scope.  Yay bash.

> +
> +	mkdir -p "${name}"
> +	d=0
> +	while true; do
> +		creat=mkdir
> +		test "$((d % 20))" -eq 0 && creat=touch
> +		$creat "${name}/$(printf "%.08d" "$d")"
> +		if [ "$((d % 40))" -eq 0 ]; then
> +			nexts="$($XFS_IO_PROG -c "stat" $name | grep 'fsxattr.nextents' | sed -e 's/^.*nextents = //g' -e 's/\([0-9]*\).*$/\1/g')"

_xfs_get_fsxattr...

> +			[ "$nexts" -gt "$(((isize - 176) / 16))" ] && break

Only need to calculate this once if you declare this at the top:

	# We need enough extents to guarantee that the data fork is in
	# btree format.  Cycling the mount to use xfs_db is too slow, so
	# watch for when the extent count exceeds the space after the
	# inode core.
	local max_nextents="$(((isize - 176) / 16))"

and then you can do:

			[[ $nexts -gt $max_nextents ]] && break

Also not a fan of hardcoding 176 around fstests, but I don't know how
we'd detect that at all.

# Number of bytes reserved for only the inode record, excluding the
# immediate fork areas.
_xfs_inode_core_bytes()
{
	echo 176
}

I guess?  Or extract it from tests/xfs/122.out?

> +		fi
> +		d=$((d+1))
> +	done
> +}
> +
>  # Add a bunch of attrs to a file
>  __populate_create_attr() {
>  	name="$1"
> @@ -176,6 +195,7 @@ _scratch_xfs_populate() {
>  
>  	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
>  	dblksz="$(_xfs_get_dir_blocksize "$SCRATCH_MNT")"
> +	isize="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep meta-data=.*isize | sed -e 's/^.*isize=//g' -e 's/\([0-9]*\).*$/\1/g')"

Please hoist this to common/xfs:

# Number of bytes reserved for a full inode record, which includes the
# immediate fork areas.
_xfs_inode_size()
{
	local mntpoint="$1"

	$XFS_INFO_PROG "$mountpoint" | grep 'meta-data=.*isize' | sed -e 's/^.*isize=\([0-9]*\).*$/\1/g')"
}

Otherwise this looks reasonable.

--D

>  	crc="$(_xfs_has_feature "$SCRATCH_MNT" crc -v)"
>  	if [ $crc -eq 1 ]; then
>  		leaf_hdr_size=64
> @@ -226,7 +246,7 @@ _scratch_xfs_populate() {
>  
>  	# - BTREE
>  	echo "+ btree dir"
> -	__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_BTREE" "$((128 * dblksz / 40))" true
> +	__populate_create_btree_dir "${SCRATCH_MNT}/S_IFDIR.FMT_BTREE" "$isize"
>  
>  	# Symlinks
>  	# - FMT_LOCAL
> -- 
> 2.24.4
>