[PATCH 0/3] xfs: more misc code deduplication

[PATCH 0/3] xfs: more misc code deduplication

[Eric Sandeen]

Reduce more cut and paste.

These 3 are independent IIRC, so if anything gives anyone heartburn we
can just drop the patch(es).

 libxfs/xfs_dir2_sf.c      |   68 +++++------
 scrub/attr_repair.c       |   12 --
 scrub/dir_repair.c        |   11 -
 scrub/refcount.c          |   27 ----
 scrub/refcount.h          |   56 +++++++++
 scrub/refcount_repair.c   |    3 
 scrub/repair.c            |   16 ++
 scrub/repair.h            |    1 
 scrub/rtrefcount.c        |  263 +-------------------------------------------
 scrub/rtrefcount_repair.c |   39 ------
 scrub/symlink_repair.c    |   11 -
 11 files changed, 122 insertions(+), 385 deletions(-)

Thanks,
-Eric

[PATCH 1/3] xfs: factor out xfs_dir2_sf_copy_entries helper

[Eric Sandeen]

xfs_dir2_sf_toino8 and xfs_dir2_sf_toino64 share a dozen or so lines
of copied code used to move short form directory entries; factor that
out to eliminate cut and paste.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/libxfs/xfs_dir2_sf.c | 68 ++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 38 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 0567cf8b9c1b..a8675a0d574e 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -1124,6 +1124,34 @@ xfs_dir2_sf_replace(
 	return 0;
 }
 
+static inline void
+xfs_dir2_sf_copy_entries(
+	struct xfs_mount	*mp,
+	struct xfs_dir2_sf_hdr	*sfp,
+	struct xfs_dir2_sf_hdr	*oldsfp)
+{
+	int			i;
+	struct xfs_dir2_sf_entry *sfep;		/* new sf entry */
+	struct xfs_dir2_sf_entry *oldsfep;	/* old sf entry */
+
+	/*
+	 * Copy the entries field by field.
+	 */
+	for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp),
+		    oldsfep = xfs_dir2_sf_firstentry(oldsfp);
+	     i < sfp->count;
+	     i++, sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep),
+		  oldsfep = xfs_dir2_sf_nextentry(mp, oldsfp, oldsfep)) {
+		sfep->namelen = oldsfep->namelen;
+		memcpy(sfep->offset, oldsfep->offset, sizeof(sfep->offset));
+		memcpy(sfep->name, oldsfep->name, sfep->namelen);
+		xfs_dir2_sf_put_ino(mp, sfp, sfep,
+				xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep));
+		xfs_dir2_sf_put_ftype(mp, sfep,
+				xfs_dir2_sf_get_ftype(mp, oldsfep));
+	}
+}
+
 /*
  * Convert from 8-byte inode numbers to 4-byte inode numbers.
  * The last 8-byte inode number is gone, but the count is still 1.
@@ -1136,11 +1164,8 @@ xfs_dir2_sf_toino4(
 	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_dir2_sf_hdr	*oldsfp = dp->i_df.if_data;
 	char			*buf;		/* old dir's buffer */
-	int			i;		/* entry index */
 	int			newsize;	/* new inode size */
-	xfs_dir2_sf_entry_t	*oldsfep;	/* old sf entry */
 	int			oldsize;	/* old inode size */
-	xfs_dir2_sf_entry_t	*sfep;		/* new sf entry */
 	xfs_dir2_sf_hdr_t	*sfp;		/* new sf directory */
 
 	trace_xfs_dir2_sf_toino4(args);
@@ -1171,22 +1196,7 @@ xfs_dir2_sf_toino4(
 	sfp->count = oldsfp->count;
 	sfp->i8count = 0;
 	xfs_dir2_sf_put_parent_ino(sfp, xfs_dir2_sf_get_parent_ino(oldsfp));
-	/*
-	 * Copy the entries field by field.
-	 */
-	for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp),
-		    oldsfep = xfs_dir2_sf_firstentry(oldsfp);
-	     i < sfp->count;
-	     i++, sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep),
-		  oldsfep = xfs_dir2_sf_nextentry(mp, oldsfp, oldsfep)) {
-		sfep->namelen = oldsfep->namelen;
-		memcpy(sfep->offset, oldsfep->offset, sizeof(sfep->offset));
-		memcpy(sfep->name, oldsfep->name, sfep->namelen);
-		xfs_dir2_sf_put_ino(mp, sfp, sfep,
-				xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep));
-		xfs_dir2_sf_put_ftype(mp, sfep,
-				xfs_dir2_sf_get_ftype(mp, oldsfep));
-	}
+	xfs_dir2_sf_copy_entries(mp, sfp, oldsfp);
 	/*
 	 * Clean up the inode.
 	 */
@@ -1208,11 +1218,8 @@ xfs_dir2_sf_toino8(
 	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_dir2_sf_hdr	*oldsfp = dp->i_df.if_data;
 	char			*buf;		/* old dir's buffer */
-	int			i;		/* entry index */
 	int			newsize;	/* new inode size */
-	xfs_dir2_sf_entry_t	*oldsfep;	/* old sf entry */
 	int			oldsize;	/* old inode size */
-	xfs_dir2_sf_entry_t	*sfep;		/* new sf entry */
 	xfs_dir2_sf_hdr_t	*sfp;		/* new sf directory */
 
 	trace_xfs_dir2_sf_toino8(args);
@@ -1243,22 +1250,7 @@ xfs_dir2_sf_toino8(
 	sfp->count = oldsfp->count;
 	sfp->i8count = 1;
 	xfs_dir2_sf_put_parent_ino(sfp, xfs_dir2_sf_get_parent_ino(oldsfp));
-	/*
-	 * Copy the entries field by field.
-	 */
-	for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp),
-		    oldsfep = xfs_dir2_sf_firstentry(oldsfp);
-	     i < sfp->count;
-	     i++, sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep),
-		  oldsfep = xfs_dir2_sf_nextentry(mp, oldsfp, oldsfep)) {
-		sfep->namelen = oldsfep->namelen;
-		memcpy(sfep->offset, oldsfep->offset, sizeof(sfep->offset));
-		memcpy(sfep->name, oldsfep->name, sfep->namelen);
-		xfs_dir2_sf_put_ino(mp, sfp, sfep,
-				xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep));
-		xfs_dir2_sf_put_ftype(mp, sfep,
-				xfs_dir2_sf_get_ftype(mp, oldsfep));
-	}
+	xfs_dir2_sf_copy_entries(mp, sfp, oldsfp);
 	/*
 	 * Clean up the inode.
 	 */
-- 
2.55.0

Re: [PATCH 1/3] xfs: factor out xfs_dir2_sf_copy_entries helper

[Carlos Maiolino]

On Fri, Oct 02, 2026 at 04:08:09PM -0500, Eric Sandeen wrote:
> xfs_dir2_sf_toino8 and xfs_dir2_sf_toino64 share a dozen or so lines
> of copied code used to move short form directory entries; factor that
> out to eliminate cut and paste.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Looks good.

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

>  fs/xfs/libxfs/xfs_dir2_sf.c | 68 ++++++++++++++++---------------------
>  1 file changed, 30 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 0567cf8b9c1b..a8675a0d574e 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -1124,6 +1124,34 @@ xfs_dir2_sf_replace(
>  	return 0;
>  }
>  
> +static inline void
> +xfs_dir2_sf_copy_entries(
> +	struct xfs_mount	*mp,
> +	struct xfs_dir2_sf_hdr	*sfp,
> +	struct xfs_dir2_sf_hdr	*oldsfp)
> +{
> +	int			i;
> +	struct xfs_dir2_sf_entry *sfep;		/* new sf entry */
> +	struct xfs_dir2_sf_entry *oldsfep;	/* old sf entry */
> +
> +	/*
> +	 * Copy the entries field by field.
> +	 */
> +	for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp),
> +		    oldsfep = xfs_dir2_sf_firstentry(oldsfp);
> +	     i < sfp->count;
> +	     i++, sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep),
> +		  oldsfep = xfs_dir2_sf_nextentry(mp, oldsfp, oldsfep)) {
> +		sfep->namelen = oldsfep->namelen;
> +		memcpy(sfep->offset, oldsfep->offset, sizeof(sfep->offset));
> +		memcpy(sfep->name, oldsfep->name, sfep->namelen);
> +		xfs_dir2_sf_put_ino(mp, sfp, sfep,
> +				xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep));
> +		xfs_dir2_sf_put_ftype(mp, sfep,
> +				xfs_dir2_sf_get_ftype(mp, oldsfep));
> +	}
> +}
> +
>  /*
>   * Convert from 8-byte inode numbers to 4-byte inode numbers.
>   * The last 8-byte inode number is gone, but the count is still 1.
> @@ -1136,11 +1164,8 @@ xfs_dir2_sf_toino4(
>  	struct xfs_mount	*mp = dp->i_mount;
>  	struct xfs_dir2_sf_hdr	*oldsfp = dp->i_df.if_data;
>  	char			*buf;		/* old dir's buffer */
> -	int			i;		/* entry index */
>  	int			newsize;	/* new inode size */
> -	xfs_dir2_sf_entry_t	*oldsfep;	/* old sf entry */
>  	int			oldsize;	/* old inode size */
> -	xfs_dir2_sf_entry_t	*sfep;		/* new sf entry */
>  	xfs_dir2_sf_hdr_t	*sfp;		/* new sf directory */
>  
>  	trace_xfs_dir2_sf_toino4(args);
> @@ -1171,22 +1196,7 @@ xfs_dir2_sf_toino4(
>  	sfp->count = oldsfp->count;
>  	sfp->i8count = 0;
>  	xfs_dir2_sf_put_parent_ino(sfp, xfs_dir2_sf_get_parent_ino(oldsfp));
> -	/*
> -	 * Copy the entries field by field.
> -	 */
> -	for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp),
> -		    oldsfep = xfs_dir2_sf_firstentry(oldsfp);
> -	     i < sfp->count;
> -	     i++, sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep),
> -		  oldsfep = xfs_dir2_sf_nextentry(mp, oldsfp, oldsfep)) {
> -		sfep->namelen = oldsfep->namelen;
> -		memcpy(sfep->offset, oldsfep->offset, sizeof(sfep->offset));
> -		memcpy(sfep->name, oldsfep->name, sfep->namelen);
> -		xfs_dir2_sf_put_ino(mp, sfp, sfep,
> -				xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep));
> -		xfs_dir2_sf_put_ftype(mp, sfep,
> -				xfs_dir2_sf_get_ftype(mp, oldsfep));
> -	}
> +	xfs_dir2_sf_copy_entries(mp, sfp, oldsfp);
>  	/*
>  	 * Clean up the inode.
>  	 */
> @@ -1208,11 +1218,8 @@ xfs_dir2_sf_toino8(
>  	struct xfs_mount	*mp = dp->i_mount;
>  	struct xfs_dir2_sf_hdr	*oldsfp = dp->i_df.if_data;
>  	char			*buf;		/* old dir's buffer */
> -	int			i;		/* entry index */
>  	int			newsize;	/* new inode size */
> -	xfs_dir2_sf_entry_t	*oldsfep;	/* old sf entry */
>  	int			oldsize;	/* old inode size */
> -	xfs_dir2_sf_entry_t	*sfep;		/* new sf entry */
>  	xfs_dir2_sf_hdr_t	*sfp;		/* new sf directory */
>  
>  	trace_xfs_dir2_sf_toino8(args);
> @@ -1243,22 +1250,7 @@ xfs_dir2_sf_toino8(
>  	sfp->count = oldsfp->count;
>  	sfp->i8count = 1;
>  	xfs_dir2_sf_put_parent_ino(sfp, xfs_dir2_sf_get_parent_ino(oldsfp));
> -	/*
> -	 * Copy the entries field by field.
> -	 */
> -	for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp),
> -		    oldsfep = xfs_dir2_sf_firstentry(oldsfp);
> -	     i < sfp->count;
> -	     i++, sfep = xfs_dir2_sf_nextentry(mp, sfp, sfep),
> -		  oldsfep = xfs_dir2_sf_nextentry(mp, oldsfp, oldsfep)) {
> -		sfep->namelen = oldsfep->namelen;
> -		memcpy(sfep->offset, oldsfep->offset, sizeof(sfep->offset));
> -		memcpy(sfep->name, oldsfep->name, sfep->namelen);
> -		xfs_dir2_sf_put_ino(mp, sfp, sfep,
> -				xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep));
> -		xfs_dir2_sf_put_ftype(mp, sfep,
> -				xfs_dir2_sf_get_ftype(mp, oldsfep));
> -	}
> +	xfs_dir2_sf_copy_entries(mp, sfp, oldsfp);
>  	/*
>  	 * Clean up the inode.
>  	 */
> -- 
> 2.55.0
> 
> 

Re: [PATCH 1/3] xfs: factor out xfs_dir2_sf_copy_entries helper

[Christoph Hellwig]

On Fri, Oct 02, 2026 at 04:08:09PM -0500, Eric Sandeen wrote:
> xfs_dir2_sf_toino8 and xfs_dir2_sf_toino64 share a dozen or so lines
> of copied code used to move short form directory entries; factor that
> out to eliminate cut and paste.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Eric Sandeen]

The open-coded 8-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>
---
 fs/xfs/scrub/attr_repair.c    | 12 +-----------
 fs/xfs/scrub/dir_repair.c     | 11 +----------
 fs/xfs/scrub/repair.c         | 16 ++++++++++++++++
 fs/xfs/scrub/repair.h         |  1 +
 fs/xfs/scrub/symlink_repair.c | 11 +----------
 5 files changed, 20 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/scrub/attr_repair.c b/fs/xfs/scrub/attr_repair.c
index 28f92e9ba72b..08afb522cd5e 100644
--- a/fs/xfs/scrub/attr_repair.c
+++ b/fs/xfs/scrub/attr_repair.c
@@ -1317,17 +1317,7 @@ xrep_xattr_swap_prep(
 	 * 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;
-
+		xrep_reset_fork_to_extents(sc, XFS_ATTR_FORK);
 		xfs_trans_log_inode(sc->tp, sc->ip,
 				XFS_ILOG_CORE | XFS_ILOG_ADATA);
 	}
