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

[Carlos Maiolino <cem@kernel.org>]

On Mon, Oct 05, 2026 at 02:17:41PM -0700, Darrick J. Wong wrote:
> On Mon, Oct 05, 2026 at 03:43:08PM -0500, Eric Sandeen wrote:
> > On 10/5/26 3:20 PM, Darrick J. Wong wrote:
> > > 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.
> > 
> > ...
> > 
> > >> 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:
> > 
> > I thought about a case switch but it seemed excessive for 2 choices.
> > My ASSERT does make sure we have one or the other valid value... *shrug*
> > 
> > > 	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;
> > 

> > I guess this has the advantage of kinda handling an unknown whichfork or
> > XFS_COW_FORK, though who knows what happens to callers that get a do-nothing
> > return at that point?
> > > 	}
> 
> There shouldn't be any, I just thought it looks cleaner. :)

Perhaps...

ilog_flags |= (whichfork == XFS_DATA_FORK ? XFS_ILOG_DDATA : XFS_ILOG_ADATA)

?

But the switch indeed looks nicer.

> 
> --D
> 
> > > 
> > > 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
> > >>
> > >>
> > > 
> > 
>