Re: [PATCH 3/3] xfs: reduce debug overhead of dir leaf/node checks

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

On Tue, Feb 23, 2021 at 04:47:48PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> On debug kernels, we call xfs_dir3_leaf_check_int() multiple times
> on every directory modification. The robust hash ordering checks it
> does on every entry in the leaf on every call results in a massive
> CPU overhead which slows down debug kernels by a large amount.
> 
> We use xfs_dir3_leaf_check_int() for the verifiers as well, so we
> can't just gut the function to reduce overhead. What we can do,
> however, is reduce the work it does when it is called from the
> debug interfaces, just leaving the high level checks in place and
> leaving the robust validation to the verifiers. This means the debug
> checks will catch gross errors, but subtle bugs might not be caught
> until a verifier is run.
> 
> It is easy enough to restore the existing debug behaviour if the
> developer needs it (just change a call parameter in the debug code),
> but overwise the overhead makes testing large directory block sizes
> on debug kernels very slow.
> 
> Profile at an unlink rate of ~80k file/s on a 64k block size
> filesystem before the patch:
> 
>   40.30%  [kernel]  [k] xfs_dir3_leaf_check_int
>   10.98%  [kernel]  [k] __xfs_dir3_data_check
>    8.10%  [kernel]  [k] xfs_verify_dir_ino
>    4.42%  [kernel]  [k] memcpy
>    2.22%  [kernel]  [k] xfs_dir2_data_get_ftype
>    1.52%  [kernel]  [k] do_raw_spin_lock
> 
> Profile after, at an unlink rate of ~125k files/s (+50% improvement)
> has largely dropped the leaf verification debug overhead out of the
> profile.
> 
>   16.53%  [kernel]  [k] __xfs_dir3_data_check
>   12.53%  [kernel]  [k] xfs_verify_dir_ino
>    7.97%  [kernel]  [k] memcpy
>    3.36%  [kernel]  [k] xfs_dir2_data_get_ftype
>    2.86%  [kernel]  [k] __pv_queued_spin_lock_slowpath
> 
> Create shows a similar change in profile and a +25% improvement in
> performance.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_dir2_leaf.c | 10 +++++++---
>  fs/xfs/libxfs/xfs_dir2_node.c |  2 +-
>  fs/xfs/libxfs/xfs_dir2_priv.h |  3 ++-
>  3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> index 95d2a3f92d75..ccd8d0aa62b8 100644
> --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> @@ -113,7 +113,7 @@ xfs_dir3_leaf1_check(
>  	} else if (leafhdr.magic != XFS_DIR2_LEAF1_MAGIC)
>  		return __this_address;
>  
> -	return xfs_dir3_leaf_check_int(dp->i_mount, &leafhdr, leaf);
> +	return xfs_dir3_leaf_check_int(dp->i_mount, &leafhdr, leaf, false);
>  }
>  
>  static inline void
> @@ -139,7 +139,8 @@ xfs_failaddr_t
>  xfs_dir3_leaf_check_int(
>  	struct xfs_mount		*mp,
>  	struct xfs_dir3_icleaf_hdr	*hdr,
> -	struct xfs_dir2_leaf		*leaf)
> +	struct xfs_dir2_leaf		*leaf,
> +	bool				expensive_checking)
>  {
>  	struct xfs_da_geometry		*geo = mp->m_dir_geo;
>  	xfs_dir2_leaf_tail_t		*ltp;
> @@ -162,6 +163,9 @@ xfs_dir3_leaf_check_int(
>  	    (char *)&hdr->ents[hdr->count] > (char *)xfs_dir2_leaf_bests_p(ltp))
>  		return __this_address;
>  
> +	if (!expensive_checking)
> +		return NULL;
> +
>  	/* Check hash value order, count stale entries.  */
>  	for (i = stale = 0; i < hdr->count; i++) {
>  		if (i + 1 < hdr->count) {
> @@ -195,7 +199,7 @@ xfs_dir3_leaf_verify(
>  		return fa;
>  
>  	xfs_dir2_leaf_hdr_from_disk(mp, &leafhdr, bp->b_addr);
> -	return xfs_dir3_leaf_check_int(mp, &leafhdr, bp->b_addr);
> +	return xfs_dir3_leaf_check_int(mp, &leafhdr, bp->b_addr, true);
>  }
>  
>  static void
> diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> index 5d51265d29d6..80a64117b460 100644
> --- a/fs/xfs/libxfs/xfs_dir2_node.c
> +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> @@ -73,7 +73,7 @@ xfs_dir3_leafn_check(
>  	} else if (leafhdr.magic != XFS_DIR2_LEAFN_MAGIC)
>  		return __this_address;
>  
> -	return xfs_dir3_leaf_check_int(dp->i_mount, &leafhdr, leaf);
> +	return xfs_dir3_leaf_check_int(dp->i_mount, &leafhdr, leaf, false);
>  }
>  
>  static inline void
> diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> index 44c6a77cba05..94943ce49cab 100644
> --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> @@ -127,7 +127,8 @@ xfs_dir3_leaf_find_entry(struct xfs_dir3_icleaf_hdr *leafhdr,
>  extern int xfs_dir2_node_to_leaf(struct xfs_da_state *state);
>  
>  extern xfs_failaddr_t xfs_dir3_leaf_check_int(struct xfs_mount *mp,
> -		struct xfs_dir3_icleaf_hdr *hdr, struct xfs_dir2_leaf *leaf);
> +		struct xfs_dir3_icleaf_hdr *hdr, struct xfs_dir2_leaf *leaf,
> +		bool expensive_checks);
>  
>  /* xfs_dir2_node.c */
>  void xfs_dir2_free_hdr_from_disk(struct xfs_mount *mp,
> -- 
> 2.28.0
>