diff --git a/fs/xfs/scrub/dir_repair.c b/fs/xfs/scrub/dir_repair.c
index 2cfcf1c35679..2d04c5ae63a7 100644
--- a/fs/xfs/scrub/dir_repair.c
+++ b/fs/xfs/scrub/dir_repair.c
@@ -1511,16 +1511,7 @@ xrep_dir_swap_prep(
 	 * 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;
-
+		xrep_reset_fork_to_extents(sc, XFS_DATA_FORK);
 		xfs_trans_log_inode(sc->tp, sc->ip,
 				XFS_ILOG_CORE | XFS_ILOG_DDATA);
 	}
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index c2a437416227..20b82c7e72b8 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -882,6 +882,22 @@ 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);
+
+	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;
+}
+
 /*
  * 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..5f5bc5211600 100644
--- a/fs/xfs/scrub/symlink_repair.c
+++ b/fs/xfs/scrub/symlink_repair.c
@@ -304,16 +304,7 @@ xrep_symlink_swap_prep(
 	 * 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;
-
+		xrep_reset_fork_to_extents(sc, XFS_DATA_FORK);
 		xfs_trans_log_inode(sc->tp, sc->ip,
 				XFS_ILOG_CORE | XFS_ILOG_DDATA);
 	}
-- 
2.55.0

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

[Carlos Maiolino]

On Fri, Oct 02, 2026 at 04:08:10PM -0500, Eric Sandeen wrote:
> The open-coded 8-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.
> 
> +/* 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);
> +
> +	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;
> +}

Why not also move xfs_trans_log_inode() here?
Sure it will need to use a different flag depending on the fork type,
but then the whole reset and log will be contained within there.



> +
>  /*
>   * 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..5f5bc5211600 100644
> --- a/fs/xfs/scrub/symlink_repair.c
> +++ b/fs/xfs/scrub/symlink_repair.c
> @@ -304,16 +304,7 @@ xrep_symlink_swap_prep(
>  	 * 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;
> -
> +		xrep_reset_fork_to_extents(sc, XFS_DATA_FORK);
>  		xfs_trans_log_inode(sc->tp, sc->ip,
>  				XFS_ILOG_CORE | XFS_ILOG_DDATA);
>  	}
> -- 
> 2.55.0
> 
> 

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

[Eric Sandeen]

On 10/5/26 8:07 AM, Carlos Maiolino wrote:
> On Fri, Oct 02, 2026 at 04:08:10PM -0500, Eric Sandeen wrote:
>> The open-coded 8-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.
>>
>> +/* 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);
>> +
>> +	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;
>> +}
> 
> Why not also move xfs_trans_log_inode() here?
> Sure it will need to use a different flag depending on the fork type,
> but then the whole reset and log will be contained within there.
> 
Hm yeah, could do. I don't know if hiding the logging in the helper obfuscates
things or not. Any others have thoughts? (I can see if there's precedent for
whether logging should happen in the main flow or in a helper elsewhere, too.)

Thanks,
-Eric

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

[Eric Sandeen]

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;
+
+	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

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

[Darrick J. Wong]

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

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

[Eric Sandeen]

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?
> 	}
> 
> 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
>>
>>
> 

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

[Darrick J. Wong]

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. :)

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

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

[Carlos Maiolino]

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

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

[Christoph Hellwig]

On Tue, Oct 06, 2026 at 10:32:44AM +0200, Carlos Maiolino wrote:
> > There shouldn't be any, I just thought it looks cleaner. :)
> 
> Perhaps...
> 
> ilog_flags |= (whichfork == XFS_DATA_FORK ? XFS_ILOG_DDATA : XFS_ILOG_ADATA)
> 
> ?

Urgg.

> But the switch indeed looks nicer.

Agreed, switch over if over ternary operator.

But either version is rechnically correct, so for whichever version
lands:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Eric Sandeen]

The open-coded 8-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>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---

V3: Ok, ok, fine - it's a switch now. Merge whatever you feel is best. :)
    This V3 change is compile-tested only.

 fs/xfs/scrub/attr_repair.c    | 17 ++---------------
 fs/xfs/scrub/dir_repair.c     | 16 ++--------------
 fs/xfs/scrub/repair.c         | 31 +++++++++++++++++++++++++++++++
 fs/xfs/scrub/repair.h         |  1 +
 fs/xfs/scrub/symlink_repair.c | 16 ++--------------
 5 files changed, 38 insertions(+), 43 deletions(-)

diff --git a/fs/xfs/scrub/attr_repair.c b/fs/xfs/scrub/attr_repair.c
index 387ad909e20b..46e6b2fe109a 100644
--- a/fs/xfs/scrub/attr_repair.c
+++ b/fs/xfs/scrub/attr_repair.c
@@ -1319,21 +1319,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 956aa75218aa..dd00d4ed8c17 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -882,6 +882,37 @@ 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;
+
+	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;
+	}
+
+	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

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

[Carlos Maiolino]

On Wed, Oct 07, 2026 at 10:25:21AM -0500, Eric Sandeen wrote:
> The open-coded 8-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>
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> 
> V3: Ok, ok, fine - it's a switch now. Merge whatever you feel is best. :)
>     This V3 change is compile-tested only.

Looks good,
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> 
>  fs/xfs/scrub/attr_repair.c    | 17 ++---------------
>  fs/xfs/scrub/dir_repair.c     | 16 ++--------------
>  fs/xfs/scrub/repair.c         | 31 +++++++++++++++++++++++++++++++
>  fs/xfs/scrub/repair.h         |  1 +
>  fs/xfs/scrub/symlink_repair.c | 16 ++--------------
>  5 files changed, 38 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/xfs/scrub/attr_repair.c b/fs/xfs/scrub/attr_repair.c
> index 387ad909e20b..46e6b2fe109a 100644
> --- a/fs/xfs/scrub/attr_repair.c
> +++ b/fs/xfs/scrub/attr_repair.c
> @@ -1319,21 +1319,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 956aa75218aa..dd00d4ed8c17 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -882,6 +882,37 @@ 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;
> +
> +	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;
> +	}
> +
> +	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
> 
> 

[PATCH 3/3] xfs: re-use refcount scrub/repair code for rtrefcount

[Eric Sandeen]

Some code in scrub/rtrefcount.c is copied from scrub/refcount.c; it
differs only in function & structure names and a couple types and
macros, which share the same underlying types and values.

Similarly for code in fs/xfs/scrub/[rt]refcount_repair.c

Export the refcount versions, use them for rtrefcount scrubbing,
eliminate the copies, and add build-time asserts so that the type/value
matches mentioned above do not drift.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/scrub/refcount.c          |  27 +---
 fs/xfs/scrub/refcount.h          |  56 +++++++
 fs/xfs/scrub/refcount_repair.c   |   3 +-
 fs/xfs/scrub/rtrefcount.c        | 263 +------------------------------
 fs/xfs/scrub/rtrefcount_repair.c |  39 +----
 5 files changed, 72 insertions(+), 316 deletions(-)
 create mode 100644 fs/xfs/scrub/refcount.h

diff --git a/fs/xfs/scrub/refcount.c b/fs/xfs/scrub/refcount.c
index f8c51d8fbb3d..c18bb89325a5 100644
--- a/fs/xfs/scrub/refcount.c
+++ b/fs/xfs/scrub/refcount.c
@@ -20,6 +20,7 @@
 #include "scrub/btree.h"
 #include "scrub/trace.h"
 #include "scrub/repair.h"
+#include "scrub/refcount.h"
 
 /*
  * Set us up to scrub reference count btrees.
@@ -80,24 +81,6 @@ xchk_setup_ag_refcountbt(
  * If the refcount is correct, all the check conditions in the algorithm
  * should always hold true.  If not, the refcount is incorrect.
  */
-struct xchk_refcnt_frag {
-	struct list_head	list;
-	struct xfs_rmap_irec	rm;
-};
-
-struct xchk_refcnt_check {
-	struct xfs_scrub	*sc;
-	struct list_head	fragments;
-
-	/* refcount extent we're examining */
-	xfs_agblock_t		bno;
-	xfs_extlen_t		len;
-	xfs_nlink_t		refcount;
-
-	/* number of owners seen */
-	xfs_nlink_t		seen;
-};
-
 /*
  * Decide if the given rmap is large enough that we can redeem it
  * towards refcount verification now, or if it's a fragment, in
@@ -105,7 +88,7 @@ struct xchk_refcnt_check {
  * discover that we've collected exactly the correct number of
  * fragments as the refcountbt says we should have.
  */
