Re: [PATCH 2/3] populate: ensure btree directories are created reliably

From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: fstests@vger.kernel.org
Subject: Re: [PATCH 2/3] populate: ensure btree directories are created reliably
Date: Tue, 10 Jan 2023 21:47:26 -0800	[thread overview]
Message-ID: <Y75NbhbmZJMnVBFK@magnolia> (raw)
In-Reply-To: <20230110224906.1171483-3-david@fromorbit.com>

On Wed, Jan 11, 2023 at 09:49:05AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The population function creates an XFS btree format directory by
> polling the extent count of the inode and creating new dirents until
> the extent count goes over the limit that pushes it into btree
> format.
> 
> It then removes every second dirent to create empty space in the
> directory data to ensure that operations like metadump with
> obfuscation can check that they don't leak stale data from deleted
> dirents.
> 
> Whilst this does not result in directory data blocks being freed, it
> does not take into account the fact that the dabtree index has half
> the entries removed from it and that can result in btree nodes
> merging and extents being freed. This causes the extent count to go
> down, and the inode is converted back into extent form. The
> population checks then fail because it should be in btree form.
> 
> Fix this by counting the number of directory data extents rather than
> the total number of extents in the data fork. We can do this simply
> by using xfs_bmap and counting the number of extents returned as it
> does not report extents beyond EOF (which is where the dabtree is
> located). As the number of data blocks does not change with the
> dirent removal algorithm used, this will ensure that the inode data
> fork remains in btree format.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  common/populate | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/common/populate b/common/populate
> index 9b60fa5c1..7b5b16fb8 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -80,8 +80,11 @@ __populate_create_nfiles() {
>  			continue
>  		fi
>  
> -		local nextents="$(_xfs_get_fsxattr nextents $name)"
> -		if [ "${nextents}" -gt "${max_nextents}" ]; then
> +		# Extent count checks use data blocks only to avoid the removal
> +		# step from removing dabtree index blocks and reducing the
> +		# number of extents below the required threshold.
> +		local nextents="$(xfs_bmap ${name} |grep -v hole | wc -l)"
> +		if [ "$((nextents - 1))" -gt "${max_nextents}" ]; then

Pretty much the same patch I had in my tree...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  			echo ${d}
>  			break
>  		fi
> -- 
> 2.38.1
> 