Re: [PATCH V2 2/3] xfs: factor out xrep_reset_fork_to_extents helper for scrub/repair

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

On Mon, Oct 05, 2026 at 02:11:37PM -0500, Eric Sandeen wrote:
> The open-coded ~10 line series of steps to reset a fork to empty extents
> format is repeated 3 times; factor this out into a helper to eliminate
> cut and paste.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> Better? (nb: compile tested only)
> 
>  fs/xfs/scrub/attr_repair.c    | 17 ++---------------
>  fs/xfs/scrub/dir_repair.c     | 16 ++--------------
>  fs/xfs/scrub/repair.c         | 26 ++++++++++++++++++++++++++
>  fs/xfs/scrub/repair.h         |  1 +
>  fs/xfs/scrub/symlink_repair.c | 16 ++--------------
>  5 files changed, 33 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/xfs/scrub/attr_repair.c b/fs/xfs/scrub/attr_repair.c
> index 28f92e9ba72b..25bf4e49d56c 100644
> --- a/fs/xfs/scrub/attr_repair.c
> +++ b/fs/xfs/scrub/attr_repair.c
> @@ -1316,21 +1316,8 @@ xrep_xattr_swap_prep(
>  	 * that to an empty extent list in preparation for the atomic mapping
>  	 * exchange.
>  	 */
> -	if (ip_local) {
> -		struct xfs_ifork	*ifp;
> -
> -		ifp = xfs_ifork_ptr(sc->ip, XFS_ATTR_FORK);
> -
> -		xfs_idestroy_fork(ifp);
> -		ifp->if_format = XFS_DINODE_FMT_EXTENTS;
> -		ifp->if_nextents = 0;
> -		ifp->if_bytes = 0;
> -		ifp->if_data = NULL;
> -		ifp->if_height = 0;
> -
> -		xfs_trans_log_inode(sc->tp, sc->ip,
> -				XFS_ILOG_CORE | XFS_ILOG_ADATA);
> -	}
> +	if (ip_local)
> +		xrep_reset_fork_to_extents(sc, XFS_ATTR_FORK);
>  
>  	return 0;
>  }
> diff --git a/fs/xfs/scrub/dir_repair.c b/fs/xfs/scrub/dir_repair.c
> index 2cfcf1c35679..5db1ec324409 100644
> --- a/fs/xfs/scrub/dir_repair.c
> +++ b/fs/xfs/scrub/dir_repair.c
> @@ -1510,20 +1510,8 @@ xrep_dir_swap_prep(
>  	 * to an empty extent list in preparation for the atomic mapping
>  	 * exchange.
>  	 */
> -	if (ip_local) {
> -		struct xfs_ifork	*ifp;
> -
> -		ifp = xfs_ifork_ptr(sc->ip, XFS_DATA_FORK);
> -		xfs_idestroy_fork(ifp);
> -		ifp->if_format = XFS_DINODE_FMT_EXTENTS;
> -		ifp->if_nextents = 0;
> -		ifp->if_bytes = 0;
> -		ifp->if_data = NULL;
> -		ifp->if_height = 0;
> -
> -		xfs_trans_log_inode(sc->tp, sc->ip,
> -				XFS_ILOG_CORE | XFS_ILOG_DDATA);
> -	}
> +	if (ip_local)
> +		xrep_reset_fork_to_extents(sc, XFS_DATA_FORK);
>  
>  	return 0;
>  }
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index c2a437416227..8385308c852f 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -882,6 +882,32 @@ xrep_ino_ensure_extent_count(
>  	return 0;
>  }
>  
> +/* Discard the contents of this fork and initialize as empty extent-format. */
> +void
> +xrep_reset_fork_to_extents(
> +	struct xfs_scrub	*sc,
> +	int			whichfork)
> +{
> +	struct xfs_ifork	*ifp = xfs_ifork_ptr(sc->ip, whichfork);
> +	uint			ilog_flags = XFS_ILOG_CORE;
> +
> +	ASSERT(whichfork == XFS_DATA_FORK || whichfork == XFS_ATTR_FORK);
> +
> +	if (whichfork == XFS_DATA_FORK)
> +		ilog_flags |= XFS_ILOG_DDATA;
> +	else
> +		ilog_flags |= XFS_ILOG_ADATA;

Nit pick:

	switch (whichfork) {
	case XFS_DATA_FORK:
		ilog_flags |= XFS_ILOG_DDATA;
		break;
	case XFS_ATTR_FORK:
		ilog_flags |= XFS_ILOG_ADATA;
		break;
	default:
		ASSERT(0);
		return;
	}

Either way this looks like a good hoist to me, so
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> +
> +	xfs_idestroy_fork(ifp);
> +	ifp->if_format = XFS_DINODE_FMT_EXTENTS;
> +	ifp->if_nextents = 0;
> +	ifp->if_bytes = 0;
> +	ifp->if_data = NULL;
> +	ifp->if_height = 0;
> +
> +	xfs_trans_log_inode(sc->tp, sc->ip, ilog_flags);
> +}
> +
>  /*
>   * Initialize all the btree cursors for an AG repair except for the btree that
>   * we're rebuilding.
> diff --git a/fs/xfs/scrub/repair.h b/fs/xfs/scrub/repair.h
> index 2bb125c4f9bf..c1ba462e4426 100644
> --- a/fs/xfs/scrub/repair.h
> +++ b/fs/xfs/scrub/repair.h
> @@ -81,6 +81,7 @@ int xrep_setup_xfbtree(struct xfs_scrub *sc, const char *descr);
>  int xrep_ino_ensure_extent_count(struct xfs_scrub *sc, int whichfork,
>  		xfs_extnum_t nextents);
>  int xrep_reset_perag_resv(struct xfs_scrub *sc);
> +void xrep_reset_fork_to_extents(struct xfs_scrub *sc, int whichfork);
>  int xrep_bmap(struct xfs_scrub *sc, int whichfork, bool allow_unwritten);
>  int xrep_metadata_inode_forks(struct xfs_scrub *sc);
>  int xrep_setup_ag_rmapbt(struct xfs_scrub *sc);
> diff --git a/fs/xfs/scrub/symlink_repair.c b/fs/xfs/scrub/symlink_repair.c
> index 181961364233..9f6901905bfd 100644
> --- a/fs/xfs/scrub/symlink_repair.c
> +++ b/fs/xfs/scrub/symlink_repair.c
> @@ -303,20 +303,8 @@ xrep_symlink_swap_prep(
>  	 * to an empty extent list in preparation for the atomic mapping
>  	 * exchange.
>  	 */
> -	if (ip_local) {
> -		struct xfs_ifork	*ifp;
> -
> -		ifp = xfs_ifork_ptr(sc->ip, XFS_DATA_FORK);
> -		xfs_idestroy_fork(ifp);
> -		ifp->if_format = XFS_DINODE_FMT_EXTENTS;
> -		ifp->if_nextents = 0;
> -		ifp->if_bytes = 0;
> -		ifp->if_data = NULL;
> -		ifp->if_height = 0;
> -
> -		xfs_trans_log_inode(sc->tp, sc->ip,
> -				XFS_ILOG_CORE | XFS_ILOG_DDATA);
> -	}
> +	if (ip_local)
> +		xrep_reset_fork_to_extents(sc, XFS_DATA_FORK);
>  
>  	return 0;
>  }
> -- 
> 2.55.0
> 
>