-STATIC int
+int
 xchk_refcountbt_rmap_check(
 	struct xfs_btree_cur		*cur,
 	const struct xfs_rmap_irec	*rec,
@@ -159,7 +142,7 @@ xchk_refcountbt_rmap_check(
  * number of extents that totally covered the refcountbt extent),
  * we have a refcountbt error.
  */
-STATIC void
+void
 xchk_refcountbt_process_rmap_fragments(
 	struct xchk_refcnt_check	*refchk)
 {
@@ -290,7 +273,7 @@ xchk_refcountbt_xref_rmap(
 		.bno			= irec->rc_startblock,
 		.len			= irec->rc_blockcount,
 		.refcount		= irec->rc_refcount,
-		.seen = 0,
+		.seen			= 0,
 	};
 	struct xfs_rmap_irec		low;
 	struct xfs_rmap_irec		high;
@@ -355,7 +338,7 @@ struct xchk_refcbt_records {
 	enum xfs_refc_domain	prev_domain;
 };
 
-STATIC int
+int
 xchk_refcountbt_rmap_check_gap(
 	struct xfs_btree_cur		*cur,
 	const struct xfs_rmap_irec	*rec,
diff --git a/fs/xfs/scrub/refcount.h b/fs/xfs/scrub/refcount.h
new file mode 100644
index 000000000000..0706afd9f80b
--- /dev/null
+++ b/fs/xfs/scrub/refcount.h
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2017-2023 Oracle.  All Rights Reserved.
+ * Author: Darrick J. Wong <djwong@kernel.org>
+ */
+#ifndef __XFS_SCRUB_REFCOUNT_H__
+#define __XFS_SCRUB_REFCOUNT_H__
+
+/*
+ * Ensure that types and macros match across rt and non-rt variants,
+ * as this code is shared for both.
+ */
+static_assert(__same_type(xfs_agblock_t, xfs_rgblock_t));
+static_assert(NULLAGBLOCK == NULLRGBLOCK);
+
+/*
+ * Confirming reference counts via reverse mappings.
+ *
+ * These helpers are shared by the AG (refcount.c) and realtime
+ * (rtrefcount.c) refcount scrubbers.  Both the rmap and refcount incore
+ * records use xfs_agblock_t for their start block regardless of whether the
+ * data lives in an allocation group or a realtime group, so the fragment
+ * bookkeeping here uses xfs_agblock_t for group-relative block numbers.
+ */
+struct xchk_refcnt_frag {
+	struct list_head	list;
+	struct xfs_rmap_irec	rm;
+};
+
+struct xchk_refcnt_check {
+	struct xfs_scrub	*sc;
+	struct list_head	fragments;
+
+	/* refcount extent we're examining */
+	xfs_agblock_t		bno;
+	xfs_extlen_t		len;
+	xfs_nlink_t		refcount;
+
+	/* number of owners seen */
+	xfs_nlink_t		seen;
+};
+
+int xchk_refcountbt_rmap_check(struct xfs_btree_cur *cur,
+		const struct xfs_rmap_irec *rec, void *priv);
+void xchk_refcountbt_process_rmap_fragments(struct xchk_refcnt_check *refchk);
+int xchk_refcountbt_rmap_check_gap(struct xfs_btree_cur *cur,
+		const struct xfs_rmap_irec *rec, void *priv);
+
+/*
+ * Compare two refcount records so that both the AG (refcount_repair.c) and
+ * realtime (rtrefcount_repair.c) rebuilders sort staged records into the same
+ * order as the ondisk records.
+ */
+int xrep_refc_extent_cmp(const void *a, const void *b);
+
+#endif /* __XFS_SCRUB_REFCOUNT_H__ */
diff --git a/fs/xfs/scrub/refcount_repair.c b/fs/xfs/scrub/refcount_repair.c
index ca9c382005ff..72de263117a7 100644
--- a/fs/xfs/scrub/refcount_repair.c
+++ b/fs/xfs/scrub/refcount_repair.c
@@ -39,6 +39,7 @@
 #include "scrub/newbt.h"
 #include "scrub/reap.h"
 #include "scrub/rcbag.h"
+#include "scrub/refcount.h"
 
 /*
  * Rebuilding the Reference Count Btree
@@ -295,7 +296,7 @@ xrep_refc_encode_startblock(
 }
 
 /* Sort in the same order as the ondisk records. */
-static int
+int
 xrep_refc_extent_cmp(
 	const void			*a,
 	const void			*b)
diff --git a/fs/xfs/scrub/rtrefcount.c b/fs/xfs/scrub/rtrefcount.c
index 460e3f094adf..b78784051308 100644
--- a/fs/xfs/scrub/rtrefcount.c
+++ b/fs/xfs/scrub/rtrefcount.c
@@ -25,6 +25,7 @@
 #include "scrub/common.h"
 #include "scrub/btree.h"
 #include "scrub/repair.h"
+#include "scrub/refcount.h"
 
 /* Set us up with the realtime refcount metadata locked. */
 int
@@ -59,248 +60,13 @@ xchk_setup_rtrefcountbt(
 
 /* Realtime Reference count btree scrubber. */
 
-/*
- * Confirming Reference Counts via Reverse Mappings
- *
- * We want to count the reverse mappings overlapping a refcount record
- * (bno, len, refcount), allowing for the possibility that some of the
- * overlap may come from smaller adjoining reverse mappings, while some
- * comes from single extents which overlap the range entirely.  The
- * outer loop is as follows:
- *
- * 1. For all reverse mappings overlapping the refcount extent,
- *    a. If a given rmap completely overlaps, mark it as seen.
- *    b. Otherwise, record the fragment (in agbno order) for later
- *       processing.
- *
- * Once we've seen all the rmaps, we know that for all blocks in the
- * refcount record we want to find $refcount owners and we've already
- * visited $seen extents that overlap all the blocks.  Therefore, we
- * need to find ($refcount - $seen) owners for every block in the
- * extent; call that quantity $target_nr.  Proceed as follows:
- *
- * 2. Pull the first $target_nr fragments from the list; all of them
- *    should start at or before the start of the extent.
- *    Call this subset of fragments the working set.
- * 3. Until there are no more unprocessed fragments,
- *    a. Find the shortest fragments in the set and remove them.
- *    b. Note the block number of the end of these fragments.
- *    c. Pull the same number of fragments from the list.  All of these
- *       fragments should start at the block number recorded in the
- *       previous step.
- *    d. Put those fragments in the set.
- * 4. Check that there are $target_nr fragments remaining in the list,
- *    and that they all end at or beyond the end of the refcount extent.
- *
- * If the refcount is correct, all the check conditions in the algorithm
- * should always hold true.  If not, the refcount is incorrect.
- */
-struct xchk_rtrefcnt_frag {
-	struct list_head	list;
-	struct xfs_rmap_irec	rm;
-};
-
-struct xchk_rtrefcnt_check {
-	struct xfs_scrub	*sc;
-	struct list_head	fragments;
-
-	/* refcount extent we're examining */
-	xfs_rgblock_t		bno;
-	xfs_extlen_t		len;
-	xfs_nlink_t		refcount;
-
-	/* number of owners seen */
-	xfs_nlink_t		seen;
-};
-
-/*
- * Decide if the given rmap is large enough that we can redeem it
- * towards refcount verification now, or if it's a fragment, in
- * which case we'll hang onto it in the hopes that we'll later
- * discover that we've collected exactly the correct number of
- * fragments as the rtrefcountbt says we should have.
- */
-STATIC int
-xchk_rtrefcountbt_rmap_check(
-	struct xfs_btree_cur		*cur,
-	const struct xfs_rmap_irec	*rec,
-	void				*priv)
-{
-	struct xchk_rtrefcnt_check	*refchk = priv;
-	struct xchk_rtrefcnt_frag	*frag;
-	xfs_rgblock_t			rm_last;
-	xfs_rgblock_t			rc_last;
-	int				error = 0;
-
-	if (xchk_should_terminate(refchk->sc, &error))
-		return error;
-
-	rm_last = rec->rm_startblock + rec->rm_blockcount - 1;
-	rc_last = refchk->bno + refchk->len - 1;
-
-	/* Confirm that a single-owner refc extent is a CoW stage. */
-	if (refchk->refcount == 1 && rec->rm_owner != XFS_RMAP_OWN_COW) {
-		xchk_btree_xref_set_corrupt(refchk->sc, cur, 0);
-		return 0;
-	}
-
-	if (rec->rm_startblock <= refchk->bno && rm_last >= rc_last) {
-		/*
-		 * The rmap overlaps the refcount record, so we can confirm
-		 * one refcount owner seen.
-		 */
-		refchk->seen++;
-	} else {
-		/*
-		 * This rmap covers only part of the refcount record, so
-		 * save the fragment for later processing.  If the rmapbt
-		 * is healthy each rmap_irec we see will be in agbno order
-		 * so we don't need insertion sort here.
-		 */
-		frag = kmalloc_obj(struct xchk_rtrefcnt_frag, XCHK_GFP_FLAGS);
-		if (!frag)
-			return -ENOMEM;
-		memcpy(&frag->rm, rec, sizeof(frag->rm));
-		list_add_tail(&frag->list, &refchk->fragments);
-	}
-
-	return 0;
-}
-
-/*
- * Given a bunch of rmap fragments, iterate through them, keeping
- * a running tally of the refcount.  If this ever deviates from
- * what we expect (which is the rtrefcountbt's refcount minus the
- * number of extents that totally covered the rtrefcountbt extent),
- * we have a rtrefcountbt error.
- */
-STATIC void
-xchk_rtrefcountbt_process_rmap_fragments(
-	struct xchk_rtrefcnt_check	*refchk)
-{
-	struct list_head		worklist;
-	struct xchk_rtrefcnt_frag	*frag;
-	struct xchk_rtrefcnt_frag	*n;
-	xfs_rgblock_t			bno;
-	xfs_rgblock_t			rbno;
-	xfs_rgblock_t			next_rbno;
-	xfs_nlink_t			nr;
-	xfs_nlink_t			target_nr;
-
-	target_nr = refchk->refcount - refchk->seen;
-	if (target_nr == 0)
-		return;
-
-	/*
-	 * There are (refchk->rc.rc_refcount - refchk->nr refcount)
-	 * references we haven't found yet.  Pull that many off the
-	 * fragment list and figure out where the smallest rmap ends
-	 * (and therefore the next rmap should start).  All the rmaps
-	 * we pull off should start at or before the beginning of the
-	 * refcount record's range.
-	 */
-	INIT_LIST_HEAD(&worklist);
-	rbno = NULLRGBLOCK;
-
-	/* Make sure the fragments actually /are/ in bno order. */
-	bno = 0;
-	list_for_each_entry(frag, &refchk->fragments, list) {
-		if (frag->rm.rm_startblock < bno)
-			goto done;
-		bno = frag->rm.rm_startblock;
-	}
-
-	/*
-	 * Find all the rmaps that start at or before the refc extent,
-	 * and put them on the worklist.
-	 */
-	nr = 0;
-	list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
-		if (frag->rm.rm_startblock > refchk->bno || nr > target_nr)
-			break;
-		bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
-		if (bno < rbno)
-			rbno = bno;
-		list_move_tail(&frag->list, &worklist);
-		nr++;
-	}
-
-	/*
-	 * We should have found exactly $target_nr rmap fragments starting
-	 * at or before the refcount extent.
-	 */
-	if (nr != target_nr)
-		goto done;
-
-	while (!list_empty(&refchk->fragments)) {
-		/* Discard any fragments ending at rbno from the worklist. */
-		nr = 0;
-		next_rbno = NULLRGBLOCK;
-		list_for_each_entry_safe(frag, n, &worklist, list) {
-			bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
-			if (bno != rbno) {
-				if (bno < next_rbno)
-					next_rbno = bno;
-				continue;
-			}
-			list_del(&frag->list);
-			kfree(frag);
-			nr++;
-		}
-
-		/* Try to add nr rmaps starting at rbno to the worklist. */
-		list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
-			bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
-			if (frag->rm.rm_startblock != rbno)
-				goto done;
-			list_move_tail(&frag->list, &worklist);
-			if (next_rbno > bno)
-				next_rbno = bno;
-			nr--;
-			if (nr == 0)
-				break;
-		}
-
-		/*
-		 * If we get here and nr > 0, this means that we added fewer
-		 * items to the worklist than we discarded because the fragment
-		 * list ran out of items.  Therefore, we cannot maintain the
-		 * required refcount.  Something is wrong, so we're done.
-		 */
-		if (nr)
-			goto done;
-
-		rbno = next_rbno;
-	}
-
-	/*
-	 * Make sure the last extent we processed ends at or beyond
-	 * the end of the refcount extent.
-	 */
-	if (rbno < refchk->bno + refchk->len)
-		goto done;
-
-	/* Actually record us having seen the remaining refcount. */
-	refchk->seen = refchk->refcount;
-done:
-	/* Delete fragments and work list. */
-	list_for_each_entry_safe(frag, n, &worklist, list) {
-		list_del(&frag->list);
-		kfree(frag);
-	}
-	list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
-		list_del(&frag->list);
-		kfree(frag);
-	}
-}
-
 /* Use the rmap entries covering this extent to verify the refcount. */
 STATIC void
 xchk_rtrefcountbt_xref_rmap(
 	struct xfs_scrub		*sc,
 	const struct xfs_refcount_irec	*irec)
 {
-	struct xchk_rtrefcnt_check	refchk = {
+	struct xchk_refcnt_check	refchk = {
 		.sc			= sc,
 		.bno			= irec->rc_startblock,
 		.len			= irec->rc_blockcount,
@@ -309,8 +75,8 @@ xchk_rtrefcountbt_xref_rmap(
 	};
 	struct xfs_rmap_irec		low;
 	struct xfs_rmap_irec		high;
-	struct xchk_rtrefcnt_frag	*frag;
-	struct xchk_rtrefcnt_frag	*n;
+	struct xchk_refcnt_frag		*frag;
+	struct xchk_refcnt_frag		*n;
 	int				error;
 
 	if (!sc->sr.rmap_cur || xchk_skip_xref(sc->sm))
@@ -324,11 +90,11 @@ xchk_rtrefcountbt_xref_rmap(
 
 	INIT_LIST_HEAD(&refchk.fragments);
 	error = xfs_rmap_query_range(sc->sr.rmap_cur, &low, &high,
-			xchk_rtrefcountbt_rmap_check, &refchk);
+			xchk_refcountbt_rmap_check, &refchk);
 	if (!xchk_should_check_xref(sc, &error, &sc->sr.rmap_cur))
 		goto out_free;
 
-	xchk_rtrefcountbt_process_rmap_fragments(&refchk);
+	xchk_refcountbt_process_rmap_fragments(&refchk);
 	if (irec->rc_refcount != refchk.seen)
 		xchk_btree_xref_set_corrupt(sc, sc->sr.rmap_cur, 0);
 
@@ -408,21 +174,6 @@ xchk_rtrefcountbt_check_mergeable(
 	memcpy(&rrc->prev_rec, irec, sizeof(struct xfs_refcount_irec));
 }
 
-STATIC int
-xchk_rtrefcountbt_rmap_check_gap(
-	struct xfs_btree_cur		*cur,
-	const struct xfs_rmap_irec	*rec,
-	void				*priv)
-{
-	xfs_rgblock_t			*next_bno = priv;
-
-	if (*next_bno != NULLRGBLOCK && rec->rm_startblock < *next_bno)
-		return -ECANCELED;
-
-	*next_bno = rec->rm_startblock + rec->rm_blockcount;
-	return 0;
-}
-
 /*
  * Make sure that a gap in the reference count records does not correspond to
  * overlapping records (i.e. shared extents) in the reverse mappings.
@@ -448,7 +199,7 @@ xchk_rtrefcountbt_xref_gaps(
 	high.rm_startblock = bno - 1;
 
 	error = xfs_rmap_query_range(sc->sr.rmap_cur, &low, &high,
-			xchk_rtrefcountbt_rmap_check_gap, &next_bno);
+			xchk_refcountbt_rmap_check_gap, &next_bno);
 	if (error == -ECANCELED)
 		xchk_btree_xref_set_corrupt(sc, sc->sr.rmap_cur, 0);
 	else
diff --git a/fs/xfs/scrub/rtrefcount_repair.c b/fs/xfs/scrub/rtrefcount_repair.c
index c78a6d2990c5..5ca9959dfacc 100644
--- a/fs/xfs/scrub/rtrefcount_repair.c
+++ b/fs/xfs/scrub/rtrefcount_repair.c
@@ -44,6 +44,7 @@
 #include "scrub/newbt.h"
 #include "scrub/reap.h"
 #include "scrub/rcbag.h"
+#include "scrub/refcount.h"
 
 /*
  * Rebuilding the Reference Count Btree
@@ -266,42 +267,6 @@ xrep_rtrefc_walk_rmaps(
 	return 0;
 }
 
-static inline uint32_t
-xrep_rtrefc_encode_startblock(
-	const struct xfs_refcount_irec	*irec)
-{
-	uint32_t			start;
-
-	start = irec->rc_startblock & ~XFS_REFC_COWFLAG;
-	if (irec->rc_domain == XFS_REFC_DOMAIN_COW)
-		start |= XFS_REFC_COWFLAG;
-
-	return start;
-}
-
-/*
- * Compare two refcount records.  We want to sort in order of increasing block
- * number.
- */
-static int
-xrep_rtrefc_extent_cmp(
-	const void			*a,
-	const void			*b)
-{
-	const struct xfs_refcount_irec	*ap = a;
-	const struct xfs_refcount_irec	*bp = b;
-	uint32_t			sa, sb;
-
-	sa = xrep_rtrefc_encode_startblock(ap);
-	sb = xrep_rtrefc_encode_startblock(bp);
-
-	if (sa > sb)
-		return 1;
-	if (sa < sb)
-		return -1;
-	return 0;
-}
-
 /*
  * Sort the refcount extents by startblock or else the btree records will be in
  * the wrong order.  Make sure the records do not overlap in physical space.
@@ -316,7 +281,7 @@ xrep_rtrefc_sort_records(
 	xfs_rgblock_t			next_rgbno = 0;
 	int				error;
 
-	error = xfarray_sort(rr->refcount_records, xrep_rtrefc_extent_cmp,
+	error = xfarray_sort(rr->refcount_records, xrep_refc_extent_cmp,
 			XFARRAY_SORT_KILLABLE);
 	if (error)
 		return error;
-- 
2.55.0

Re: [PATCH 3/3] xfs: re-use refcount scrub/repair code for rtrefcount

[Darrick J. Wong]

On Fri, Oct 02, 2026 at 04:08:11PM -0500, Eric Sandeen wrote:
> Some code in scrub/rtrefcount.c is copied from scrub/refcount.c; it
> differs only in function & structure names and a couple types and
> macros, which share the same underlying types and values.
> 
> Similarly for code in fs/xfs/scrub/[rt]refcount_repair.c
> 
> Export the refcount versions, use them for rtrefcount scrubbing,
> eliminate the copies, and add build-time asserts so that the type/value
> matches mentioned above do not drift.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

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

--D

> ---
>  fs/xfs/scrub/refcount.c          |  27 +---
>  fs/xfs/scrub/refcount.h          |  56 +++++++
>  fs/xfs/scrub/refcount_repair.c   |   3 +-
>  fs/xfs/scrub/rtrefcount.c        | 263 +------------------------------
>  fs/xfs/scrub/rtrefcount_repair.c |  39 +----
>  5 files changed, 72 insertions(+), 316 deletions(-)
>  create mode 100644 fs/xfs/scrub/refcount.h
> 
> diff --git a/fs/xfs/scrub/refcount.c b/fs/xfs/scrub/refcount.c
> index f8c51d8fbb3d..c18bb89325a5 100644
> --- a/fs/xfs/scrub/refcount.c
> +++ b/fs/xfs/scrub/refcount.c
> @@ -20,6 +20,7 @@
>  #include "scrub/btree.h"
>  #include "scrub/trace.h"
>  #include "scrub/repair.h"
> +#include "scrub/refcount.h"
>  
>  /*
>   * Set us up to scrub reference count btrees.
> @@ -80,24 +81,6 @@ xchk_setup_ag_refcountbt(
>   * If the refcount is correct, all the check conditions in the algorithm
>   * should always hold true.  If not, the refcount is incorrect.
>   */
> -struct xchk_refcnt_frag {
> -	struct list_head	list;
> -	struct xfs_rmap_irec	rm;
> -};
> -
> -struct xchk_refcnt_check {
> -	struct xfs_scrub	*sc;
> -	struct list_head	fragments;
> -
> -	/* refcount extent we're examining */
> -	xfs_agblock_t		bno;
> -	xfs_extlen_t		len;
> -	xfs_nlink_t		refcount;
> -
> -	/* number of owners seen */
> -	xfs_nlink_t		seen;
> -};
> -
>  /*
>   * Decide if the given rmap is large enough that we can redeem it
>   * towards refcount verification now, or if it's a fragment, in
> @@ -105,7 +88,7 @@ struct xchk_refcnt_check {
>   * discover that we've collected exactly the correct number of
>   * fragments as the refcountbt says we should have.
>   */
> -STATIC int
> +int
>  xchk_refcountbt_rmap_check(
>  	struct xfs_btree_cur		*cur,
>  	const struct xfs_rmap_irec	*rec,
> @@ -159,7 +142,7 @@ xchk_refcountbt_rmap_check(
>   * number of extents that totally covered the refcountbt extent),
>   * we have a refcountbt error.
>   */
> -STATIC void
> +void
>  xchk_refcountbt_process_rmap_fragments(
>  	struct xchk_refcnt_check	*refchk)
>  {
> @@ -290,7 +273,7 @@ xchk_refcountbt_xref_rmap(
>  		.bno			= irec->rc_startblock,
>  		.len			= irec->rc_blockcount,
>  		.refcount		= irec->rc_refcount,
> -		.seen = 0,
> +		.seen			= 0,
>  	};
>  	struct xfs_rmap_irec		low;
>  	struct xfs_rmap_irec		high;
> @@ -355,7 +338,7 @@ struct xchk_refcbt_records {
>  	enum xfs_refc_domain	prev_domain;
>  };
>  
> -STATIC int
> +int
>  xchk_refcountbt_rmap_check_gap(
>  	struct xfs_btree_cur		*cur,
>  	const struct xfs_rmap_irec	*rec,
> diff --git a/fs/xfs/scrub/refcount.h b/fs/xfs/scrub/refcount.h
> new file mode 100644
> index 000000000000..0706afd9f80b
> --- /dev/null
> +++ b/fs/xfs/scrub/refcount.h
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2017-2023 Oracle.  All Rights Reserved.
> + * Author: Darrick J. Wong <djwong@kernel.org>
> + */
> +#ifndef __XFS_SCRUB_REFCOUNT_H__
> +#define __XFS_SCRUB_REFCOUNT_H__
> +
> +/*
> + * Ensure that types and macros match across rt and non-rt variants,
> + * as this code is shared for both.
> + */
> +static_assert(__same_type(xfs_agblock_t, xfs_rgblock_t));
> +static_assert(NULLAGBLOCK == NULLRGBLOCK);
> +
> +/*
> + * Confirming reference counts via reverse mappings.
> + *
> + * These helpers are shared by the AG (refcount.c) and realtime
> + * (rtrefcount.c) refcount scrubbers.  Both the rmap and refcount incore
> + * records use xfs_agblock_t for their start block regardless of whether the
> + * data lives in an allocation group or a realtime group, so the fragment
> + * bookkeeping here uses xfs_agblock_t for group-relative block numbers.
> + */
> +struct xchk_refcnt_frag {
> +	struct list_head	list;
> +	struct xfs_rmap_irec	rm;
> +};
> +
> +struct xchk_refcnt_check {
> +	struct xfs_scrub	*sc;
> +	struct list_head	fragments;
> +
> +	/* refcount extent we're examining */
> +	xfs_agblock_t		bno;
> +	xfs_extlen_t		len;
> +	xfs_nlink_t		refcount;
> +
> +	/* number of owners seen */
> +	xfs_nlink_t		seen;
> +};
> +
> +int xchk_refcountbt_rmap_check(struct xfs_btree_cur *cur,
> +		const struct xfs_rmap_irec *rec, void *priv);
> +void xchk_refcountbt_process_rmap_fragments(struct xchk_refcnt_check *refchk);
> +int xchk_refcountbt_rmap_check_gap(struct xfs_btree_cur *cur,
> +		const struct xfs_rmap_irec *rec, void *priv);
> +
> +/*
> + * Compare two refcount records so that both the AG (refcount_repair.c) and
> + * realtime (rtrefcount_repair.c) rebuilders sort staged records into the same
> + * order as the ondisk records.
> + */
> +int xrep_refc_extent_cmp(const void *a, const void *b);
> +
> +#endif /* __XFS_SCRUB_REFCOUNT_H__ */
> diff --git a/fs/xfs/scrub/refcount_repair.c b/fs/xfs/scrub/refcount_repair.c
> index ca9c382005ff..72de263117a7 100644
> --- a/fs/xfs/scrub/refcount_repair.c
> +++ b/fs/xfs/scrub/refcount_repair.c
> @@ -39,6 +39,7 @@
>  #include "scrub/newbt.h"
>  #include "scrub/reap.h"
>  #include "scrub/rcbag.h"
> +#include "scrub/refcount.h"
>  
>  /*
>   * Rebuilding the Reference Count Btree
> @@ -295,7 +296,7 @@ xrep_refc_encode_startblock(
>  }
>  
>  /* Sort in the same order as the ondisk records. */
> -static int
> +int
>  xrep_refc_extent_cmp(
>  	const void			*a,
>  	const void			*b)
> diff --git a/fs/xfs/scrub/rtrefcount.c b/fs/xfs/scrub/rtrefcount.c
> index 460e3f094adf..b78784051308 100644
> --- a/fs/xfs/scrub/rtrefcount.c
> +++ b/fs/xfs/scrub/rtrefcount.c
> @@ -25,6 +25,7 @@
>  #include "scrub/common.h"
>  #include "scrub/btree.h"
>  #include "scrub/repair.h"
> +#include "scrub/refcount.h"
>  
>  /* Set us up with the realtime refcount metadata locked. */
>  int
> @@ -59,248 +60,13 @@ xchk_setup_rtrefcountbt(
>  
>  /* Realtime Reference count btree scrubber. */
>  
> -/*
> - * Confirming Reference Counts via Reverse Mappings
> - *
> - * We want to count the reverse mappings overlapping a refcount record
> - * (bno, len, refcount), allowing for the possibility that some of the
> - * overlap may come from smaller adjoining reverse mappings, while some
> - * comes from single extents which overlap the range entirely.  The
> - * outer loop is as follows:
> - *
> - * 1. For all reverse mappings overlapping the refcount extent,
> - *    a. If a given rmap completely overlaps, mark it as seen.
> - *    b. Otherwise, record the fragment (in agbno order) for later
> - *       processing.
> - *
> - * Once we've seen all the rmaps, we know that for all blocks in the
> - * refcount record we want to find $refcount owners and we've already
> - * visited $seen extents that overlap all the blocks.  Therefore, we
> - * need to find ($refcount - $seen) owners for every block in the
> - * extent; call that quantity $target_nr.  Proceed as follows:
> - *
> - * 2. Pull the first $target_nr fragments from the list; all of them
> - *    should start at or before the start of the extent.
> - *    Call this subset of fragments the working set.
> - * 3. Until there are no more unprocessed fragments,
> - *    a. Find the shortest fragments in the set and remove them.
> - *    b. Note the block number of the end of these fragments.
> - *    c. Pull the same number of fragments from the list.  All of these
> - *       fragments should start at the block number recorded in the
> - *       previous step.
> - *    d. Put those fragments in the set.
> - * 4. Check that there are $target_nr fragments remaining in the list,
> - *    and that they all end at or beyond the end of the refcount extent.
> - *
> - * If the refcount is correct, all the check conditions in the algorithm
> - * should always hold true.  If not, the refcount is incorrect.
> - */
> -struct xchk_rtrefcnt_frag {
> -	struct list_head	list;
> -	struct xfs_rmap_irec	rm;
> -};
> -
> -struct xchk_rtrefcnt_check {
> -	struct xfs_scrub	*sc;
> -	struct list_head	fragments;
> -
> -	/* refcount extent we're examining */
> -	xfs_rgblock_t		bno;
> -	xfs_extlen_t		len;
> -	xfs_nlink_t		refcount;
> -
> -	/* number of owners seen */
> -	xfs_nlink_t		seen;
> -};
> -
> -/*
> - * Decide if the given rmap is large enough that we can redeem it
> - * towards refcount verification now, or if it's a fragment, in
> - * which case we'll hang onto it in the hopes that we'll later
> - * discover that we've collected exactly the correct number of
> - * fragments as the rtrefcountbt says we should have.
> - */
> -STATIC int
> -xchk_rtrefcountbt_rmap_check(
> -	struct xfs_btree_cur		*cur,
> -	const struct xfs_rmap_irec	*rec,
> -	void				*priv)
> -{
> -	struct xchk_rtrefcnt_check	*refchk = priv;
> -	struct xchk_rtrefcnt_frag	*frag;
> -	xfs_rgblock_t			rm_last;
> -	xfs_rgblock_t			rc_last;
> -	int				error = 0;
> -
> -	if (xchk_should_terminate(refchk->sc, &error))
> -		return error;
> -
> -	rm_last = rec->rm_startblock + rec->rm_blockcount - 1;
> -	rc_last = refchk->bno + refchk->len - 1;
> -
> -	/* Confirm that a single-owner refc extent is a CoW stage. */
> -	if (refchk->refcount == 1 && rec->rm_owner != XFS_RMAP_OWN_COW) {
> -		xchk_btree_xref_set_corrupt(refchk->sc, cur, 0);
> -		return 0;
> -	}
> -
> -	if (rec->rm_startblock <= refchk->bno && rm_last >= rc_last) {
> -		/*
> -		 * The rmap overlaps the refcount record, so we can confirm
> -		 * one refcount owner seen.
> -		 */
> -		refchk->seen++;
> -	} else {
> -		/*
> -		 * This rmap covers only part of the refcount record, so
> -		 * save the fragment for later processing.  If the rmapbt
> -		 * is healthy each rmap_irec we see will be in agbno order
> -		 * so we don't need insertion sort here.
> -		 */
> -		frag = kmalloc_obj(struct xchk_rtrefcnt_frag, XCHK_GFP_FLAGS);
> -		if (!frag)
> -			return -ENOMEM;
> -		memcpy(&frag->rm, rec, sizeof(frag->rm));
> -		list_add_tail(&frag->list, &refchk->fragments);
> -	}
> -
> -	return 0;
> -}
> -
> -/*
> - * Given a bunch of rmap fragments, iterate through them, keeping
> - * a running tally of the refcount.  If this ever deviates from
> - * what we expect (which is the rtrefcountbt's refcount minus the
> - * number of extents that totally covered the rtrefcountbt extent),
> - * we have a rtrefcountbt error.
> - */
> -STATIC void
> -xchk_rtrefcountbt_process_rmap_fragments(
> -	struct xchk_rtrefcnt_check	*refchk)
> -{
> -	struct list_head		worklist;
> -	struct xchk_rtrefcnt_frag	*frag;
> -	struct xchk_rtrefcnt_frag	*n;
> -	xfs_rgblock_t			bno;
> -	xfs_rgblock_t			rbno;
> -	xfs_rgblock_t			next_rbno;
> -	xfs_nlink_t			nr;
> -	xfs_nlink_t			target_nr;
> -
> -	target_nr = refchk->refcount - refchk->seen;
> -	if (target_nr == 0)
> -		return;
> -
> -	/*
> -	 * There are (refchk->rc.rc_refcount - refchk->nr refcount)
> -	 * references we haven't found yet.  Pull that many off the
> -	 * fragment list and figure out where the smallest rmap ends
> -	 * (and therefore the next rmap should start).  All the rmaps
> -	 * we pull off should start at or before the beginning of the
> -	 * refcount record's range.
> -	 */
> -	INIT_LIST_HEAD(&worklist);
> -	rbno = NULLRGBLOCK;
> -
> -	/* Make sure the fragments actually /are/ in bno order. */
> -	bno = 0;
> -	list_for_each_entry(frag, &refchk->fragments, list) {
> -		if (frag->rm.rm_startblock < bno)
> -			goto done;
> -		bno = frag->rm.rm_startblock;
> -	}
> -
> -	/*
> -	 * Find all the rmaps that start at or before the refc extent,
> -	 * and put them on the worklist.
> -	 */
> -	nr = 0;
> -	list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
> -		if (frag->rm.rm_startblock > refchk->bno || nr > target_nr)
> -			break;
> -		bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
> -		if (bno < rbno)
> -			rbno = bno;
> -		list_move_tail(&frag->list, &worklist);
> -		nr++;
> -	}
> -
> -	/*
> -	 * We should have found exactly $target_nr rmap fragments starting
> -	 * at or before the refcount extent.
> -	 */
> -	if (nr != target_nr)
> -		goto done;
> -
> -	while (!list_empty(&refchk->fragments)) {
> -		/* Discard any fragments ending at rbno from the worklist. */
> -		nr = 0;
> -		next_rbno = NULLRGBLOCK;
> -		list_for_each_entry_safe(frag, n, &worklist, list) {
> -			bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
> -			if (bno != rbno) {
> -				if (bno < next_rbno)
> -					next_rbno = bno;
> -				continue;
> -			}
> -			list_del(&frag->list);
> -			kfree(frag);
> -			nr++;
> -		}
> -
> -		/* Try to add nr rmaps starting at rbno to the worklist. */
> -		list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
> -			bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
> -			if (frag->rm.rm_startblock != rbno)
> -				goto done;
> -			list_move_tail(&frag->list, &worklist);
> -			if (next_rbno > bno)
> -				next_rbno = bno;
> -			nr--;
> -			if (nr == 0)
> -				break;
> -		}
> -
> -		/*
> -		 * If we get here and nr > 0, this means that we added fewer
> -		 * items to the worklist than we discarded because the fragment
> -		 * list ran out of items.  Therefore, we cannot maintain the
> -		 * required refcount.  Something is wrong, so we're done.
> -		 */
> -		if (nr)
> -			goto done;
> -
> -		rbno = next_rbno;
> -	}
> -
> -	/*
> -	 * Make sure the last extent we processed ends at or beyond
> -	 * the end of the refcount extent.
> -	 */
> -	if (rbno < refchk->bno + refchk->len)
> -		goto done;
> -
> -	/* Actually record us having seen the remaining refcount. */
> -	refchk->seen = refchk->refcount;
> -done:
> -	/* Delete fragments and work list. */
> -	list_for_each_entry_safe(frag, n, &worklist, list) {
> -		list_del(&frag->list);
> -		kfree(frag);
> -	}
> -	list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
> -		list_del(&frag->list);
> -		kfree(frag);
> -	}
> -}
> -
>  /* Use the rmap entries covering this extent to verify the refcount. */
>  STATIC void
>  xchk_rtrefcountbt_xref_rmap(
>  	struct xfs_scrub		*sc,
>  	const struct xfs_refcount_irec	*irec)
>  {
> -	struct xchk_rtrefcnt_check	refchk = {
> +	struct xchk_refcnt_check	refchk = {
>  		.sc			= sc,
>  		.bno			= irec->rc_startblock,
>  		.len			= irec->rc_blockcount,
> @@ -309,8 +75,8 @@ xchk_rtrefcountbt_xref_rmap(
>  	};
>  	struct xfs_rmap_irec		low;
>  	struct xfs_rmap_irec		high;
> -	struct xchk_rtrefcnt_frag	*frag;
> -	struct xchk_rtrefcnt_frag	*n;
> +	struct xchk_refcnt_frag		*frag;
> +	struct xchk_refcnt_frag		*n;
>  	int				error;
>  
>  	if (!sc->sr.rmap_cur || xchk_skip_xref(sc->sm))
> @@ -324,11 +90,11 @@ xchk_rtrefcountbt_xref_rmap(
>  
>  	INIT_LIST_HEAD(&refchk.fragments);
>  	error = xfs_rmap_query_range(sc->sr.rmap_cur, &low, &high,
> -			xchk_rtrefcountbt_rmap_check, &refchk);
> +			xchk_refcountbt_rmap_check, &refchk);
>  	if (!xchk_should_check_xref(sc, &error, &sc->sr.rmap_cur))
>  		goto out_free;
>  
> -	xchk_rtrefcountbt_process_rmap_fragments(&refchk);
> +	xchk_refcountbt_process_rmap_fragments(&refchk);
>  	if (irec->rc_refcount != refchk.seen)
>  		xchk_btree_xref_set_corrupt(sc, sc->sr.rmap_cur, 0);
>  
> @@ -408,21 +174,6 @@ xchk_rtrefcountbt_check_mergeable(
>  	memcpy(&rrc->prev_rec, irec, sizeof(struct xfs_refcount_irec));
>  }
>  
> -STATIC int
> -xchk_rtrefcountbt_rmap_check_gap(
> -	struct xfs_btree_cur		*cur,
> -	const struct xfs_rmap_irec	*rec,
> -	void				*priv)
> -{
> -	xfs_rgblock_t			*next_bno = priv;
> -
> -	if (*next_bno != NULLRGBLOCK && rec->rm_startblock < *next_bno)
> -		return -ECANCELED;
> -
> -	*next_bno = rec->rm_startblock + rec->rm_blockcount;
> -	return 0;
> -}
> -
>  /*
>   * Make sure that a gap in the reference count records does not correspond to
>   * overlapping records (i.e. shared extents) in the reverse mappings.
> @@ -448,7 +199,7 @@ xchk_rtrefcountbt_xref_gaps(
>  	high.rm_startblock = bno - 1;
>  
>  	error = xfs_rmap_query_range(sc->sr.rmap_cur, &low, &high,
> -			xchk_rtrefcountbt_rmap_check_gap, &next_bno);
> +			xchk_refcountbt_rmap_check_gap, &next_bno);
>  	if (error == -ECANCELED)
>  		xchk_btree_xref_set_corrupt(sc, sc->sr.rmap_cur, 0);
>  	else
> diff --git a/fs/xfs/scrub/rtrefcount_repair.c b/fs/xfs/scrub/rtrefcount_repair.c
> index c78a6d2990c5..5ca9959dfacc 100644
> --- a/fs/xfs/scrub/rtrefcount_repair.c
> +++ b/fs/xfs/scrub/rtrefcount_repair.c
> @@ -44,6 +44,7 @@
>  #include "scrub/newbt.h"
>  #include "scrub/reap.h"
>  #include "scrub/rcbag.h"
> +#include "scrub/refcount.h"
>  
>  /*
>   * Rebuilding the Reference Count Btree
> @@ -266,42 +267,6 @@ xrep_rtrefc_walk_rmaps(
>  	return 0;
>  }
>  
> -static inline uint32_t
> -xrep_rtrefc_encode_startblock(
> -	const struct xfs_refcount_irec	*irec)
> -{
> -	uint32_t			start;
> -
> -	start = irec->rc_startblock & ~XFS_REFC_COWFLAG;
> -	if (irec->rc_domain == XFS_REFC_DOMAIN_COW)
> -		start |= XFS_REFC_COWFLAG;
> -
> -	return start;
> -}
> -
> -/*
> - * Compare two refcount records.  We want to sort in order of increasing block
> - * number.
> - */
> -static int
> -xrep_rtrefc_extent_cmp(
> -	const void			*a,
> -	const void			*b)
> -{
> -	const struct xfs_refcount_irec	*ap = a;
> -	const struct xfs_refcount_irec	*bp = b;
> -	uint32_t			sa, sb;
> -
> -	sa = xrep_rtrefc_encode_startblock(ap);
> -	sb = xrep_rtrefc_encode_startblock(bp);
> -
> -	if (sa > sb)
> -		return 1;
> -	if (sa < sb)
> -		return -1;
> -	return 0;
> -}
> -
>  /*
>   * Sort the refcount extents by startblock or else the btree records will be in
>   * the wrong order.  Make sure the records do not overlap in physical space.
> @@ -316,7 +281,7 @@ xrep_rtrefc_sort_records(
>  	xfs_rgblock_t			next_rgbno = 0;
>  	int				error;
>  
> -	error = xfarray_sort(rr->refcount_records, xrep_rtrefc_extent_cmp,
> +	error = xfarray_sort(rr->refcount_records, xrep_refc_extent_cmp,
>  			XFARRAY_SORT_KILLABLE);
>  	if (error)
>  		return error;
> -- 
> 2.55.0
> 
> 

Re: [PATCH 3/3] xfs: re-use refcount scrub/repair code for rtrefcount

[Carlos Maiolino]

On Fri, Oct 02, 2026 at 04:08:11PM -0500, Eric Sandeen wrote:
> Some code in scrub/rtrefcount.c is copied from scrub/refcount.c; it
> differs only in function & structure names and a couple types and
> macros, which share the same underlying types and values.
> 
> Similarly for code in fs/xfs/scrub/[rt]refcount_repair.c
> 
> Export the refcount versions, use them for rtrefcount scrubbing,
> eliminate the copies, and add build-time asserts so that the type/value
> matches mentioned above do not drift.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Looks good,
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

>  fs/xfs/scrub/refcount.c          |  27 +---
>  fs/xfs/scrub/refcount.h          |  56 +++++++
>  fs/xfs/scrub/refcount_repair.c   |   3 +-
>  fs/xfs/scrub/rtrefcount.c        | 263 +------------------------------
>  fs/xfs/scrub/rtrefcount_repair.c |  39 +----
>  5 files changed, 72 insertions(+), 316 deletions(-)
>  create mode 100644 fs/xfs/scrub/refcount.h
> 
> diff --git a/fs/xfs/scrub/refcount.c b/fs/xfs/scrub/refcount.c
> index f8c51d8fbb3d..c18bb89325a5 100644
> --- a/fs/xfs/scrub/refcount.c
> +++ b/fs/xfs/scrub/refcount.c
> @@ -20,6 +20,7 @@
>  #include "scrub/btree.h"
>  #include "scrub/trace.h"
>  #include "scrub/repair.h"
> +#include "scrub/refcount.h"
>  
>  /*
>   * Set us up to scrub reference count btrees.
> @@ -80,24 +81,6 @@ xchk_setup_ag_refcountbt(
>   * If the refcount is correct, all the check conditions in the algorithm
>   * should always hold true.  If not, the refcount is incorrect.
>   */
> -struct xchk_refcnt_frag {
> -	struct list_head	list;
> -	struct xfs_rmap_irec	rm;
> -};
> -
> -struct xchk_refcnt_check {
> -	struct xfs_scrub	*sc;
> -	struct list_head	fragments;
> -
> -	/* refcount extent we're examining */
> -	xfs_agblock_t		bno;
> -	xfs_extlen_t		len;
> -	xfs_nlink_t		refcount;
> -
> -	/* number of owners seen */
> -	xfs_nlink_t		seen;
> -};
> -
>  /*
>   * Decide if the given rmap is large enough that we can redeem it
>   * towards refcount verification now, or if it's a fragment, in
> @@ -105,7 +88,7 @@ struct xchk_refcnt_check {
>   * discover that we've collected exactly the correct number of
>   * fragments as the refcountbt says we should have.
>   */
> -STATIC int
> +int
>  xchk_refcountbt_rmap_check(
>  	struct xfs_btree_cur		*cur,
>  	const struct xfs_rmap_irec	*rec,
> @@ -159,7 +142,7 @@ xchk_refcountbt_rmap_check(
>   * number of extents that totally covered the refcountbt extent),
>   * we have a refcountbt error.
>   */
> -STATIC void
> +void
>  xchk_refcountbt_process_rmap_fragments(
>  	struct xchk_refcnt_check	*refchk)
>  {
> @@ -290,7 +273,7 @@ xchk_refcountbt_xref_rmap(
>  		.bno			= irec->rc_startblock,
>  		.len			= irec->rc_blockcount,
>  		.refcount		= irec->rc_refcount,
> -		.seen = 0,
> +		.seen			= 0,
>  	};
>  	struct xfs_rmap_irec		low;
>  	struct xfs_rmap_irec		high;
> @@ -355,7 +338,7 @@ struct xchk_refcbt_records {
>  	enum xfs_refc_domain	prev_domain;
>  };
>  
> -STATIC int
> +int
>  xchk_refcountbt_rmap_check_gap(
>  	struct xfs_btree_cur		*cur,
>  	const struct xfs_rmap_irec	*rec,
> diff --git a/fs/xfs/scrub/refcount.h b/fs/xfs/scrub/refcount.h
> new file mode 100644
> index 000000000000..0706afd9f80b
> --- /dev/null
> +++ b/fs/xfs/scrub/refcount.h
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2017-2023 Oracle.  All Rights Reserved.
> + * Author: Darrick J. Wong <djwong@kernel.org>
> + */
> +#ifndef __XFS_SCRUB_REFCOUNT_H__
> +#define __XFS_SCRUB_REFCOUNT_H__
> +
> +/*
> + * Ensure that types and macros match across rt and non-rt variants,
> + * as this code is shared for both.
> + */
> +static_assert(__same_type(xfs_agblock_t, xfs_rgblock_t));
> +static_assert(NULLAGBLOCK == NULLRGBLOCK);
> +
> +/*
> + * Confirming reference counts via reverse mappings.
> + *
> + * These helpers are shared by the AG (refcount.c) and realtime
> + * (rtrefcount.c) refcount scrubbers.  Both the rmap and refcount incore
> + * records use xfs_agblock_t for their start block regardless of whether the
> + * data lives in an allocation group or a realtime group, so the fragment
> + * bookkeeping here uses xfs_agblock_t for group-relative block numbers.
> + */
> +struct xchk_refcnt_frag {
> +	struct list_head	list;
> +	struct xfs_rmap_irec	rm;
> +};
> +
> +struct xchk_refcnt_check {
> +	struct xfs_scrub	*sc;
> +	struct list_head	fragments;
> +
> +	/* refcount extent we're examining */
> +	xfs_agblock_t		bno;
> +	xfs_extlen_t		len;
> +	xfs_nlink_t		refcount;
> +
> +	/* number of owners seen */
> +	xfs_nlink_t		seen;
> +};
> +
> +int xchk_refcountbt_rmap_check(struct xfs_btree_cur *cur,
> +		const struct xfs_rmap_irec *rec, void *priv);
> +void xchk_refcountbt_process_rmap_fragments(struct xchk_refcnt_check *refchk);
> +int xchk_refcountbt_rmap_check_gap(struct xfs_btree_cur *cur,
> +		const struct xfs_rmap_irec *rec, void *priv);
> +
> +/*
> + * Compare two refcount records so that both the AG (refcount_repair.c) and
> + * realtime (rtrefcount_repair.c) rebuilders sort staged records into the same
> + * order as the ondisk records.
> + */
> +int xrep_refc_extent_cmp(const void *a, const void *b);
> +
> +#endif /* __XFS_SCRUB_REFCOUNT_H__ */
> diff --git a/fs/xfs/scrub/refcount_repair.c b/fs/xfs/scrub/refcount_repair.c
> index ca9c382005ff..72de263117a7 100644
> --- a/fs/xfs/scrub/refcount_repair.c
> +++ b/fs/xfs/scrub/refcount_repair.c
> @@ -39,6 +39,7 @@
>  #include "scrub/newbt.h"
>  #include "scrub/reap.h"
>  #include "scrub/rcbag.h"
> +#include "scrub/refcount.h"
>  
>  /*
>   * Rebuilding the Reference Count Btree
> @@ -295,7 +296,7 @@ xrep_refc_encode_startblock(
>  }
>  
>  /* Sort in the same order as the ondisk records. */
> -static int
> +int
>  xrep_refc_extent_cmp(
>  	const void			*a,
>  	const void			*b)
> diff --git a/fs/xfs/scrub/rtrefcount.c b/fs/xfs/scrub/rtrefcount.c
> index 460e3f094adf..b78784051308 100644
> --- a/fs/xfs/scrub/rtrefcount.c
> +++ b/fs/xfs/scrub/rtrefcount.c
> @@ -25,6 +25,7 @@
>  #include "scrub/common.h"
>  #include "scrub/btree.h"
>  #include "scrub/repair.h"
> +#include "scrub/refcount.h"
>  
>  /* Set us up with the realtime refcount metadata locked. */
>  int
> @@ -59,248 +60,13 @@ xchk_setup_rtrefcountbt(
>  
>  /* Realtime Reference count btree scrubber. */
>  
> -/*
> - * Confirming Reference Counts via Reverse Mappings
> - *
> - * We want to count the reverse mappings overlapping a refcount record
> - * (bno, len, refcount), allowing for the possibility that some of the
> - * overlap may come from smaller adjoining reverse mappings, while some
> - * comes from single extents which overlap the range entirely.  The
> - * outer loop is as follows:
> - *
> - * 1. For all reverse mappings overlapping the refcount extent,
> - *    a. If a given rmap completely overlaps, mark it as seen.
> - *    b. Otherwise, record the fragment (in agbno order) for later
> - *       processing.
> - *
> - * Once we've seen all the rmaps, we know that for all blocks in the
> - * refcount record we want to find $refcount owners and we've already
> - * visited $seen extents that overlap all the blocks.  Therefore, we
> - * need to find ($refcount - $seen) owners for every block in the
> - * extent; call that quantity $target_nr.  Proceed as follows:
> - *
> - * 2. Pull the first $target_nr fragments from the list; all of them
> - *    should start at or before the start of the extent.
> - *    Call this subset of fragments the working set.
> - * 3. Until there are no more unprocessed fragments,
> - *    a. Find the shortest fragments in the set and remove them.
> - *    b. Note the block number of the end of these fragments.
> - *    c. Pull the same number of fragments from the list.  All of these
> - *       fragments should start at the block number recorded in the
> - *       previous step.
> - *    d. Put those fragments in the set.
> - * 4. Check that there are $target_nr fragments remaining in the list,
> - *    and that they all end at or beyond the end of the refcount extent.
> - *
> - * If the refcount is correct, all the check conditions in the algorithm
> - * should always hold true.  If not, the refcount is incorrect.
> - */
> -struct xchk_rtrefcnt_frag {
> -	struct list_head	list;
> -	struct xfs_rmap_irec	rm;
> -};
> -
> -struct xchk_rtrefcnt_check {
> -	struct xfs_scrub	*sc;
> -	struct list_head	fragments;
> -
> -	/* refcount extent we're examining */
> -	xfs_rgblock_t		bno;
> -	xfs_extlen_t		len;
> -	xfs_nlink_t		refcount;
> -
> -	/* number of owners seen */
> -	xfs_nlink_t		seen;
> -};
> -
> -/*
> - * Decide if the given rmap is large enough that we can redeem it
> - * towards refcount verification now, or if it's a fragment, in
> - * which case we'll hang onto it in the hopes that we'll later
> - * discover that we've collected exactly the correct number of
> - * fragments as the rtrefcountbt says we should have.
> - */
> -STATIC int
> -xchk_rtrefcountbt_rmap_check(
> -	struct xfs_btree_cur		*cur,
> -	const struct xfs_rmap_irec	*rec,
> -	void				*priv)
> -{
> -	struct xchk_rtrefcnt_check	*refchk = priv;
> -	struct xchk_rtrefcnt_frag	*frag;
> -	xfs_rgblock_t			rm_last;
> -	xfs_rgblock_t			rc_last;
> -	int				error = 0;
> -
> -	if (xchk_should_terminate(refchk->sc, &error))
> -		return error;
> -
> -	rm_last = rec->rm_startblock + rec->rm_blockcount - 1;
> -	rc_last = refchk->bno + refchk->len - 1;
> -
> -	/* Confirm that a single-owner refc extent is a CoW stage. */
> -	if (refchk->refcount == 1 && rec->rm_owner != XFS_RMAP_OWN_COW) {
> -		xchk_btree_xref_set_corrupt(refchk->sc, cur, 0);
> -		return 0;
> -	}
> -
> -	if (rec->rm_startblock <= refchk->bno && rm_last >= rc_last) {
> -		/*
> -		 * The rmap overlaps the refcount record, so we can confirm
> -		 * one refcount owner seen.
> -		 */
> -		refchk->seen++;
> -	} else {
> -		/*
> -		 * This rmap covers only part of the refcount record, so
> -		 * save the fragment for later processing.  If the rmapbt
> -		 * is healthy each rmap_irec we see will be in agbno order
> -		 * so we don't need insertion sort here.
> -		 */
> -		frag = kmalloc_obj(struct xchk_rtrefcnt_frag, XCHK_GFP_FLAGS);
> -		if (!frag)
> -			return -ENOMEM;
> -		memcpy(&frag->rm, rec, sizeof(frag->rm));
> -		list_add_tail(&frag->list, &refchk->fragments);
> -	}
> -
> -	return 0;
> -}
> -
> -/*
> - * Given a bunch of rmap fragments, iterate through them, keeping
> - * a running tally of the refcount.  If this ever deviates from
> - * what we expect (which is the rtrefcountbt's refcount minus the
> - * number of extents that totally covered the rtrefcountbt extent),
> - * we have a rtrefcountbt error.
> - */
> -STATIC void
> -xchk_rtrefcountbt_process_rmap_fragments(
> -	struct xchk_rtrefcnt_check	*refchk)
> -{
> -	struct list_head		worklist;
> -	struct xchk_rtrefcnt_frag	*frag;
> -	struct xchk_rtrefcnt_frag	*n;
> -	xfs_rgblock_t			bno;
> -	xfs_rgblock_t			rbno;
> -	xfs_rgblock_t			next_rbno;
> -	xfs_nlink_t			nr;
> -	xfs_nlink_t			target_nr;
> -
> -	target_nr = refchk->refcount - refchk->seen;
> -	if (target_nr == 0)
> -		return;
> -
> -	/*
> -	 * There are (refchk->rc.rc_refcount - refchk->nr refcount)
> -	 * references we haven't found yet.  Pull that many off the
> -	 * fragment list and figure out where the smallest rmap ends
> -	 * (and therefore the next rmap should start).  All the rmaps
> -	 * we pull off should start at or before the beginning of the
> -	 * refcount record's range.
> -	 */
> -	INIT_LIST_HEAD(&worklist);
> -	rbno = NULLRGBLOCK;
> -
> -	/* Make sure the fragments actually /are/ in bno order. */
> -	bno = 0;
> -	list_for_each_entry(frag, &refchk->fragments, list) {
> -		if (frag->rm.rm_startblock < bno)
> -			goto done;
> -		bno = frag->rm.rm_startblock;
> -	}
> -
> -	/*
> -	 * Find all the rmaps that start at or before the refc extent,
> -	 * and put them on the worklist.
> -	 */
> -	nr = 0;
> -	list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
> -		if (frag->rm.rm_startblock > refchk->bno || nr > target_nr)
> -			break;
> -		bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
> -		if (bno < rbno)
> -			rbno = bno;
> -		list_move_tail(&frag->list, &worklist);
> -		nr++;
> -	}
> -
> -	/*
> -	 * We should have found exactly $target_nr rmap fragments starting
> -	 * at or before the refcount extent.
> -	 */
> -	if (nr != target_nr)
> -		goto done;
> -
> -	while (!list_empty(&refchk->fragments)) {
> -		/* Discard any fragments ending at rbno from the worklist. */
> -		nr = 0;
> -		next_rbno = NULLRGBLOCK;
> -		list_for_each_entry_safe(frag, n, &worklist, list) {
> -			bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
> -			if (bno != rbno) {
> -				if (bno < next_rbno)
> -					next_rbno = bno;
> -				continue;
> -			}
> -			list_del(&frag->list);
> -			kfree(frag);
> -			nr++;
> -		}
> -
> -		/* Try to add nr rmaps starting at rbno to the worklist. */
> -		list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
> -			bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
> -			if (frag->rm.rm_startblock != rbno)
> -				goto done;
> -			list_move_tail(&frag->list, &worklist);
> -			if (next_rbno > bno)
> -				next_rbno = bno;
> -			nr--;
> -			if (nr == 0)
> -				break;
> -		}
> -
> -		/*
> -		 * If we get here and nr > 0, this means that we added fewer
> -		 * items to the worklist than we discarded because the fragment
> -		 * list ran out of items.  Therefore, we cannot maintain the
> -		 * required refcount.  Something is wrong, so we're done.
> -		 */
> -		if (nr)
> -			goto done;
> -
> -		rbno = next_rbno;
> -	}
> -
> -	/*
> -	 * Make sure the last extent we processed ends at or beyond
> -	 * the end of the refcount extent.
> -	 */
> -	if (rbno < refchk->bno + refchk->len)
> -		goto done;
> -
> -	/* Actually record us having seen the remaining refcount. */
> -	refchk->seen = refchk->refcount;
> -done:
> -	/* Delete fragments and work list. */
> -	list_for_each_entry_safe(frag, n, &worklist, list) {
> -		list_del(&frag->list);
> -		kfree(frag);
> -	}
> -	list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
> -		list_del(&frag->list);
> -		kfree(frag);
> -	}
> -}
> -
>  /* Use the rmap entries covering this extent to verify the refcount. */
>  STATIC void
>  xchk_rtrefcountbt_xref_rmap(
>  	struct xfs_scrub		*sc,
>  	const struct xfs_refcount_irec	*irec)
>  {
> -	struct xchk_rtrefcnt_check	refchk = {
> +	struct xchk_refcnt_check	refchk = {
>  		.sc			= sc,
>  		.bno			= irec->rc_startblock,
>  		.len			= irec->rc_blockcount,
> @@ -309,8 +75,8 @@ xchk_rtrefcountbt_xref_rmap(
>  	};
>  	struct xfs_rmap_irec		low;
>  	struct xfs_rmap_irec		high;
> -	struct xchk_rtrefcnt_frag	*frag;
> -	struct xchk_rtrefcnt_frag	*n;
> +	struct xchk_refcnt_frag		*frag;
> +	struct xchk_refcnt_frag		*n;
>  	int				error;
>  
>  	if (!sc->sr.rmap_cur || xchk_skip_xref(sc->sm))
> @@ -324,11 +90,11 @@ xchk_rtrefcountbt_xref_rmap(
>  
>  	INIT_LIST_HEAD(&refchk.fragments);
>  	error = xfs_rmap_query_range(sc->sr.rmap_cur, &low, &high,
> -			xchk_rtrefcountbt_rmap_check, &refchk);
> +			xchk_refcountbt_rmap_check, &refchk);
>  	if (!xchk_should_check_xref(sc, &error, &sc->sr.rmap_cur))
>  		goto out_free;
>  
> -	xchk_rtrefcountbt_process_rmap_fragments(&refchk);
> +	xchk_refcountbt_process_rmap_fragments(&refchk);
>  	if (irec->rc_refcount != refchk.seen)
>  		xchk_btree_xref_set_corrupt(sc, sc->sr.rmap_cur, 0);
>  
> @@ -408,21 +174,6 @@ xchk_rtrefcountbt_check_mergeable(
>  	memcpy(&rrc->prev_rec, irec, sizeof(struct xfs_refcount_irec));
>  }
>  
> -STATIC int
> -xchk_rtrefcountbt_rmap_check_gap(
> -	struct xfs_btree_cur		*cur,
> -	const struct xfs_rmap_irec	*rec,
> -	void				*priv)
> -{
> -	xfs_rgblock_t			*next_bno = priv;
> -
> -	if (*next_bno != NULLRGBLOCK && rec->rm_startblock < *next_bno)
> -		return -ECANCELED;
> -
> -	*next_bno = rec->rm_startblock + rec->rm_blockcount;
> -	return 0;
> -}
> -
>  /*
>   * Make sure that a gap in the reference count records does not correspond to
>   * overlapping records (i.e. shared extents) in the reverse mappings.
> @@ -448,7 +199,7 @@ xchk_rtrefcountbt_xref_gaps(
>  	high.rm_startblock = bno - 1;
>  
>  	error = xfs_rmap_query_range(sc->sr.rmap_cur, &low, &high,
> -			xchk_rtrefcountbt_rmap_check_gap, &next_bno);
> +			xchk_refcountbt_rmap_check_gap, &next_bno);
>  	if (error == -ECANCELED)
>  		xchk_btree_xref_set_corrupt(sc, sc->sr.rmap_cur, 0);
>  	else
> diff --git a/fs/xfs/scrub/rtrefcount_repair.c b/fs/xfs/scrub/rtrefcount_repair.c
> index c78a6d2990c5..5ca9959dfacc 100644
> --- a/fs/xfs/scrub/rtrefcount_repair.c
> +++ b/fs/xfs/scrub/rtrefcount_repair.c
> @@ -44,6 +44,7 @@
>  #include "scrub/newbt.h"
>  #include "scrub/reap.h"
>  #include "scrub/rcbag.h"
> +#include "scrub/refcount.h"
>  
>  /*
>   * Rebuilding the Reference Count Btree
> @@ -266,42 +267,6 @@ xrep_rtrefc_walk_rmaps(
>  	return 0;
>  }
>  
> -static inline uint32_t
> -xrep_rtrefc_encode_startblock(
> -	const struct xfs_refcount_irec	*irec)
> -{
> -	uint32_t			start;
> -
> -	start = irec->rc_startblock & ~XFS_REFC_COWFLAG;
> -	if (irec->rc_domain == XFS_REFC_DOMAIN_COW)
> -		start |= XFS_REFC_COWFLAG;
> -
> -	return start;
> -}
> -
> -/*
> - * Compare two refcount records.  We want to sort in order of increasing block
> - * number.
> - */
> -static int
> -xrep_rtrefc_extent_cmp(
> -	const void			*a,
> -	const void			*b)
> -{
> -	const struct xfs_refcount_irec	*ap = a;
> -	const struct xfs_refcount_irec	*bp = b;
> -	uint32_t			sa, sb;
> -
> -	sa = xrep_rtrefc_encode_startblock(ap);
> -	sb = xrep_rtrefc_encode_startblock(bp);
> -
> -	if (sa > sb)
> -		return 1;
> -	if (sa < sb)
> -		return -1;
> -	return 0;
> -}
> -
>  /*
>   * Sort the refcount extents by startblock or else the btree records will be in
>   * the wrong order.  Make sure the records do not overlap in physical space.
> @@ -316,7 +281,7 @@ xrep_rtrefc_sort_records(
>  	xfs_rgblock_t			next_rgbno = 0;
>  	int				error;
>  
> -	error = xfarray_sort(rr->refcount_records, xrep_rtrefc_extent_cmp,
> +	error = xfarray_sort(rr->refcount_records, xrep_refc_extent_cmp,
>  			XFARRAY_SORT_KILLABLE);
>  	if (error)
>  		return error;
> -- 
> 2.55.0
> 
> 

Re: [PATCH 3/3] xfs: re-use refcount scrub/repair code for rtrefcount

[Christoph Hellwig]

On Fri, Oct 02, 2026 at 04:08:11PM -0500, Eric Sandeen wrote:
> Some code in scrub/rtrefcount.c is copied from scrub/refcount.c; it
> differs only in function & structure names and a couple types and
> macros, which share the same underlying types and values.
> 
> Similarly for code in fs/xfs/scrub/[rt]refcount_repair.c
> 
> Export the refcount versions, use them for rtrefcount scrubbing,

Not really an export fortunately :)

> eliminate the copies, and add build-time asserts so that the type/value
> matches mentioned above do not drift.

>  5 files changed, 72 insertions(+), 316 deletions(-)

Nice code savings!

Looks good modulo the commit log nitpick:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/3] xfs: re-use refcount scrub/repair code for rtrefcount

[Eric Sandeen]

On 10/7/26 8:51 AM, Christoph Hellwig wrote:
> On Fri, Oct 02, 2026 at 04:08:11PM -0500, Eric Sandeen wrote:
>> Some code in scrub/rtrefcount.c is copied from scrub/refcount.c; it
>> differs only in function & structure names and a couple types and
>> macros, which share the same underlying types and values.
>>
>> Similarly for code in fs/xfs/scrub/[rt]refcount_repair.c
>>
>> Export the refcount versions, use them for rtrefcount scrubbing,
> 
> Not really an export fortunately :)

Oh, uh, yeah. I didn't mean like EXPORT_SYMBOL just un-STATIC. :( 

Maybe Carlos fix it to:

Make the refcount versions non-static and use them for rtrefcount scrubbing.

on commit, or I can send another version if needed.

> 
>> eliminate the copies, and add build-time asserts so that the type/value
>> matches mentioned above do not drift.
> 
>>  5 files changed, 72 insertions(+), 316 deletions(-)
> 
> Nice code savings!
> 
> Looks good modulo the commit log nitpick:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

Thanks,
-Eric

Re: [PATCH 3/3] xfs: re-use refcount scrub/repair code for rtrefcount

[Darrick J. Wong]

On Wed, Oct 07, 2026 at 10:32:20AM -0500, Eric Sandeen wrote:
> On 10/7/26 8:51 AM, Christoph Hellwig wrote:
> > On Fri, Oct 02, 2026 at 04:08:11PM -0500, Eric Sandeen wrote:
> >> Some code in scrub/rtrefcount.c is copied from scrub/refcount.c; it
> >> differs only in function & structure names and a couple types and
> >> macros, which share the same underlying types and values.
> >>
> >> Similarly for code in fs/xfs/scrub/[rt]refcount_repair.c
> >>
> >> Export the refcount versions, use them for rtrefcount scrubbing,
> > 
> > Not really an export fortunately :)
> 
> Oh, uh, yeah. I didn't mean like EXPORT_SYMBOL just un-STATIC. :( 
> 
> Maybe Carlos fix it to:
> 
> Make the refcount versions non-static and use them for rtrefcount scrubbing.
> 
> on commit, or I can send another version if needed.

s/Export/Share/ ?

--D

> > 
> >> eliminate the copies, and add build-time asserts so that the type/value
> >> matches mentioned above do not drift.
> > 
> >>  5 files changed, 72 insertions(+), 316 deletions(-)
> > 
> > Nice code savings!
> > 
> > Looks good modulo the commit log nitpick:
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > 
> 
> Thanks,
> -Eric
> 

Re: [PATCH 3/3] xfs: re-use refcount scrub/repair code for rtrefcount

[Carlos Maiolino]

On Wed, Oct 07, 2026 at 08:37:57AM -0700, Darrick J. Wong wrote:
> On Wed, Oct 07, 2026 at 10:32:20AM -0500, Eric Sandeen wrote:
> > On 10/7/26 8:51 AM, Christoph Hellwig wrote:
> > > On Fri, Oct 02, 2026 at 04:08:11PM -0500, Eric Sandeen wrote:
> > >> Some code in scrub/rtrefcount.c is copied from scrub/refcount.c; it
> > >> differs only in function & structure names and a couple types and
> > >> macros, which share the same underlying types and values.
> > >>
> > >> Similarly for code in fs/xfs/scrub/[rt]refcount_repair.c
> > >>
> > >> Export the refcount versions, use them for rtrefcount scrubbing,
> > > 
> > > Not really an export fortunately :)
> > 
> > Oh, uh, yeah. I didn't mean like EXPORT_SYMBOL just un-STATIC. :( 
> > 
> > Maybe Carlos fix it to:
> > 
> > Make the refcount versions non-static and use them for rtrefcount scrubbing.

yup, no big deal.

> > 
> > on commit, or I can send another version if needed.
> 
> s/Export/Share/ ?

I went Eric's way, at least looked better for my taste. If you have
any objections, let me know.

> 
> --D
> 
> > > 
> > >> eliminate the copies, and add build-time asserts so that the type/value
> > >> matches mentioned above do not drift.
> > > 
> > >>  5 files changed, 72 insertions(+), 316 deletions(-)
> > > 
> > > Nice code savings!
> > > 
> > > Looks good modulo the commit log nitpick:
> > > 
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > 
> > 
> > Thanks,
> > -Eric
> > 
> 

Re: [PATCH 3/3] xfs: re-use refcount scrub/repair code for rtrefcount

[Carlos Maiolino]

On Fri, Oct 02, 2026 at 04:08:11PM -0500, Eric Sandeen wrote:
> Some code in scrub/rtrefcount.c is copied from scrub/refcount.c; it
> differs only in function & structure names and a couple types and
> macros, which share the same underlying types and values.
> 
> Similarly for code in fs/xfs/scrub/[rt]refcount_repair.c
> 
> Export the refcount versions, use them for rtrefcount scrubbing,
> eliminate the copies, and add build-time asserts so that the type/value
> matches mentioned above do not drift.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

With the description nitpick fixed:
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> ---
>  fs/xfs/scrub/refcount.c          |  27 +---
>  fs/xfs/scrub/refcount.h          |  56 +++++++
>  fs/xfs/scrub/refcount_repair.c   |   3 +-
>  fs/xfs/scrub/rtrefcount.c        | 263 +------------------------------
>  fs/xfs/scrub/rtrefcount_repair.c |  39 +----
>  5 files changed, 72 insertions(+), 316 deletions(-)
>  create mode 100644 fs/xfs/scrub/refcount.h
> 
> diff --git a/fs/xfs/scrub/refcount.c b/fs/xfs/scrub/refcount.c
> index f8c51d8fbb3d..c18bb89325a5 100644
> --- a/fs/xfs/scrub/refcount.c
> +++ b/fs/xfs/scrub/refcount.c
> @@ -20,6 +20,7 @@
>  #include "scrub/btree.h"
>  #include "scrub/trace.h"
>  #include "scrub/repair.h"
> +#include "scrub/refcount.h"
>  
>  /*
>   * Set us up to scrub reference count btrees.
> @@ -80,24 +81,6 @@ xchk_setup_ag_refcountbt(
>   * If the refcount is correct, all the check conditions in the algorithm
>   * should always hold true.  If not, the refcount is incorrect.
>   */
> -struct xchk_refcnt_frag {
> -	struct list_head	list;
> -	struct xfs_rmap_irec	rm;
> -};
> -
> -struct xchk_refcnt_check {
> -	struct xfs_scrub	*sc;
> -	struct list_head	fragments;
> -
> -	/* refcount extent we're examining */
> -	xfs_agblock_t		bno;
> -	xfs_extlen_t		len;
> -	xfs_nlink_t		refcount;
> -
> -	/* number of owners seen */
> -	xfs_nlink_t		seen;
> -};
> -
>  /*
>   * Decide if the given rmap is large enough that we can redeem it
>   * towards refcount verification now, or if it's a fragment, in
> @@ -105,7 +88,7 @@ struct xchk_refcnt_check {
>   * discover that we've collected exactly the correct number of
>   * fragments as the refcountbt says we should have.
>   */
> -STATIC int
> +int
>  xchk_refcountbt_rmap_check(
>  	struct xfs_btree_cur		*cur,
>  	const struct xfs_rmap_irec	*rec,
> @@ -159,7 +142,7 @@ xchk_refcountbt_rmap_check(
>   * number of extents that totally covered the refcountbt extent),
>   * we have a refcountbt error.
>   */
> -STATIC void
> +void
>  xchk_refcountbt_process_rmap_fragments(
>  	struct xchk_refcnt_check	*refchk)
>  {
> @@ -290,7 +273,7 @@ xchk_refcountbt_xref_rmap(
>  		.bno			= irec->rc_startblock,
>  		.len			= irec->rc_blockcount,
>  		.refcount		= irec->rc_refcount,
> -		.seen = 0,
> +		.seen			= 0,
>  	};
>  	struct xfs_rmap_irec		low;
>  	struct xfs_rmap_irec		high;
> @@ -355,7 +338,7 @@ struct xchk_refcbt_records {
>  	enum xfs_refc_domain	prev_domain;
>  };
>  
> -STATIC int
> +int
>  xchk_refcountbt_rmap_check_gap(
>  	struct xfs_btree_cur		*cur,
>  	const struct xfs_rmap_irec	*rec,
> diff --git a/fs/xfs/scrub/refcount.h b/fs/xfs/scrub/refcount.h
> new file mode 100644
> index 000000000000..0706afd9f80b
> --- /dev/null
> +++ b/fs/xfs/scrub/refcount.h
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2017-2023 Oracle.  All Rights Reserved.
> + * Author: Darrick J. Wong <djwong@kernel.org>
> + */
> +#ifndef __XFS_SCRUB_REFCOUNT_H__
> +#define __XFS_SCRUB_REFCOUNT_H__
> +
> +/*
> + * Ensure that types and macros match across rt and non-rt variants,
> + * as this code is shared for both.
> + */
> +static_assert(__same_type(xfs_agblock_t, xfs_rgblock_t));
> +static_assert(NULLAGBLOCK == NULLRGBLOCK);
> +
> +/*
> + * Confirming reference counts via reverse mappings.
> + *
> + * These helpers are shared by the AG (refcount.c) and realtime
> + * (rtrefcount.c) refcount scrubbers.  Both the rmap and refcount incore
> + * records use xfs_agblock_t for their start block regardless of whether the
> + * data lives in an allocation group or a realtime group, so the fragment
> + * bookkeeping here uses xfs_agblock_t for group-relative block numbers.
> + */
> +struct xchk_refcnt_frag {
> +	struct list_head	list;
> +	struct xfs_rmap_irec	rm;
> +};
> +
> +struct xchk_refcnt_check {
> +	struct xfs_scrub	*sc;
> +	struct list_head	fragments;
> +
> +	/* refcount extent we're examining */
> +	xfs_agblock_t		bno;
> +	xfs_extlen_t		len;
> +	xfs_nlink_t		refcount;
> +
> +	/* number of owners seen */
> +	xfs_nlink_t		seen;
> +};
> +
> +int xchk_refcountbt_rmap_check(struct xfs_btree_cur *cur,
> +		const struct xfs_rmap_irec *rec, void *priv);
> +void xchk_refcountbt_process_rmap_fragments(struct xchk_refcnt_check *refchk);
> +int xchk_refcountbt_rmap_check_gap(struct xfs_btree_cur *cur,
> +		const struct xfs_rmap_irec *rec, void *priv);
> +
> +/*
> + * Compare two refcount records so that both the AG (refcount_repair.c) and
> + * realtime (rtrefcount_repair.c) rebuilders sort staged records into the same
> + * order as the ondisk records.
> + */
> +int xrep_refc_extent_cmp(const void *a, const void *b);
> +
> +#endif /* __XFS_SCRUB_REFCOUNT_H__ */
> diff --git a/fs/xfs/scrub/refcount_repair.c b/fs/xfs/scrub/refcount_repair.c
> index ca9c382005ff..72de263117a7 100644
> --- a/fs/xfs/scrub/refcount_repair.c
> +++ b/fs/xfs/scrub/refcount_repair.c
> @@ -39,6 +39,7 @@
>  #include "scrub/newbt.h"
>  #include "scrub/reap.h"
>  #include "scrub/rcbag.h"
> +#include "scrub/refcount.h"
>  
>  /*
>   * Rebuilding the Reference Count Btree
> @@ -295,7 +296,7 @@ xrep_refc_encode_startblock(
>  }
>  
>  /* Sort in the same order as the ondisk records. */
> -static int
> +int
>  xrep_refc_extent_cmp(
>  	const void			*a,
>  	const void			*b)
> diff --git a/fs/xfs/scrub/rtrefcount.c b/fs/xfs/scrub/rtrefcount.c
> index 460e3f094adf..b78784051308 100644
> --- a/fs/xfs/scrub/rtrefcount.c
> +++ b/fs/xfs/scrub/rtrefcount.c
> @@ -25,6 +25,7 @@
>  #include "scrub/common.h"
>  #include "scrub/btree.h"
>  #include "scrub/repair.h"
> +#include "scrub/refcount.h"
>  
>  /* Set us up with the realtime refcount metadata locked. */
>  int
> @@ -59,248 +60,13 @@ xchk_setup_rtrefcountbt(
>  
>  /* Realtime Reference count btree scrubber. */
>  
> -/*
> - * Confirming Reference Counts via Reverse Mappings
> - *
> - * We want to count the reverse mappings overlapping a refcount record
> - * (bno, len, refcount), allowing for the possibility that some of the
> - * overlap may come from smaller adjoining reverse mappings, while some
> - * comes from single extents which overlap the range entirely.  The
> - * outer loop is as follows:
> - *
> - * 1. For all reverse mappings overlapping the refcount extent,
> - *    a. If a given rmap completely overlaps, mark it as seen.
> - *    b. Otherwise, record the fragment (in agbno order) for later
> - *       processing.
> - *
> - * Once we've seen all the rmaps, we know that for all blocks in the
> - * refcount record we want to find $refcount owners and we've already
> - * visited $seen extents that overlap all the blocks.  Therefore, we
> - * need to find ($refcount - $seen) owners for every block in the
> - * extent; call that quantity $target_nr.  Proceed as follows:
> - *
> - * 2. Pull the first $target_nr fragments from the list; all of them
> - *    should start at or before the start of the extent.
> - *    Call this subset of fragments the working set.
> - * 3. Until there are no more unprocessed fragments,
> - *    a. Find the shortest fragments in the set and remove them.
> - *    b. Note the block number of the end of these fragments.
> - *    c. Pull the same number of fragments from the list.  All of these
> - *       fragments should start at the block number recorded in the
> - *       previous step.
> - *    d. Put those fragments in the set.
> - * 4. Check that there are $target_nr fragments remaining in the list,
> - *    and that they all end at or beyond the end of the refcount extent.
> - *
> - * If the refcount is correct, all the check conditions in the algorithm
> - * should always hold true.  If not, the refcount is incorrect.
> - */
> -struct xchk_rtrefcnt_frag {
> -	struct list_head	list;
> -	struct xfs_rmap_irec	rm;
> -};
> -
> -struct xchk_rtrefcnt_check {
> -	struct xfs_scrub	*sc;
> -	struct list_head	fragments;
> -
> -	/* refcount extent we're examining */
> -	xfs_rgblock_t		bno;
> -	xfs_extlen_t		len;
> -	xfs_nlink_t		refcount;
> -
> -	/* number of owners seen */
> -	xfs_nlink_t		seen;
> -};
> -
> -/*
> - * Decide if the given rmap is large enough that we can redeem it
> - * towards refcount verification now, or if it's a fragment, in
> - * which case we'll hang onto it in the hopes that we'll later
> - * discover that we've collected exactly the correct number of
> - * fragments as the rtrefcountbt says we should have.
> - */
> -STATIC int
> -xchk_rtrefcountbt_rmap_check(
> -	struct xfs_btree_cur		*cur,
> -	const struct xfs_rmap_irec	*rec,
> -	void				*priv)
> -{
> -	struct xchk_rtrefcnt_check	*refchk = priv;
> -	struct xchk_rtrefcnt_frag	*frag;
> -	xfs_rgblock_t			rm_last;
> -	xfs_rgblock_t			rc_last;
> -	int				error = 0;
> -
> -	if (xchk_should_terminate(refchk->sc, &error))
> -		return error;
> -
> -	rm_last = rec->rm_startblock + rec->rm_blockcount - 1;
> -	rc_last = refchk->bno + refchk->len - 1;
> -
> -	/* Confirm that a single-owner refc extent is a CoW stage. */
> -	if (refchk->refcount == 1 && rec->rm_owner != XFS_RMAP_OWN_COW) {
> -		xchk_btree_xref_set_corrupt(refchk->sc, cur, 0);
> -		return 0;
> -	}
> -
> -	if (rec->rm_startblock <= refchk->bno && rm_last >= rc_last) {
> -		/*
> -		 * The rmap overlaps the refcount record, so we can confirm
> -		 * one refcount owner seen.
> -		 */
> -		refchk->seen++;
> -	} else {
> -		/*
> -		 * This rmap covers only part of the refcount record, so
> -		 * save the fragment for later processing.  If the rmapbt
> -		 * is healthy each rmap_irec we see will be in agbno order
> -		 * so we don't need insertion sort here.
> -		 */
> -		frag = kmalloc_obj(struct xchk_rtrefcnt_frag, XCHK_GFP_FLAGS);
> -		if (!frag)
> -			return -ENOMEM;
> -		memcpy(&frag->rm, rec, sizeof(frag->rm));
> -		list_add_tail(&frag->list, &refchk->fragments);
> -	}
> -
> -	return 0;
> -}
> -
> -/*
> - * Given a bunch of rmap fragments, iterate through them, keeping
> - * a running tally of the refcount.  If this ever deviates from
> - * what we expect (which is the rtrefcountbt's refcount minus the
> - * number of extents that totally covered the rtrefcountbt extent),
> - * we have a rtrefcountbt error.
> - */
> -STATIC void
> -xchk_rtrefcountbt_process_rmap_fragments(
> -	struct xchk_rtrefcnt_check	*refchk)
> -{
> -	struct list_head		worklist;
> -	struct xchk_rtrefcnt_frag	*frag;
> -	struct xchk_rtrefcnt_frag	*n;
> -	xfs_rgblock_t			bno;
> -	xfs_rgblock_t			rbno;
> -	xfs_rgblock_t			next_rbno;
> -	xfs_nlink_t			nr;
> -	xfs_nlink_t			target_nr;
> -
> -	target_nr = refchk->refcount - refchk->seen;
> -	if (target_nr == 0)
> -		return;
> -
> -	/*
> -	 * There are (refchk->rc.rc_refcount - refchk->nr refcount)
> -	 * references we haven't found yet.  Pull that many off the
> -	 * fragment list and figure out where the smallest rmap ends
> -	 * (and therefore the next rmap should start).  All the rmaps
> -	 * we pull off should start at or before the beginning of the
> -	 * refcount record's range.
> -	 */
> -	INIT_LIST_HEAD(&worklist);
> -	rbno = NULLRGBLOCK;
> -
> -	/* Make sure the fragments actually /are/ in bno order. */
> -	bno = 0;
> -	list_for_each_entry(frag, &refchk->fragments, list) {
> -		if (frag->rm.rm_startblock < bno)
> -			goto done;
> -		bno = frag->rm.rm_startblock;
> -	}
> -
> -	/*
> -	 * Find all the rmaps that start at or before the refc extent,
> -	 * and put them on the worklist.
> -	 */
> -	nr = 0;
> -	list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
> -		if (frag->rm.rm_startblock > refchk->bno || nr > target_nr)
> -			break;
> -		bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
> -		if (bno < rbno)
> -			rbno = bno;
> -		list_move_tail(&frag->list, &worklist);
> -		nr++;
> -	}
> -
> -	/*
> -	 * We should have found exactly $target_nr rmap fragments starting
> -	 * at or before the refcount extent.
> -	 */
> -	if (nr != target_nr)
> -		goto done;
> -
> -	while (!list_empty(&refchk->fragments)) {
> -		/* Discard any fragments ending at rbno from the worklist. */
> -		nr = 0;
> -		next_rbno = NULLRGBLOCK;
> -		list_for_each_entry_safe(frag, n, &worklist, list) {
> -			bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
> -			if (bno != rbno) {
> -				if (bno < next_rbno)
> -					next_rbno = bno;
> -				continue;
> -			}
> -			list_del(&frag->list);
> -			kfree(frag);
> -			nr++;
> -		}
> -
> -		/* Try to add nr rmaps starting at rbno to the worklist. */
> -		list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
> -			bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
> -			if (frag->rm.rm_startblock != rbno)
> -				goto done;
> -			list_move_tail(&frag->list, &worklist);
> -			if (next_rbno > bno)
> -				next_rbno = bno;
> -			nr--;
> -			if (nr == 0)
> -				break;
> -		}
> -
> -		/*
> -		 * If we get here and nr > 0, this means that we added fewer
> -		 * items to the worklist than we discarded because the fragment
> -		 * list ran out of items.  Therefore, we cannot maintain the
> -		 * required refcount.  Something is wrong, so we're done.
> -		 */
> -		if (nr)
> -			goto done;
> -
> -		rbno = next_rbno;
> -	}
> -
> -	/*
> -	 * Make sure the last extent we processed ends at or beyond
> -	 * the end of the refcount extent.
> -	 */
> -	if (rbno < refchk->bno + refchk->len)
> -		goto done;
> -
> -	/* Actually record us having seen the remaining refcount. */
> -	refchk->seen = refchk->refcount;
> -done:
> -	/* Delete fragments and work list. */
> -	list_for_each_entry_safe(frag, n, &worklist, list) {
> -		list_del(&frag->list);
> -		kfree(frag);
> -	}
> -	list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
> -		list_del(&frag->list);
> -		kfree(frag);
> -	}
> -}
> -
>  /* Use the rmap entries covering this extent to verify the refcount. */
>  STATIC void
>  xchk_rtrefcountbt_xref_rmap(
>  	struct xfs_scrub		*sc,
>  	const struct xfs_refcount_irec	*irec)
>  {
> -	struct xchk_rtrefcnt_check	refchk = {
> +	struct xchk_refcnt_check	refchk = {
>  		.sc			= sc,
>  		.bno			= irec->rc_startblock,
>  		.len			= irec->rc_blockcount,
> @@ -309,8 +75,8 @@ xchk_rtrefcountbt_xref_rmap(
>  	};
>  	struct xfs_rmap_irec		low;
>  	struct xfs_rmap_irec		high;
> -	struct xchk_rtrefcnt_frag	*frag;
> -	struct xchk_rtrefcnt_frag	*n;
> +	struct xchk_refcnt_frag		*frag;
> +	struct xchk_refcnt_frag		*n;
>  	int				error;
>  
>  	if (!sc->sr.rmap_cur || xchk_skip_xref(sc->sm))
> @@ -324,11 +90,11 @@ xchk_rtrefcountbt_xref_rmap(
>  
>  	INIT_LIST_HEAD(&refchk.fragments);
>  	error = xfs_rmap_query_range(sc->sr.rmap_cur, &low, &high,
> -			xchk_rtrefcountbt_rmap_check, &refchk);
> +			xchk_refcountbt_rmap_check, &refchk);
>  	if (!xchk_should_check_xref(sc, &error, &sc->sr.rmap_cur))
>  		goto out_free;
>  
> -	xchk_rtrefcountbt_process_rmap_fragments(&refchk);
> +	xchk_refcountbt_process_rmap_fragments(&refchk);
>  	if (irec->rc_refcount != refchk.seen)
>  		xchk_btree_xref_set_corrupt(sc, sc->sr.rmap_cur, 0);
>  
> @@ -408,21 +174,6 @@ xchk_rtrefcountbt_check_mergeable(
>  	memcpy(&rrc->prev_rec, irec, sizeof(struct xfs_refcount_irec));
>  }
>  
> -STATIC int
> -xchk_rtrefcountbt_rmap_check_gap(
> -	struct xfs_btree_cur		*cur,
> -	const struct xfs_rmap_irec	*rec,
> -	void				*priv)
> -{
> -	xfs_rgblock_t			*next_bno = priv;
> -
> -	if (*next_bno != NULLRGBLOCK && rec->rm_startblock < *next_bno)
> -		return -ECANCELED;
> -
> -	*next_bno = rec->rm_startblock + rec->rm_blockcount;
> -	return 0;
> -}
> -
>  /*
>   * Make sure that a gap in the reference count records does not correspond to
>   * overlapping records (i.e. shared extents) in the reverse mappings.
> @@ -448,7 +199,7 @@ xchk_rtrefcountbt_xref_gaps(
>  	high.rm_startblock = bno - 1;
>  
>  	error = xfs_rmap_query_range(sc->sr.rmap_cur, &low, &high,
> -			xchk_rtrefcountbt_rmap_check_gap, &next_bno);
> +			xchk_refcountbt_rmap_check_gap, &next_bno);
>  	if (error == -ECANCELED)
>  		xchk_btree_xref_set_corrupt(sc, sc->sr.rmap_cur, 0);
>  	else
> diff --git a/fs/xfs/scrub/rtrefcount_repair.c b/fs/xfs/scrub/rtrefcount_repair.c
> index c78a6d2990c5..5ca9959dfacc 100644
> --- a/fs/xfs/scrub/rtrefcount_repair.c
> +++ b/fs/xfs/scrub/rtrefcount_repair.c
> @@ -44,6 +44,7 @@
>  #include "scrub/newbt.h"
>  #include "scrub/reap.h"
>  #include "scrub/rcbag.h"
> +#include "scrub/refcount.h"
>  
>  /*
>   * Rebuilding the Reference Count Btree
> @@ -266,42 +267,6 @@ xrep_rtrefc_walk_rmaps(
>  	return 0;
>  }
>  
> -static inline uint32_t
> -xrep_rtrefc_encode_startblock(
> -	const struct xfs_refcount_irec	*irec)
> -{
> -	uint32_t			start;
> -
> -	start = irec->rc_startblock & ~XFS_REFC_COWFLAG;
> -	if (irec->rc_domain == XFS_REFC_DOMAIN_COW)
> -		start |= XFS_REFC_COWFLAG;
> -
> -	return start;
> -}
> -
> -/*
> - * Compare two refcount records.  We want to sort in order of increasing block
> - * number.
> - */
> -static int
> -xrep_rtrefc_extent_cmp(
> -	const void			*a,
> -	const void			*b)
> -{
> -	const struct xfs_refcount_irec	*ap = a;
> -	const struct xfs_refcount_irec	*bp = b;
> -	uint32_t			sa, sb;
> -
> -	sa = xrep_rtrefc_encode_startblock(ap);
> -	sb = xrep_rtrefc_encode_startblock(bp);
> -
> -	if (sa > sb)
> -		return 1;
> -	if (sa < sb)
> -		return -1;
> -	return 0;
> -}
> -
>  /*
>   * Sort the refcount extents by startblock or else the btree records will be in
>   * the wrong order.  Make sure the records do not overlap in physical space.
> @@ -316,7 +281,7 @@ xrep_rtrefc_sort_records(
>  	xfs_rgblock_t			next_rgbno = 0;
>  	int				error;
>  
> -	error = xfarray_sort(rr->refcount_records, xrep_rtrefc_extent_cmp,
> +	error = xfarray_sort(rr->refcount_records, xrep_refc_extent_cmp,
>  			XFARRAY_SORT_KILLABLE);
>  	if (error)
>  		return error;
> -- 
> 2.55.0
> 
> 

Re: [PATCH 0/3] xfs: more misc code deduplication

[Carlos Maiolino]

On Fri, 02 Oct 2026 16:08:08 -0500, Eric Sandeen wrote:
> Reduce more cut and paste.
> 
> These 3 are independent IIRC, so if anything gives anyone heartburn we
> can just drop the patch(es).
> 
>  libxfs/xfs_dir2_sf.c      |   68 +++++------
>  scrub/attr_repair.c       |   12 --
>  scrub/dir_repair.c        |   11 -
>  scrub/refcount.c          |   27 ----
>  scrub/refcount.h          |   56 +++++++++
>  scrub/refcount_repair.c   |    3
>  scrub/repair.c            |   16 ++
>  scrub/repair.h            |    1
>  scrub/rtrefcount.c        |  263 +-------------------------------------------
>  scrub/rtrefcount_repair.c |   39 ------
>  scrub/symlink_repair.c    |   11 -
>  11 files changed, 122 insertions(+), 385 deletions(-)
> 
> [...]

Applied to for-next, thanks!

[1/3] xfs: factor out xfs_dir2_sf_copy_entries helper
      commit: e029aeece84b0fe4f0aa6b1dd64c0c1c49281cde
[2/3] xfs: factor out xrep_reset_fork_to_extents helper for scrub/repair
      commit: 1e3f1adf6d533b503bc2356129bd7e27c199ad5b
[3/3] xfs: re-use refcount scrub/repair code for rtrefcount
      commit: 1d55a10278054fbebb9f571c99c32dac42327361

Best regards,
-- 
Carlos Maiolino <cem@kernel.org>