repair tidyups for metadir handling

repair tidyups for metadir handling

[Christoph Hellwig]

Hi all,

I recently played with adding another metafile type, and of course
messed up my first attempt.  This series enhances the metadir
repair code a bit based on things I found while debugging that,
and in the last patch changes repair to warn about unexpected
metafile types.

[PATCH 1/4] repair: add a enum for the XR_INO_* values

[Christoph Hellwig]

Move the XR_INO_ definitions into dinode.c as they aren't used anywhere
else, and turn them into an enum to improve type safety.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 repair/dinode.c | 52 +++++++++++++++++++++++++++++++++++--------------
 repair/incore.h | 19 ------------------
 2 files changed, 37 insertions(+), 34 deletions(-)

diff --git a/repair/dinode.c b/repair/dinode.c
index 8ca0aa0238c7..b824dfc0a59f 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -23,6 +23,26 @@
 #include "bmap_repair.h"
 #include "rt.h"
 
+/* inode types */
+enum xr_ino_type {
+	XR_INO_UNKNOWN,		/* unknown */
+	XR_INO_DIR,		/* directory */
+	XR_INO_RTDATA,		/* realtime file */
+	XR_INO_RTBITMAP,	/* realtime bitmap inode */
+	XR_INO_RTSUM,		/* realtime summary inode */
+	XR_INO_DATA,		/* regular file */
+	XR_INO_SYMLINK,		/* symlink */
+	XR_INO_CHRDEV,		/* character device */
+	XR_INO_BLKDEV,		/* block device */
+	XR_INO_SOCK,		/* socket */
+	XR_INO_FIFO,		/* fifo */
+	XR_INO_UQUOTA,		/* user quota inode */
+	XR_INO_GQUOTA,		/* group quota inode */
+	XR_INO_PQUOTA,		/* project quota inode */
+	XR_INO_RTRMAP,		/* realtime rmap */
+	XR_INO_RTREFC,		/* realtime refcount */
+};
+
 /*
  * gettext lookups for translations of strings use mutexes internally to
  * the library. Hence when we come through here doing parallel scans in
@@ -482,7 +502,7 @@ out_unlock:
 static inline bool
 is_reflink_type(
 	struct xfs_mount	*mp,
-	int			type)
+	enum xr_ino_type	type)
 {
 	if (type == XR_INO_DATA && xfs_has_reflink(mp))
 		return true;
@@ -503,7 +523,7 @@ process_bmbt_reclist_int(
 	xfs_mount_t		*mp,
 	xfs_bmbt_rec_t		*rp,
 	xfs_extnum_t		*numrecs,
-	int			type,
+	enum xr_ino_type	type,
 	xfs_ino_t		ino,
 	xfs_rfsblock_t		*tot,
 	blkmap_t		**blkmapp,
@@ -952,7 +972,7 @@ process_rtrmap(
 	xfs_agnumber_t		agno,
 	xfs_agino_t		ino,
 	struct xfs_dinode	*dip,
-	int			type,
+	enum xr_ino_type	type,
 	int			*dirty,
 	xfs_rfsblock_t		*tot,
 	uint64_t		*nex,
@@ -1123,7 +1143,7 @@ process_rtrefc(
 	xfs_agnumber_t			agno,
 	xfs_agino_t			ino,
 	struct xfs_dinode		*dip,
-	int				type,
+	enum xr_ino_type		type,
 	int				*dirty,
 	xfs_rfsblock_t			*tot,
 	uint64_t			*nex,
@@ -1279,7 +1299,7 @@ process_btinode(
 	xfs_agnumber_t		agno,
 	xfs_agino_t		ino,
 	struct xfs_dinode	*dip,
-	int			type,
+	enum xr_ino_type	type,
 	int			*dirty,
 	xfs_rfsblock_t		*tot,
 	xfs_extnum_t		*nex,
@@ -1455,7 +1475,7 @@ process_exinode(
 	xfs_agnumber_t		agno,
 	xfs_agino_t		ino,
 	struct xfs_dinode	*dip,
-	int			type,
+	enum xr_ino_type	type,
 	int			*dirty,
 	xfs_rfsblock_t		*tot,
 	xfs_extnum_t		*nex,
@@ -1648,7 +1668,7 @@ process_quota_inode(
 	struct xfs_mount	*mp,
 	xfs_ino_t		lino,
 	struct xfs_dinode	*dino,
-	uint			ino_type,
+	enum xr_ino_type	ino_type,
 	struct blkmap		*blkmap)
 {
 	xfs_fsblock_t		fsbno;
@@ -1935,7 +1955,7 @@ process_misc_ino_types(
 	xfs_mount_t		*mp,
 	struct xfs_dinode	*dino,
 	xfs_ino_t		lino,
-	int			type)
+	enum xr_ino_type	type)
 {
 	/*
 	 * must also have a zero size
@@ -1982,7 +2002,10 @@ _("size of quota inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"), lino,
 }
 
 static int
-process_misc_ino_types_blocks(xfs_rfsblock_t totblocks, xfs_ino_t lino, int type)
+process_misc_ino_types_blocks(
+	xfs_rfsblock_t		totblocks,
+	xfs_ino_t		lino,
+	enum xr_ino_type	type)
 {
 	/*
 	 * you can not enforce all misc types have zero data fork blocks
@@ -2092,7 +2115,7 @@ process_check_rt_inode(
 	struct xfs_mount	*mp,
 	struct xfs_dinode	*dinoc,
 	xfs_ino_t		lino,
-	int			*type,
+	enum xr_ino_type	*type,
 	int			*dirty,
 	int			expected_type,
 	const char		*tag)
@@ -2130,7 +2153,7 @@ process_check_metadata_inodes(
 	xfs_mount_t		*mp,
 	struct xfs_dinode	*dinoc,
 	xfs_ino_t		lino,
-	int			*type,
+	enum xr_ino_type	*type,
 	int			*dirty)
 {
 	if (lino == mp->m_sb.sb_rootino) {
@@ -2205,7 +2228,7 @@ process_check_inode_sizes(
 	xfs_mount_t		*mp,
 	struct xfs_dinode	*dino,
 	xfs_ino_t		lino,
-	int			type)
+	enum xr_ino_type	type)
 {
 	xfs_fsize_t		size = be64_to_cpu(dino->di_size);
 
@@ -2466,7 +2489,7 @@ process_inode_data_fork(
 	xfs_agnumber_t		agno,
 	xfs_agino_t		ino,
 	struct xfs_dinode	**dinop,
-	int			type,
+	enum xr_ino_type	type,
 	int			*dirty,
 	xfs_rfsblock_t		*totblocks,
 	xfs_extnum_t		*nextents,
@@ -3029,10 +3052,10 @@ process_dinode_int(
 	xfs_ino_t		*parent,	/* out -- parent if ino is a dir */
 	struct xfs_buf		**ino_bpp)
 {
+	enum xr_ino_type	type = XR_INO_UNKNOWN;
 	xfs_rfsblock_t		totblocks = 0;
 	xfs_rfsblock_t		atotblocks = 0;
 	int			di_mode;
-	int			type;
 	int			retval = 0;
 	xfs_extnum_t		nextents;
 	xfs_extnum_t		anextents;
@@ -3048,7 +3071,6 @@ process_dinode_int(
 
 	*dirty = *isa_dir = 0;
 	*used = is_used;
-	type = XR_INO_UNKNOWN;
 
 	lino = XFS_AGINO_TO_INO(mp, agno, ino);
 	di_mode = be16_to_cpu(dino->di_mode);
diff --git a/repair/incore.h b/repair/incore.h
index 57019148f588..293988c9769d 100644
--- a/repair/incore.h
+++ b/repair/incore.h
@@ -225,25 +225,6 @@ int		count_bcnt_extents(xfs_agnumber_t);
  * inode definitions
  */
 
-/* inode types */
-
-#define XR_INO_UNKNOWN	0		/* unknown */
-#define XR_INO_DIR	1		/* directory */
-#define XR_INO_RTDATA	2		/* realtime file */
-#define XR_INO_RTBITMAP	3		/* realtime bitmap inode */
-#define XR_INO_RTSUM	4		/* realtime summary inode */
-#define XR_INO_DATA	5		/* regular file */
-#define XR_INO_SYMLINK	6		/* symlink */
-#define XR_INO_CHRDEV	7		/* character device */
-#define XR_INO_BLKDEV	8		/* block device */
-#define XR_INO_SOCK	9		/* socket */
-#define XR_INO_FIFO	10		/* fifo */
-#define XR_INO_UQUOTA	12		/* user quota inode */
-#define XR_INO_GQUOTA	13		/* group quota inode */
-#define XR_INO_PQUOTA	14		/* project quota inode */
-#define XR_INO_RTRMAP	15		/* realtime rmap */
-#define XR_INO_RTREFC	16		/* realtime refcount */
-
 /* inode allocation tree */
 
 /*
-- 
2.47.3

Re: [PATCH 1/4] repair: add a enum for the XR_INO_* values

[Carlos Maiolino]

On Fri, Nov 28, 2025 at 07:36:59AM +0100, Christoph Hellwig wrote:
> Move the XR_INO_ definitions into dinode.c as they aren't used anywhere
> else, and turn them into an enum to improve type safety.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  repair/dinode.c | 52 +++++++++++++++++++++++++++++++++++--------------
>  repair/incore.h | 19 ------------------
>  2 files changed, 37 insertions(+), 34 deletions(-)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 8ca0aa0238c7..b824dfc0a59f 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -23,6 +23,26 @@
>  #include "bmap_repair.h"
>  #include "rt.h"
> 
> +/* inode types */
> +enum xr_ino_type {
> +	XR_INO_UNKNOWN,		/* unknown */
> +	XR_INO_DIR,		/* directory */
> +	XR_INO_RTDATA,		/* realtime file */
> +	XR_INO_RTBITMAP,	/* realtime bitmap inode */
> +	XR_INO_RTSUM,		/* realtime summary inode */
> +	XR_INO_DATA,		/* regular file */
> +	XR_INO_SYMLINK,		/* symlink */
> +	XR_INO_CHRDEV,		/* character device */
> +	XR_INO_BLKDEV,		/* block device */
> +	XR_INO_SOCK,		/* socket */
> +	XR_INO_FIFO,		/* fifo */
> +	XR_INO_UQUOTA,		/* user quota inode */
> +	XR_INO_GQUOTA,		/* group quota inode */
> +	XR_INO_PQUOTA,		/* project quota inode */
> +	XR_INO_RTRMAP,		/* realtime rmap */
> +	XR_INO_RTREFC,		/* realtime refcount */

I think those comments are redundant as the enums are mostly
self-descriptive, but independent of that the patch looks good.

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

> +};
> +
>  /*
>   * gettext lookups for translations of strings use mutexes internally to
>   * the library. Hence when we come through here doing parallel scans in
> @@ -482,7 +502,7 @@ out_unlock:
>  static inline bool
>  is_reflink_type(
>  	struct xfs_mount	*mp,
> -	int			type)
> +	enum xr_ino_type	type)
>  {
>  	if (type == XR_INO_DATA && xfs_has_reflink(mp))
>  		return true;
> @@ -503,7 +523,7 @@ process_bmbt_reclist_int(
>  	xfs_mount_t		*mp,
>  	xfs_bmbt_rec_t		*rp,
>  	xfs_extnum_t		*numrecs,
> -	int			type,
> +	enum xr_ino_type	type,
>  	xfs_ino_t		ino,
>  	xfs_rfsblock_t		*tot,
>  	blkmap_t		**blkmapp,
> @@ -952,7 +972,7 @@ process_rtrmap(
>  	xfs_agnumber_t		agno,
>  	xfs_agino_t		ino,
>  	struct xfs_dinode	*dip,
> -	int			type,
> +	enum xr_ino_type	type,
>  	int			*dirty,
>  	xfs_rfsblock_t		*tot,
>  	uint64_t		*nex,
> @@ -1123,7 +1143,7 @@ process_rtrefc(
>  	xfs_agnumber_t			agno,
>  	xfs_agino_t			ino,
>  	struct xfs_dinode		*dip,
> -	int				type,
> +	enum xr_ino_type		type,
>  	int				*dirty,
>  	xfs_rfsblock_t			*tot,
>  	uint64_t			*nex,
> @@ -1279,7 +1299,7 @@ process_btinode(
>  	xfs_agnumber_t		agno,
>  	xfs_agino_t		ino,
>  	struct xfs_dinode	*dip,
> -	int			type,
> +	enum xr_ino_type	type,
>  	int			*dirty,
>  	xfs_rfsblock_t		*tot,
>  	xfs_extnum_t		*nex,
> @@ -1455,7 +1475,7 @@ process_exinode(
>  	xfs_agnumber_t		agno,
>  	xfs_agino_t		ino,
>  	struct xfs_dinode	*dip,
> -	int			type,
> +	enum xr_ino_type	type,
>  	int			*dirty,
>  	xfs_rfsblock_t		*tot,
>  	xfs_extnum_t		*nex,
> @@ -1648,7 +1668,7 @@ process_quota_inode(
>  	struct xfs_mount	*mp,
>  	xfs_ino_t		lino,
>  	struct xfs_dinode	*dino,
> -	uint			ino_type,
> +	enum xr_ino_type	ino_type,
>  	struct blkmap		*blkmap)
>  {
>  	xfs_fsblock_t		fsbno;
> @@ -1935,7 +1955,7 @@ process_misc_ino_types(
>  	xfs_mount_t		*mp,
>  	struct xfs_dinode	*dino,
>  	xfs_ino_t		lino,
> -	int			type)
> +	enum xr_ino_type	type)
>  {
>  	/*
>  	 * must also have a zero size
> @@ -1982,7 +2002,10 @@ _("size of quota inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"), lino,
>  }
> 
>  static int
> -process_misc_ino_types_blocks(xfs_rfsblock_t totblocks, xfs_ino_t lino, int type)
> +process_misc_ino_types_blocks(
> +	xfs_rfsblock_t		totblocks,
> +	xfs_ino_t		lino,
> +	enum xr_ino_type	type)
>  {
>  	/*
>  	 * you can not enforce all misc types have zero data fork blocks
> @@ -2092,7 +2115,7 @@ process_check_rt_inode(
>  	struct xfs_mount	*mp,
>  	struct xfs_dinode	*dinoc,
>  	xfs_ino_t		lino,
> -	int			*type,
> +	enum xr_ino_type	*type,
>  	int			*dirty,
>  	int			expected_type,
>  	const char		*tag)
> @@ -2130,7 +2153,7 @@ process_check_metadata_inodes(
>  	xfs_mount_t		*mp,
>  	struct xfs_dinode	*dinoc,
>  	xfs_ino_t		lino,
> -	int			*type,
> +	enum xr_ino_type	*type,
>  	int			*dirty)
>  {
>  	if (lino == mp->m_sb.sb_rootino) {
> @@ -2205,7 +2228,7 @@ process_check_inode_sizes(
>  	xfs_mount_t		*mp,
>  	struct xfs_dinode	*dino,
>  	xfs_ino_t		lino,
> -	int			type)
> +	enum xr_ino_type	type)
>  {
>  	xfs_fsize_t		size = be64_to_cpu(dino->di_size);
> 
> @@ -2466,7 +2489,7 @@ process_inode_data_fork(
>  	xfs_agnumber_t		agno,
>  	xfs_agino_t		ino,
>  	struct xfs_dinode	**dinop,
> -	int			type,
> +	enum xr_ino_type	type,
>  	int			*dirty,
>  	xfs_rfsblock_t		*totblocks,
>  	xfs_extnum_t		*nextents,
> @@ -3029,10 +3052,10 @@ process_dinode_int(
>  	xfs_ino_t		*parent,	/* out -- parent if ino is a dir */
>  	struct xfs_buf		**ino_bpp)
>  {
> +	enum xr_ino_type	type = XR_INO_UNKNOWN;
>  	xfs_rfsblock_t		totblocks = 0;
>  	xfs_rfsblock_t		atotblocks = 0;
>  	int			di_mode;
> -	int			type;
>  	int			retval = 0;
>  	xfs_extnum_t		nextents;
>  	xfs_extnum_t		anextents;
> @@ -3048,7 +3071,6 @@ process_dinode_int(
> 
>  	*dirty = *isa_dir = 0;
>  	*used = is_used;
> -	type = XR_INO_UNKNOWN;
> 
>  	lino = XFS_AGINO_TO_INO(mp, agno, ino);
>  	di_mode = be16_to_cpu(dino->di_mode);
> diff --git a/repair/incore.h b/repair/incore.h
> index 57019148f588..293988c9769d 100644
> --- a/repair/incore.h
> +++ b/repair/incore.h
> @@ -225,25 +225,6 @@ int		count_bcnt_extents(xfs_agnumber_t);
>   * inode definitions
>   */
> 
> -/* inode types */
> -
> -#define XR_INO_UNKNOWN	0		/* unknown */
> -#define XR_INO_DIR	1		/* directory */
> -#define XR_INO_RTDATA	2		/* realtime file */
> -#define XR_INO_RTBITMAP	3		/* realtime bitmap inode */
> -#define XR_INO_RTSUM	4		/* realtime summary inode */
> -#define XR_INO_DATA	5		/* regular file */
> -#define XR_INO_SYMLINK	6		/* symlink */
> -#define XR_INO_CHRDEV	7		/* character device */
> -#define XR_INO_BLKDEV	8		/* block device */
> -#define XR_INO_SOCK	9		/* socket */
> -#define XR_INO_FIFO	10		/* fifo */
> -#define XR_INO_UQUOTA	12		/* user quota inode */
> -#define XR_INO_GQUOTA	13		/* group quota inode */
> -#define XR_INO_PQUOTA	14		/* project quota inode */
> -#define XR_INO_RTRMAP	15		/* realtime rmap */
> -#define XR_INO_RTREFC	16		/* realtime refcount */
> -
>  /* inode allocation tree */
> 
>  /*
> --
> 2.47.3
> 
> 

Re: [PATCH 1/4] repair: add a enum for the XR_INO_* values

[Christoph Hellwig]

On Fri, Nov 28, 2025 at 08:53:16AM +0100, Carlos Maiolino wrote:
> I think those comments are redundant as the enums are mostly
> self-descriptive, but independent of that the patch looks good.

Most seem on their own, but given that this is mixing file type, data vs
rt device, and magic metafiles into a single enum I think it's worth
keeping.

Re: [PATCH 1/4] repair: add a enum for the XR_INO_* values

[Carlos Maiolino]

On Mon, Dec 01, 2025 at 07:22:41AM +0100, Christoph Hellwig wrote:
> On Fri, Nov 28, 2025 at 08:53:16AM +0100, Carlos Maiolino wrote:
> > I think those comments are redundant as the enums are mostly
> > self-descriptive, but independent of that the patch looks good.
> 
> Most seem on their own, but given that this is mixing file type, data vs
> rt device, and magic metafiles into a single enum I think it's worth
> keeping.

Reasonable enough.

Re: [PATCH 1/4] repair: add a enum for the XR_INO_* values

[Darrick J. Wong]

On Mon, Dec 01, 2025 at 10:00:55AM +0100, Carlos Maiolino wrote:
> On Mon, Dec 01, 2025 at 07:22:41AM +0100, Christoph Hellwig wrote:
> > On Fri, Nov 28, 2025 at 08:53:16AM +0100, Carlos Maiolino wrote:
> > > I think those comments are redundant as the enums are mostly
> > > self-descriptive, but independent of that the patch looks good.
> > 
> > Most seem on their own, but given that this is mixing file type, data vs
> > rt device, and magic metafiles into a single enum I think it's worth
> > keeping.
> 
> Reasonable enough.

Agreed, I don't mind the extra comments.

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

[PATCH 2/4] repair: add canonical names for the XR_INO_ constants

[Christoph Hellwig]

Add an array with the canonical name for each inode type so that code
doesn't have to implement switch statements for that, and remove the now
trivial process_misc_ino_types and process_misc_ino_types_blocks
functions.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 repair/dinode.c | 157 +++++++++++++++---------------------------------
 1 file changed, 48 insertions(+), 109 deletions(-)

diff --git a/repair/dinode.c b/repair/dinode.c
index b824dfc0a59f..fd40fdcce665 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -43,6 +43,25 @@ enum xr_ino_type {
 	XR_INO_RTREFC,		/* realtime refcount */
 };
 
+static const char *xr_ino_type_name[] = {
+	[XR_INO_UNKNOWN]	= "unknown",
+	[XR_INO_DIR]		= "directory",
+	[XR_INO_RTDATA]		= "realtime file",
+	[XR_INO_RTBITMAP]	= "realtime bitmap",
+	[XR_INO_RTSUM]		= "realtime summary",
+	[XR_INO_DATA]		= "regular file",
+	[XR_INO_SYMLINK]	= "symlink",
+	[XR_INO_CHRDEV]		= "character device",
+	[XR_INO_BLKDEV]		= "block device",
+	[XR_INO_SOCK]		= "socket",
+	[XR_INO_FIFO]		= "fifo",
+	[XR_INO_UQUOTA]		= "user quota",
+	[XR_INO_GQUOTA]		= "group quota",
+	[XR_INO_PQUOTA]		= "project quota",
+	[XR_INO_RTRMAP]		= "realtime rmap",
+	[XR_INO_RTREFC]		= "realtime refcount",
+};
+
 /*
  * gettext lookups for translations of strings use mutexes internally to
  * the library. Hence when we come through here doing parallel scans in
@@ -1946,106 +1965,6 @@ _("found illegal null character in symlink inode %" PRIu64 "\n"),
 	return(0);
 }
 
-/*
- * called to process the set of misc inode special inode types
- * that have no associated data storage (fifos, pipes, devices, etc.).
- */
-static int
-process_misc_ino_types(
-	xfs_mount_t		*mp,
-	struct xfs_dinode	*dino,
-	xfs_ino_t		lino,
-	enum xr_ino_type	type)
-{
-	/*
-	 * must also have a zero size
-	 */
-	if (be64_to_cpu(dino->di_size) != 0)  {
-		switch (type)  {
-		case XR_INO_CHRDEV:
-			do_warn(
-_("size of character device inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"), lino,
-				(int64_t)be64_to_cpu(dino->di_size));
-			break;
-		case XR_INO_BLKDEV:
-			do_warn(
-_("size of block device inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"), lino,
-				(int64_t)be64_to_cpu(dino->di_size));
-			break;
-		case XR_INO_SOCK:
-			do_warn(
-_("size of socket inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"), lino,
-				(int64_t)be64_to_cpu(dino->di_size));
-			break;
-		case XR_INO_FIFO:
-			do_warn(
-_("size of fifo inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"), lino,
-				(int64_t)be64_to_cpu(dino->di_size));
-			break;
-		case XR_INO_UQUOTA:
-		case XR_INO_GQUOTA:
-		case XR_INO_PQUOTA:
-			do_warn(
-_("size of quota inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"), lino,
-				(int64_t)be64_to_cpu(dino->di_size));
-			break;
-		default:
-			do_warn(_("Internal error - process_misc_ino_types, "
-				  "illegal type %d\n"), type);
-			abort();
-		}
-
-		return(1);
-	}
-
-	return(0);
-}
-
-static int
-process_misc_ino_types_blocks(
-	xfs_rfsblock_t		totblocks,
-	xfs_ino_t		lino,
-	enum xr_ino_type	type)
-{
-	/*
-	 * you can not enforce all misc types have zero data fork blocks
-	 * by checking dino->di_nblocks because atotblocks (attribute
-	 * blocks) are part of nblocks. We must check this later when atotblocks
-	 * has been calculated or by doing a simple check that anExtents == 0.
-	 * We must also guarantee that totblocks is 0. Thus nblocks checking
-	 * will be done later in process_dinode_int for misc types.
-	 */
-
-	if (totblocks != 0)  {
-		switch (type)  {
-		case XR_INO_CHRDEV:
-			do_warn(
-_("size of character device inode %" PRIu64 " != 0 (%" PRIu64 " blocks)\n"),
-				lino, totblocks);
-			break;
-		case XR_INO_BLKDEV:
-			do_warn(
-_("size of block device inode %" PRIu64 " != 0 (%" PRIu64 " blocks)\n"),
-				lino, totblocks);
-			break;
-		case XR_INO_SOCK:
-			do_warn(
-_("size of socket inode %" PRIu64 " != 0 (%" PRIu64 " blocks)\n"),
-				lino, totblocks);
-			break;
-		case XR_INO_FIFO:
-			do_warn(
-_("size of fifo inode %" PRIu64 " != 0 (%" PRIu64 " blocks)\n"),
-				lino, totblocks);
-			break;
-		default:
-			return(0);
-		}
-		return(1);
-	}
-	return (0);
-}
-
 static inline int
 dinode_fmt(
 	struct xfs_dinode *dino)
@@ -2261,16 +2180,20 @@ _("directory inode %" PRIu64 " has bad size %" PRId64 "\n"),
 	case XR_INO_BLKDEV:
 	case XR_INO_SOCK:
 	case XR_INO_FIFO:
-		if (process_misc_ino_types(mp, dino, lino, type))
-			return 1;
-		break;
-
 	case XR_INO_UQUOTA:
 	case XR_INO_GQUOTA:
 	case XR_INO_PQUOTA:
-		/* Quota inodes have same restrictions as above types */
-		if (process_misc_ino_types(mp, dino, lino, type))
+		/*
+		 * Misc inode types that have no associated data storage (fifos,
+		 * pipes, devices, etc.) mad thus must also have a zero size.
+		 */
+		if (dino->di_size != 0)  {
+			do_warn(
+_("size of %s inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"),
+				xr_ino_type_name[type], lino,
+				(int64_t)be64_to_cpu(dino->di_size));
 			return 1;
+		}
 		break;
 
 	case XR_INO_RTDATA:
@@ -3704,10 +3627,26 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 	dino = *dinop;
 
 	/*
-	 * enforce totblocks is 0 for misc types
+	 * Enforce totblocks is 0 for misc types.
+	 *
+	 * Note that di_nblocks includes attribute fork blocks, so we can only
+	 * do this here instead of when reading the inode.
 	 */
-	if (process_misc_ino_types_blocks(totblocks, lino, type))
-		goto clear_bad_out;
+	switch (type)  {
+	case XR_INO_CHRDEV:
+	case XR_INO_BLKDEV:
+	case XR_INO_SOCK:
+	case XR_INO_FIFO:
+		if (totblocks != 0)  {
+			do_warn(
+_("size of %s inode %" PRIu64 " != 0 (%" PRIu64 " blocks)\n"),
+				xr_ino_type_name[type], lino, totblocks);
+			goto clear_bad_out;
+		}
+		break;
+	default:
+		break;
+	}
 
 	/*
 	 * correct space counters if required
-- 
2.47.3

Re: [PATCH 2/4] repair: add canonical names for the XR_INO_ constants

[Carlos Maiolino]

On Fri, Nov 28, 2025 at 07:37:00AM +0100, Christoph Hellwig wrote:
> Add an array with the canonical name for each inode type so that code
> doesn't have to implement switch statements for that, and remove the now
> trivial process_misc_ino_types and process_misc_ino_types_blocks
> functions.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> -


>  static inline int
>  dinode_fmt(
>  	struct xfs_dinode *dino)
> @@ -2261,16 +2180,20 @@ _("directory inode %" PRIu64 " has bad size %" PRId64 "\n"),
>  	case XR_INO_BLKDEV:
>  	case XR_INO_SOCK:
>  	case XR_INO_FIFO:
> -		if (process_misc_ino_types(mp, dino, lino, type))
> -			return 1;
> -		break;
> -
>  	case XR_INO_UQUOTA:
>  	case XR_INO_GQUOTA:
>  	case XR_INO_PQUOTA:
> -		/* Quota inodes have same restrictions as above types */
> -		if (process_misc_ino_types(mp, dino, lino, type))
> +		/*
> +		 * Misc inode types that have no associated data storage (fifos,
> +		 * pipes, devices, etc.) mad thus must also have a zero size.

		Perhaps is my non-native English, but this sentence
		doesn't make much sense to me. Not sure if 'mad' has
		some special meaning in this context?

Other than that, the patch looks good.

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


> +		 */
> +		if (dino->di_size != 0)  {
> +			do_warn(
> +_("size of %s inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"),
> +				xr_ino_type_name[type], lino,
> +				(int64_t)be64_to_cpu(dino->di_size));
>  			return 1;
> +		}
>  		break;
> 
>  	case XR_INO_RTDATA:
> @@ -3704,10 +3627,26 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  	dino = *dinop;
> 
>  	/*
> -	 * enforce totblocks is 0 for misc types
> +	 * Enforce totblocks is 0 for misc types.
> +	 *
> +	 * Note that di_nblocks includes attribute fork blocks, so we can only
> +	 * do this here instead of when reading the inode.
>  	 */
> -	if (process_misc_ino_types_blocks(totblocks, lino, type))
> -		goto clear_bad_out;
> +	switch (type)  {
> +	case XR_INO_CHRDEV:
> +	case XR_INO_BLKDEV:
> +	case XR_INO_SOCK:
> +	case XR_INO_FIFO:
> +		if (totblocks != 0)  {
> +			do_warn(
> +_("size of %s inode %" PRIu64 " != 0 (%" PRIu64 " blocks)\n"),
> +				xr_ino_type_name[type], lino, totblocks);
> +			goto clear_bad_out;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> 
>  	/*
>  	 * correct space counters if required
> --
> 2.47.3
> 
> 

Re: [PATCH 2/4] repair: add canonical names for the XR_INO_ constants

[Christoph Hellwig]

On Fri, Nov 28, 2025 at 09:00:59AM +0100, Carlos Maiolino wrote:
> On Fri, Nov 28, 2025 at 07:37:00AM +0100, Christoph Hellwig wrote:
> > Add an array with the canonical name for each inode type so that code
> > doesn't have to implement switch statements for that, and remove the now
> > trivial process_misc_ino_types and process_misc_ino_types_blocks
> > functions.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > -
> 
> 
> >  static inline int
> >  dinode_fmt(
> >  	struct xfs_dinode *dino)
> > @@ -2261,16 +2180,20 @@ _("directory inode %" PRIu64 " has bad size %" PRId64 "\n"),
> >  	case XR_INO_BLKDEV:
> >  	case XR_INO_SOCK:
> >  	case XR_INO_FIFO:
> > -		if (process_misc_ino_types(mp, dino, lino, type))
> > -			return 1;
> > -		break;
> > -
> >  	case XR_INO_UQUOTA:
> >  	case XR_INO_GQUOTA:
> >  	case XR_INO_PQUOTA:
> > -		/* Quota inodes have same restrictions as above types */
> > -		if (process_misc_ino_types(mp, dino, lino, type))
> > +		/*
> > +		 * Misc inode types that have no associated data storage (fifos,
> > +		 * pipes, devices, etc.) mad thus must also have a zero size.
> 
> 		Perhaps is my non-native English, but this sentence
> 		doesn't make much sense to me. Not sure if 'mad' has
> 		some special meaning in this context?

that should be an "and"

Re: [PATCH 2/4] repair: add canonical names for the XR_INO_ constants

[Darrick J. Wong]

On Fri, Nov 28, 2025 at 07:37:00AM +0100, Christoph Hellwig wrote:
> Add an array with the canonical name for each inode type so that code
> doesn't have to implement switch statements for that, and remove the now
> trivial process_misc_ino_types and process_misc_ino_types_blocks
> functions.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  repair/dinode.c | 157 +++++++++++++++---------------------------------
>  1 file changed, 48 insertions(+), 109 deletions(-)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index b824dfc0a59f..fd40fdcce665 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -43,6 +43,25 @@ enum xr_ino_type {
>  	XR_INO_RTREFC,		/* realtime refcount */
>  };
>  
> +static const char *xr_ino_type_name[] = {
> +	[XR_INO_UNKNOWN]	= "unknown",
> +	[XR_INO_DIR]		= "directory",
> +	[XR_INO_RTDATA]		= "realtime file",
> +	[XR_INO_RTBITMAP]	= "realtime bitmap",
> +	[XR_INO_RTSUM]		= "realtime summary",
> +	[XR_INO_DATA]		= "regular file",
> +	[XR_INO_SYMLINK]	= "symlink",
> +	[XR_INO_CHRDEV]		= "character device",
> +	[XR_INO_BLKDEV]		= "block device",
> +	[XR_INO_SOCK]		= "socket",
> +	[XR_INO_FIFO]		= "fifo",
> +	[XR_INO_UQUOTA]		= "user quota",
> +	[XR_INO_GQUOTA]		= "group quota",
> +	[XR_INO_PQUOTA]		= "project quota",
> +	[XR_INO_RTRMAP]		= "realtime rmap",
> +	[XR_INO_RTREFC]		= "realtime refcount",
> +};

Might want a

BUILD_BUG_ON(ARRAY_SIZED(xr_ino_type_name) != XR_INO_MAX);

so that someone adding to the enum won't forget to add a string here.

> +
>  /*
>   * gettext lookups for translations of strings use mutexes internally to
>   * the library. Hence when we come through here doing parallel scans in
> @@ -1946,106 +1965,6 @@ _("found illegal null character in symlink inode %" PRIu64 "\n"),
>  	return(0);
>  }
>  
> -/*
> - * called to process the set of misc inode special inode types
> - * that have no associated data storage (fifos, pipes, devices, etc.).
> - */
> -static int
> -process_misc_ino_types(
> -	xfs_mount_t		*mp,
> -	struct xfs_dinode	*dino,
> -	xfs_ino_t		lino,
> -	enum xr_ino_type	type)
> -{
> -	/*
> -	 * must also have a zero size
> -	 */
> -	if (be64_to_cpu(dino->di_size) != 0)  {
> -		switch (type)  {
> -		case XR_INO_CHRDEV:
> -			do_warn(
> -_("size of character device inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"), lino,
> -				(int64_t)be64_to_cpu(dino->di_size));
> -			break;
> -		case XR_INO_BLKDEV:
> -			do_warn(
> -_("size of block device inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"), lino,
> -				(int64_t)be64_to_cpu(dino->di_size));
> -			break;
> -		case XR_INO_SOCK:
> -			do_warn(
> -_("size of socket inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"), lino,
> -				(int64_t)be64_to_cpu(dino->di_size));
> -			break;
> -		case XR_INO_FIFO:
> -			do_warn(
> -_("size of fifo inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"), lino,
> -				(int64_t)be64_to_cpu(dino->di_size));
> -			break;
> -		case XR_INO_UQUOTA:
> -		case XR_INO_GQUOTA:
> -		case XR_INO_PQUOTA:
> -			do_warn(
> -_("size of quota inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"), lino,
> -				(int64_t)be64_to_cpu(dino->di_size));
> -			break;
> -		default:
> -			do_warn(_("Internal error - process_misc_ino_types, "
> -				  "illegal type %d\n"), type);
> -			abort();
> -		}
> -
> -		return(1);
> -	}
> -
> -	return(0);
> -}
> -
> -static int
> -process_misc_ino_types_blocks(
> -	xfs_rfsblock_t		totblocks,
> -	xfs_ino_t		lino,
> -	enum xr_ino_type	type)
> -{
> -	/*
> -	 * you can not enforce all misc types have zero data fork blocks
> -	 * by checking dino->di_nblocks because atotblocks (attribute
> -	 * blocks) are part of nblocks. We must check this later when atotblocks
> -	 * has been calculated or by doing a simple check that anExtents == 0.
> -	 * We must also guarantee that totblocks is 0. Thus nblocks checking
> -	 * will be done later in process_dinode_int for misc types.
> -	 */
> -
> -	if (totblocks != 0)  {
> -		switch (type)  {
> -		case XR_INO_CHRDEV:
> -			do_warn(
> -_("size of character device inode %" PRIu64 " != 0 (%" PRIu64 " blocks)\n"),
> -				lino, totblocks);
> -			break;
> -		case XR_INO_BLKDEV:
> -			do_warn(
> -_("size of block device inode %" PRIu64 " != 0 (%" PRIu64 " blocks)\n"),
> -				lino, totblocks);
> -			break;
> -		case XR_INO_SOCK:
> -			do_warn(
> -_("size of socket inode %" PRIu64 " != 0 (%" PRIu64 " blocks)\n"),
> -				lino, totblocks);
> -			break;
> -		case XR_INO_FIFO:
> -			do_warn(
> -_("size of fifo inode %" PRIu64 " != 0 (%" PRIu64 " blocks)\n"),
> -				lino, totblocks);
> -			break;
> -		default:
> -			return(0);
> -		}
> -		return(1);
> -	}
> -	return (0);
> -}
> -
>  static inline int
>  dinode_fmt(
>  	struct xfs_dinode *dino)
> @@ -2261,16 +2180,20 @@ _("directory inode %" PRIu64 " has bad size %" PRId64 "\n"),
>  	case XR_INO_BLKDEV:
>  	case XR_INO_SOCK:
>  	case XR_INO_FIFO:
> -		if (process_misc_ino_types(mp, dino, lino, type))
> -			return 1;
> -		break;
> -
>  	case XR_INO_UQUOTA:
>  	case XR_INO_GQUOTA:
>  	case XR_INO_PQUOTA:
> -		/* Quota inodes have same restrictions as above types */
> -		if (process_misc_ino_types(mp, dino, lino, type))
> +		/*
> +		 * Misc inode types that have no associated data storage (fifos,
> +		 * pipes, devices, etc.) mad thus must also have a zero size.
> +		 */
> +		if (dino->di_size != 0)  {
> +			do_warn(
> +_("size of %s inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"),
> +				xr_ino_type_name[type], lino,

i18n nit: In the old code, the inode description ("realtime bitmap")
would be translated, now they aren't.  I don't know of a good way to
maintain that though...

"el tamaño del realtime bitmap nodo 55 != 0"

               ^^^^^^^^^^^^^^^ whee, English in the middle of Spanish!

I guess you could write "_(xr_ino_type_name[type])" for that second
argument.

That aside, this is a lot less copy pasta at least.

--D

> +				(int64_t)be64_to_cpu(dino->di_size));
>  			return 1;
> +		}
>  		break;
>  
>  	case XR_INO_RTDATA:
> @@ -3704,10 +3627,26 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  	dino = *dinop;
>  
>  	/*
> -	 * enforce totblocks is 0 for misc types
> +	 * Enforce totblocks is 0 for misc types.
> +	 *
> +	 * Note that di_nblocks includes attribute fork blocks, so we can only
> +	 * do this here instead of when reading the inode.
>  	 */
> -	if (process_misc_ino_types_blocks(totblocks, lino, type))
> -		goto clear_bad_out;
> +	switch (type)  {
> +	case XR_INO_CHRDEV:
> +	case XR_INO_BLKDEV:
> +	case XR_INO_SOCK:
> +	case XR_INO_FIFO:
> +		if (totblocks != 0)  {
> +			do_warn(
> +_("size of %s inode %" PRIu64 " != 0 (%" PRIu64 " blocks)\n"),
> +				xr_ino_type_name[type], lino, totblocks);
> +			goto clear_bad_out;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
>  
>  	/*
>  	 * correct space counters if required
> -- 
> 2.47.3
> 
> 

Re: [PATCH 2/4] repair: add canonical names for the XR_INO_ constants

[Christoph Hellwig]

On Mon, Dec 01, 2025 at 02:47:16PM -0800, Darrick J. Wong wrote:
> BUILD_BUG_ON(ARRAY_SIZED(xr_ino_type_name) != XR_INO_MAX);

Or static_assert for the standard userspace version?

> > +_("size of %s inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"),
> > +				xr_ino_type_name[type], lino,
> 
> i18n nit: In the old code, the inode description ("realtime bitmap")
> would be translated, now they aren't.  I don't know of a good way to
> maintain that though...
> 
> "el tamaño del realtime bitmap nodo 55 != 0"
> 
>                ^^^^^^^^^^^^^^^ whee, English in the middle of Spanish!
> 
> I guess you could write "_(xr_ino_type_name[type])" for that second
> argument.

Seems like the standard gettext thing to do is to mark the array
initializers as N_(), which we do in quite a few places.  Not sure
where the actual translation actually gets applied with that, though.

Re: [PATCH 2/4] repair: add canonical names for the XR_INO_ constants

[Darrick J. Wong]

On Tue, Dec 02, 2025 at 08:33:07AM +0100, Christoph Hellwig wrote:
> On Mon, Dec 01, 2025 at 02:47:16PM -0800, Darrick J. Wong wrote:
> > BUILD_BUG_ON(ARRAY_SIZED(xr_ino_type_name) != XR_INO_MAX);
> 
> Or static_assert for the standard userspace version?
> 
> > > +_("size of %s inode %" PRIu64 " != 0 (%" PRId64 " bytes)\n"),
> > > +				xr_ino_type_name[type], lino,
> > 
> > i18n nit: In the old code, the inode description ("realtime bitmap")
> > would be translated, now they aren't.  I don't know of a good way to
> > maintain that though...
> > 
> > "el tamaño del realtime bitmap nodo 55 != 0"
> > 
> >                ^^^^^^^^^^^^^^^ whee, English in the middle of Spanish!
> > 
> > I guess you could write "_(xr_ino_type_name[type])" for that second
> > argument.
> 
> Seems like the standard gettext thing to do is to mark the array
> initializers as N_(), which we do in quite a few places.  Not sure
> where the actual translation actually gets applied with that, though.

That N_() thing is #defined out of existence for compilation in
platform_defs.h:

# define N_(x)			 x

and include/buildrules passes "--keyword=N_" to xgettext so that it'll
find all the N_("string") strings and put them in the message catalog.
I think you still have to use regular _() (aka gettext()) when you pass
them to printf.

IOWs, you'd still have to define the strings array like this:

static const char *xr_ino_type_name[] = {
	[XR_INO_UNKNOWN]	= N_("unknown"),
...
};

to capture "unknown" in the .po file, but I think the printf argument
still has to call gettext() to look up the localized version.

--D

Re: [PATCH 2/4] repair: add canonical names for the XR_INO_ constants

[Christoph Hellwig]

On Tue, Dec 02, 2025 at 09:59:19AM -0800, Darrick J. Wong wrote:
> > Seems like the standard gettext thing to do is to mark the array
> > initializers as N_(), which we do in quite a few places.  Not sure
> > where the actual translation actually gets applied with that, though.
> 
> That N_() thing is #defined out of existence for compilation in
> platform_defs.h:
> 
> # define N_(x)			 x
> 
> and include/buildrules passes "--keyword=N_" to xgettext so that it'll
> find all the N_("string") strings and put them in the message catalog.
> I think you still have to use regular _() (aka gettext()) when you pass
> them to printf.
> 
> IOWs, you'd still have to define the strings array like this:
> 
> static const char *xr_ino_type_name[] = {
> 	[XR_INO_UNKNOWN]	= N_("unknown"),
> ...
> };

Yes, that was the intent.

> to capture "unknown" in the .po file, but I think the printf argument
> still has to call gettext() to look up the localized version.

And that's what I'd expect.  But there are no direct gettext calls
anywhere in xfsprogs, despite quite a bit of N_() usage.  What am I
missing?

Re: [PATCH 2/4] repair: add canonical names for the XR_INO_ constants

[Darrick J. Wong]

On Wed, Dec 03, 2025 at 07:09:42AM +0100, Christoph Hellwig wrote:
> On Tue, Dec 02, 2025 at 09:59:19AM -0800, Darrick J. Wong wrote:
> > > Seems like the standard gettext thing to do is to mark the array
> > > initializers as N_(), which we do in quite a few places.  Not sure
> > > where the actual translation actually gets applied with that, though.
> > 
> > That N_() thing is #defined out of existence for compilation in
> > platform_defs.h:
> > 
> > # define N_(x)			 x
> > 
> > and include/buildrules passes "--keyword=N_" to xgettext so that it'll
> > find all the N_("string") strings and put them in the message catalog.
> > I think you still have to use regular _() (aka gettext()) when you pass
> > them to printf.
> > 
> > IOWs, you'd still have to define the strings array like this:
> > 
> > static const char *xr_ino_type_name[] = {
> > 	[XR_INO_UNKNOWN]	= N_("unknown"),
> > ...
> > };
> 
> Yes, that was the intent.
> 
> > to capture "unknown" in the .po file, but I think the printf argument
> > still has to call gettext() to look up the localized version.
> 
> And that's what I'd expect.  But there are no direct gettext calls
> anywhere in xfsprogs, despite quite a bit of N_() usage.  What am I
> missing?

#define _(x)		gettext(x)

in platform_defs.h.

(and no, I don't get anything meaningful out of "N_()" for magic
tagging and "_()" to mean lookup, but according to Debian codesearch
it's a common practice in things like binutils and git and vim)

--D

Re: [PATCH 2/4] repair: add canonical names for the XR_INO_ constants

[Christoph Hellwig]

On Thu, Dec 04, 2025 at 09:18:11AM -0800, Darrick J. Wong wrote:
> > And that's what I'd expect.  But there are no direct gettext calls
> > anywhere in xfsprogs, despite quite a bit of N_() usage.  What am I
> > missing?
> 
> #define _(x)		gettext(x)
> 
> in platform_defs.h.
> 
> (and no, I don't get anything meaningful out of "N_()" for magic
> tagging and "_()" to mean lookup, but according to Debian codesearch
> it's a common practice in things like binutils and git and vim)

Yeah, the caller has the _() anyway for the format string, even in the
current version of the patch.  No idea how that leads to inserting the
translations passed in through %s, but I'll just stick to what we do
for other such cases in xfsprogs (e.g. the .oneline field in xfs_io).

Re: [PATCH 2/4] repair: add canonical names for the XR_INO_ constants

[Darrick J. Wong]

On Fri, Dec 05, 2025 at 09:13:37AM +0100, Christoph Hellwig wrote:
> On Thu, Dec 04, 2025 at 09:18:11AM -0800, Darrick J. Wong wrote:
> > > And that's what I'd expect.  But there are no direct gettext calls
> > > anywhere in xfsprogs, despite quite a bit of N_() usage.  What am I
> > > missing?
> > 
> > #define _(x)		gettext(x)
> > 
> > in platform_defs.h.
> > 
> > (and no, I don't get anything meaningful out of "N_()" for magic
> > tagging and "_()" to mean lookup, but according to Debian codesearch
> > it's a common practice in things like binutils and git and vim)
> 
> Yeah, the caller has the _() anyway for the format string, even in the
> current version of the patch.  No idea how that leads to inserting the
> translations passed in through %s, but I'll just stick to what we do
> for other such cases in xfsprogs (e.g. the .oneline field in xfs_io).

<nod> It took me a while to wrap my head around what's really happening
with gettext.  Would an example help?  Start with:

static const char foostrings[] = {
	[0]	= N_("foo"),
	[1]	= N_("bar"),
};

int main(int argc, char *argv[])
{
	int i = argc > 2 ? 1 : 0;

	printf("%s\n", _("Hello world!"));
	printf("%s\n", _(foostrings[i]));
}

First, make runs the gettext catalog generator, which sees two N_()
expression with a constant string inside of it ("foo" and "bar"), and
one _() expression with a constant string inside of it ("Hello world!").
These three strings are added to the catalog.  It ignores the _()
expression with a variable reference in it because that's not a string.

gcc then runs the source through the preprocessor:

static const char foostrings[] = {
	[0]	= "foo",
	[1]	= "bar",
};

int main(int argc, char *argv[])
{
	int i = argc > 2 ? 1 : 0;

	printf("%s\n", gettext("Hello world!"));
	printf("%s\n", gettext(foostrings[i]));
}

So foostrings is an array of pointers to const strings, as expected.
The actual message catalogue lookups happen in the printfs at the end of
main.

(Also note that we're not supposed to put format specifiers in message
catalog strings because malicious translation files can screw over the
program by replacing %d with %s, etc.  But most programs are super
guilty of violating that, xfsprogs included...)

--D

[PATCH 3/4] repair: factor out a process_dinode_metafile helper

[Christoph Hellwig]

Split the metafile logic from process_dinode_int into a separate
helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 repair/dinode.c | 87 ++++++++++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 40 deletions(-)

diff --git a/repair/dinode.c b/repair/dinode.c
index fd40fdcce665..f77c8e86c6f1 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2948,6 +2948,52 @@ _("Bad CoW extent size hint %u on inode %" PRIu64 ", "),
 	}
 }
 
+/*
+ * We always rebuild the metadata directory tree during phase 6, so we mark all
+ * directory blocks and other metadata files whose contents we don't want to
+ * save to be zapped.
+ *
+ * Currently, there are no metadata files that use xattrs, so we always drop the
+ * xattr blocks of metadata files.  Parent pointers will be rebuilt during
+ * phase 6.
+ */
+static bool
+process_dinode_metafile(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		agno,
+	xfs_agino_t		agino,
+	xfs_ino_t		lino,
+	enum xr_ino_type	type)
+{
+	struct ino_tree_node	*irec = find_inode_rec(mp, agno, agino);
+	int			off = get_inode_offset(mp, lino, irec);
+
+	set_inode_is_meta(irec, off);
+
+	switch (type) {
+	case XR_INO_RTBITMAP:
+	case XR_INO_RTSUM:
+		/*
+		 * RT bitmap and summary files are always recreated when
+		 * rtgroups are enabled.  For older filesystems, they exist at
+		 * fixed locations and cannot be zapped.
+		 */
+		if (xfs_has_rtgroups(mp))
+			return true;
+		return false;
+	case XR_INO_UQUOTA:
+	case XR_INO_GQUOTA:
+	case XR_INO_PQUOTA:
+		/*
+		 * Quota checking and repair doesn't happen until phase7, so
+		 * preserve quota inodes and their contents for later.
+		 */
+		return false;
+	default:
+		return true;
+	}
+}
+
 /*
  * returns 0 if the inode is ok, 1 if the inode is corrupt
  * check_dups can be set to 1 *only* when called by the
@@ -3563,48 +3609,9 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 	/* Does this inode think it was metadata? */
 	if (dino->di_version >= 3 &&
 	    (dino->di_flags2 & cpu_to_be64(XFS_DIFLAG2_METADATA))) {
-		struct ino_tree_node	*irec;
-		int			off;
-
-		irec = find_inode_rec(mp, agno, ino);
-		off = get_inode_offset(mp, lino, irec);
-		set_inode_is_meta(irec, off);
 		is_meta = true;
-
-		/*
-		 * We always rebuild the metadata directory tree during phase
-		 * 6, so we use this flag to get all the directory blocks
-		 * marked as free, and any other metadata files whose contents
-		 * we don't want to save.
-		 *
-		 * Currently, there are no metadata files that use xattrs, so
-		 * we always drop the xattr blocks of metadata files.  Parent
-		 * pointers will be rebuilt during phase 6.
-		 */
-		switch (type) {
-		case XR_INO_RTBITMAP:
-		case XR_INO_RTSUM:
-			/*
-			 * rt bitmap and summary files are always recreated
-			 * when rtgroups are enabled.  For older filesystems,
-			 * they exist at fixed locations and cannot be zapped.
-			 */
-			if (xfs_has_rtgroups(mp))
-				zap_metadata = true;
-			break;
-		case XR_INO_UQUOTA:
-		case XR_INO_GQUOTA:
-		case XR_INO_PQUOTA:
-			/*
-			 * Quota checking and repair doesn't happen until
-			 * phase7, so preserve quota inodes and their contents
-			 * for later.
-			 */
-			break;
-		default:
+		if (process_dinode_metafile(mp, agno, ino, lino, type))
 			zap_metadata = true;
-			break;
-		}
 	}
 
 	/*
-- 
2.47.3

Re: [PATCH 3/4] repair: factor out a process_dinode_metafile helper

[Carlos Maiolino]

On Fri, Nov 28, 2025 at 07:37:01AM +0100, Christoph Hellwig wrote:
> Split the metafile logic from process_dinode_int into a separate
> helper.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  repair/dinode.c | 87 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 47 insertions(+), 40 deletions(-)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index fd40fdcce665..f77c8e86c6f1 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2948,6 +2948,52 @@ _("Bad CoW extent size hint %u on inode %" PRIu64 ", "),
>  	}
>  }
> 

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


> +/*
> + * We always rebuild the metadata directory tree during phase 6, so we mark all
> + * directory blocks and other metadata files whose contents we don't want to
> + * save to be zapped.
> + *
> + * Currently, there are no metadata files that use xattrs, so we always drop the
> + * xattr blocks of metadata files.  Parent pointers will be rebuilt during
> + * phase 6.
> + */
> +static bool
> +process_dinode_metafile(
> +	struct xfs_mount	*mp,
> +	xfs_agnumber_t		agno,
> +	xfs_agino_t		agino,
> +	xfs_ino_t		lino,
> +	enum xr_ino_type	type)
> +{
> +	struct ino_tree_node	*irec = find_inode_rec(mp, agno, agino);
> +	int			off = get_inode_offset(mp, lino, irec);
> +
> +	set_inode_is_meta(irec, off);
> +
> +	switch (type) {
> +	case XR_INO_RTBITMAP:
> +	case XR_INO_RTSUM:
> +		/*
> +		 * RT bitmap and summary files are always recreated when
> +		 * rtgroups are enabled.  For older filesystems, they exist at
> +		 * fixed locations and cannot be zapped.
> +		 */
> +		if (xfs_has_rtgroups(mp))
> +			return true;
> +		return false;
> +	case XR_INO_UQUOTA:
> +	case XR_INO_GQUOTA:
> +	case XR_INO_PQUOTA:
> +		/*
> +		 * Quota checking and repair doesn't happen until phase7, so
> +		 * preserve quota inodes and their contents for later.
> +		 */
> +		return false;
> +	default:
> +		return true;
> +	}
> +}
> +
>  /*
>   * returns 0 if the inode is ok, 1 if the inode is corrupt
>   * check_dups can be set to 1 *only* when called by the
> @@ -3563,48 +3609,9 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  	/* Does this inode think it was metadata? */
>  	if (dino->di_version >= 3 &&
>  	    (dino->di_flags2 & cpu_to_be64(XFS_DIFLAG2_METADATA))) {
> -		struct ino_tree_node	*irec;
> -		int			off;
> -
> -		irec = find_inode_rec(mp, agno, ino);
> -		off = get_inode_offset(mp, lino, irec);
> -		set_inode_is_meta(irec, off);
>  		is_meta = true;
> -
> -		/*
> -		 * We always rebuild the metadata directory tree during phase
> -		 * 6, so we use this flag to get all the directory blocks
> -		 * marked as free, and any other metadata files whose contents
> -		 * we don't want to save.
> -		 *
> -		 * Currently, there are no metadata files that use xattrs, so
> -		 * we always drop the xattr blocks of metadata files.  Parent
> -		 * pointers will be rebuilt during phase 6.
> -		 */
> -		switch (type) {
> -		case XR_INO_RTBITMAP:
> -		case XR_INO_RTSUM:
> -			/*
> -			 * rt bitmap and summary files are always recreated
> -			 * when rtgroups are enabled.  For older filesystems,
> -			 * they exist at fixed locations and cannot be zapped.
> -			 */
> -			if (xfs_has_rtgroups(mp))
> -				zap_metadata = true;
> -			break;
> -		case XR_INO_UQUOTA:
> -		case XR_INO_GQUOTA:
> -		case XR_INO_PQUOTA:
> -			/*
> -			 * Quota checking and repair doesn't happen until
> -			 * phase7, so preserve quota inodes and their contents
> -			 * for later.
> -			 */
> -			break;
> -		default:
> +		if (process_dinode_metafile(mp, agno, ino, lino, type))
>  			zap_metadata = true;
> -			break;
> -		}
>  	}
> 
>  	/*
> --
> 2.47.3
> 
> 

Re: [PATCH 3/4] repair: factor out a process_dinode_metafile helper

[Darrick J. Wong]

On Fri, Nov 28, 2025 at 07:37:01AM +0100, Christoph Hellwig wrote:
> Split the metafile logic from process_dinode_int into a separate
> helper.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  repair/dinode.c | 87 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 47 insertions(+), 40 deletions(-)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index fd40fdcce665..f77c8e86c6f1 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2948,6 +2948,52 @@ _("Bad CoW extent size hint %u on inode %" PRIu64 ", "),
>  	}
>  }
>  
> +/*
> + * We always rebuild the metadata directory tree during phase 6, so we mark all
> + * directory blocks and other metadata files whose contents we don't want to
> + * save to be zapped.
> + *
> + * Currently, there are no metadata files that use xattrs, so we always drop the
> + * xattr blocks of metadata files.  Parent pointers will be rebuilt during
> + * phase 6.
> + */
> +static bool
> +process_dinode_metafile(
> +	struct xfs_mount	*mp,
> +	xfs_agnumber_t		agno,
> +	xfs_agino_t		agino,
> +	xfs_ino_t		lino,
> +	enum xr_ino_type	type)
> +{
> +	struct ino_tree_node	*irec = find_inode_rec(mp, agno, agino);
> +	int			off = get_inode_offset(mp, lino, irec);
> +
> +	set_inode_is_meta(irec, off);
> +
> +	switch (type) {
> +	case XR_INO_RTBITMAP:
> +	case XR_INO_RTSUM:
> +		/*
> +		 * RT bitmap and summary files are always recreated when
> +		 * rtgroups are enabled.  For older filesystems, they exist at
> +		 * fixed locations and cannot be zapped.
> +		 */
> +		if (xfs_has_rtgroups(mp))
> +			return true;
> +		return false;
> +	case XR_INO_UQUOTA:
> +	case XR_INO_GQUOTA:
> +	case XR_INO_PQUOTA:
> +		/*
> +		 * Quota checking and repair doesn't happen until phase7, so
> +		 * preserve quota inodes and their contents for later.
> +		 */
> +		return false;
> +	default:
> +		return true;
> +	}
> +}
> +
>  /*
>   * returns 0 if the inode is ok, 1 if the inode is corrupt
>   * check_dups can be set to 1 *only* when called by the
> @@ -3563,48 +3609,9 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  	/* Does this inode think it was metadata? */
>  	if (dino->di_version >= 3 &&
>  	    (dino->di_flags2 & cpu_to_be64(XFS_DIFLAG2_METADATA))) {
> -		struct ino_tree_node	*irec;
> -		int			off;
> -
> -		irec = find_inode_rec(mp, agno, ino);
> -		off = get_inode_offset(mp, lino, irec);
> -		set_inode_is_meta(irec, off);
>  		is_meta = true;
> -
> -		/*
> -		 * We always rebuild the metadata directory tree during phase
> -		 * 6, so we use this flag to get all the directory blocks
> -		 * marked as free, and any other metadata files whose contents
> -		 * we don't want to save.
> -		 *
> -		 * Currently, there are no metadata files that use xattrs, so
> -		 * we always drop the xattr blocks of metadata files.  Parent
> -		 * pointers will be rebuilt during phase 6.
> -		 */
> -		switch (type) {
> -		case XR_INO_RTBITMAP:
> -		case XR_INO_RTSUM:
> -			/*
> -			 * rt bitmap and summary files are always recreated
> -			 * when rtgroups are enabled.  For older filesystems,
> -			 * they exist at fixed locations and cannot be zapped.
> -			 */
> -			if (xfs_has_rtgroups(mp))
> -				zap_metadata = true;
> -			break;
> -		case XR_INO_UQUOTA:
> -		case XR_INO_GQUOTA:
> -		case XR_INO_PQUOTA:
> -			/*
> -			 * Quota checking and repair doesn't happen until
> -			 * phase7, so preserve quota inodes and their contents
> -			 * for later.
> -			 */
> -			break;
> -		default:
> +		if (process_dinode_metafile(mp, agno, ino, lino, type))
>  			zap_metadata = true;
> -			break;
> -		}
>  	}
>  
>  	/*
> -- 
> 2.47.3
> 
> 

[PATCH 4/4] repair: enhance process_dinode_metafile

[Christoph Hellwig]

Explicitly list the destiny of each metafile inode type, and warn about
unexpected types instead of just silently zapping them.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 repair/dinode.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/repair/dinode.c b/repair/dinode.c
index f77c8e86c6f1..695ce0410395 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2963,7 +2963,8 @@ process_dinode_metafile(
 	xfs_agnumber_t		agno,
 	xfs_agino_t		agino,
 	xfs_ino_t		lino,
-	enum xr_ino_type	type)
+	enum xr_ino_type	type,
+	bool			*zap_metadata)
 {
 	struct ino_tree_node	*irec = find_inode_rec(mp, agno, agino);
 	int			off = get_inode_offset(mp, lino, irec);
@@ -2971,16 +2972,6 @@ process_dinode_metafile(
 	set_inode_is_meta(irec, off);
 
 	switch (type) {
-	case XR_INO_RTBITMAP:
-	case XR_INO_RTSUM:
-		/*
-		 * RT bitmap and summary files are always recreated when
-		 * rtgroups are enabled.  For older filesystems, they exist at
-		 * fixed locations and cannot be zapped.
-		 */
-		if (xfs_has_rtgroups(mp))
-			return true;
-		return false;
 	case XR_INO_UQUOTA:
 	case XR_INO_GQUOTA:
 	case XR_INO_PQUOTA:
@@ -2989,7 +2980,25 @@ process_dinode_metafile(
 		 * preserve quota inodes and their contents for later.
 		 */
 		return false;
+	case XR_INO_DIR:
+	case XR_INO_RTBITMAP:
+	case XR_INO_RTSUM:
+	case XR_INO_RTRMAP:
+	case XR_INO_RTREFC:
+		/*
+		 * These are always recreated.  Note that for pre-metadir file
+		 * systems, the RT bitmap and summary inodes need to be
+		 * preserved, but we'll never end up here for them.
+		 */
+		*zap_metadata = true;
+		return false;
 	default:
+		do_warn(_("unexpected %s inode %" PRIu64 " with metadata flag"),
+				xr_ino_type_name[type], lino);
+		if (!no_modify)
+			do_warn(_(" will zap\n"));
+		else
+			do_warn(_(" would zap\n"));
 		return true;
 	}
 }
@@ -3610,8 +3619,9 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 	if (dino->di_version >= 3 &&
 	    (dino->di_flags2 & cpu_to_be64(XFS_DIFLAG2_METADATA))) {
 		is_meta = true;
-		if (process_dinode_metafile(mp, agno, ino, lino, type))
-			zap_metadata = true;
+		if (process_dinode_metafile(mp, agno, ino, lino, type,
+				&zap_metadata))
+			goto bad_out;
 	}
 
 	/*
-- 
2.47.3

Re: [PATCH 4/4] repair: enhance process_dinode_metafile

[Carlos Maiolino]

On Fri, Nov 28, 2025 at 07:37:02AM +0100, Christoph Hellwig wrote:
> Explicitly list the destiny of each metafile inode type, and warn about
> unexpected types instead of just silently zapping them.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  repair/dinode.c | 36 +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index f77c8e86c6f1..695ce0410395 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2963,7 +2963,8 @@ process_dinode_metafile(
>  	xfs_agnumber_t		agno,
>  	xfs_agino_t		agino,
>  	xfs_ino_t		lino,
> -	enum xr_ino_type	type)
> +	enum xr_ino_type	type,
> +	bool			*zap_metadata)
>  {
>  	struct ino_tree_node	*irec = find_inode_rec(mp, agno, agino);
>  	int			off = get_inode_offset(mp, lino, irec);
> @@ -2971,16 +2972,6 @@ process_dinode_metafile(
>  	set_inode_is_meta(irec, off);
> 
>  	switch (type) {
> -	case XR_INO_RTBITMAP:
> -	case XR_INO_RTSUM:
> -		/*
> -		 * RT bitmap and summary files are always recreated when
> -		 * rtgroups are enabled.  For older filesystems, they exist at
> -		 * fixed locations and cannot be zapped.
> -		 */
> -		if (xfs_has_rtgroups(mp))
> -			return true;
> -		return false;
>  	case XR_INO_UQUOTA:
>  	case XR_INO_GQUOTA:
>  	case XR_INO_PQUOTA:
> @@ -2989,7 +2980,25 @@ process_dinode_metafile(
>  		 * preserve quota inodes and their contents for later.
>  		 */
>  		return false;
> +	case XR_INO_DIR:
> +	case XR_INO_RTBITMAP:
> +	case XR_INO_RTSUM:
> +	case XR_INO_RTRMAP:
> +	case XR_INO_RTREFC:
> +		/*
> +		 * These are always recreated.  Note that for pre-metadir file
> +		 * systems, the RT bitmap and summary inodes need to be
> +		 * preserved, but we'll never end up here for them.

		Perhaps worth adding an assertion to catch a pre-metadir
		reaching here for whatever reason and zapping RT
		bitmap and summary?


Other than that, LGTM

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


> +		 */
> +		*zap_metadata = true;
> +		return false;
>  	default:
> +		do_warn(_("unexpected %s inode %" PRIu64 " with metadata flag"),
> +				xr_ino_type_name[type], lino);
> +		if (!no_modify)
> +			do_warn(_(" will zap\n"));
> +		else
> +			do_warn(_(" would zap\n"));
>  		return true;
>  	}
>  }
> @@ -3610,8 +3619,9 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  	if (dino->di_version >= 3 &&
>  	    (dino->di_flags2 & cpu_to_be64(XFS_DIFLAG2_METADATA))) {
>  		is_meta = true;
> -		if (process_dinode_metafile(mp, agno, ino, lino, type))
> -			zap_metadata = true;
> +		if (process_dinode_metafile(mp, agno, ino, lino, type,
> +				&zap_metadata))
> +			goto bad_out;


> -}
>  	}
> 
>  	/*
> --
> 2.47.3
> 
> 

Re: [PATCH 4/4] repair: enhance process_dinode_metafile

[Christoph Hellwig]

On Fri, Nov 28, 2025 at 09:15:34AM +0100, Carlos Maiolino wrote:
> 		Perhaps worth adding an assertion to catch a pre-metadir
> 		reaching here for whatever reason and zapping RT
> 		bitmap and summary?

inodes with the metafile flag on non-metadir file systems are caught
by the inode buffer verifier.

Re: [PATCH 4/4] repair: enhance process_dinode_metafile

[Carlos Maiolino]

On Mon, Dec 01, 2025 at 07:23:49AM +0100, Christoph Hellwig wrote:
> On Fri, Nov 28, 2025 at 09:15:34AM +0100, Carlos Maiolino wrote:
> > 		Perhaps worth adding an assertion to catch a pre-metadir
> > 		reaching here for whatever reason and zapping RT
> > 		bitmap and summary?
> 
> inodes with the metafile flag on non-metadir file systems are caught
> by the inode buffer verifier.

I see, thanks for the info!

Re: [PATCH 4/4] repair: enhance process_dinode_metafile

[Darrick J. Wong]

On Fri, Nov 28, 2025 at 07:37:02AM +0100, Christoph Hellwig wrote:
> Explicitly list the destiny of each metafile inode type, and warn about
> unexpected types instead of just silently zapping them.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  repair/dinode.c | 36 +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index f77c8e86c6f1..695ce0410395 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2963,7 +2963,8 @@ process_dinode_metafile(
>  	xfs_agnumber_t		agno,
>  	xfs_agino_t		agino,
>  	xfs_ino_t		lino,
> -	enum xr_ino_type	type)
> +	enum xr_ino_type	type,
> +	bool			*zap_metadata)
>  {
>  	struct ino_tree_node	*irec = find_inode_rec(mp, agno, agino);
>  	int			off = get_inode_offset(mp, lino, irec);
> @@ -2971,16 +2972,6 @@ process_dinode_metafile(
>  	set_inode_is_meta(irec, off);
>  
>  	switch (type) {
> -	case XR_INO_RTBITMAP:
> -	case XR_INO_RTSUM:
> -		/*
> -		 * RT bitmap and summary files are always recreated when
> -		 * rtgroups are enabled.  For older filesystems, they exist at
> -		 * fixed locations and cannot be zapped.
> -		 */
> -		if (xfs_has_rtgroups(mp))
> -			return true;
> -		return false;
>  	case XR_INO_UQUOTA:
>  	case XR_INO_GQUOTA:
>  	case XR_INO_PQUOTA:
> @@ -2989,7 +2980,25 @@ process_dinode_metafile(
>  		 * preserve quota inodes and their contents for later.
>  		 */
>  		return false;
> +	case XR_INO_DIR:
> +	case XR_INO_RTBITMAP:
> +	case XR_INO_RTSUM:
> +	case XR_INO_RTRMAP:
> +	case XR_INO_RTREFC:
> +		/*
> +		 * These are always recreated.  Note that for pre-metadir file
> +		 * systems, the RT bitmap and summary inodes need to be
> +		 * preserved, but we'll never end up here for them.
> +		 */
> +		*zap_metadata = true;

Heh.  I remember agonizing over this a looooong time ago.

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> +		return false;
>  	default:
> +		do_warn(_("unexpected %s inode %" PRIu64 " with metadata flag"),
> +				xr_ino_type_name[type], lino);
> +		if (!no_modify)
> +			do_warn(_(" will zap\n"));
> +		else
> +			do_warn(_(" would zap\n"));
>  		return true;
>  	}
>  }
> @@ -3610,8 +3619,9 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  	if (dino->di_version >= 3 &&
>  	    (dino->di_flags2 & cpu_to_be64(XFS_DIFLAG2_METADATA))) {
>  		is_meta = true;
> -		if (process_dinode_metafile(mp, agno, ino, lino, type))
> -			zap_metadata = true;
> +		if (process_dinode_metafile(mp, agno, ino, lino, type,
> +				&zap_metadata))
> +			goto bad_out;
>  	}
>  
>  	/*
> -- 
> 2.47.3
> 
> 

repair tidyups for metadir handling v2

[Christoph Hellwig]

Hi all,

I recently played with adding another metafile type, and of course
messed up my first attempt.  This series enhances the metadir
repair code a bit based on things I found while debugging that,
and in the last patch changes repair to warn about unexpected
metafile types.

Changes since v1:
 - add N_() annotations for gettext
 - add a build-time size assert for the inode type names
 - fix a comment typo

[PATCH 3/4] repair: factor out a process_dinode_metafile helper

[Christoph Hellwig]

Split the metafile logic from process_dinode_int into a separate
helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 repair/dinode.c | 87 ++++++++++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 40 deletions(-)

diff --git a/repair/dinode.c b/repair/dinode.c
index bf2739a6eae5..61ccbbc25ca9 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2950,6 +2950,52 @@ _("Bad CoW extent size hint %u on inode %" PRIu64 ", "),
 	}
 }
 
+/*
+ * We always rebuild the metadata directory tree during phase 6, so we mark all
+ * directory blocks and other metadata files whose contents we don't want to
+ * save to be zapped.
+ *
+ * Currently, there are no metadata files that use xattrs, so we always drop the
+ * xattr blocks of metadata files.  Parent pointers will be rebuilt during
+ * phase 6.
+ */
+static bool
+process_dinode_metafile(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		agno,
+	xfs_agino_t		agino,
+	xfs_ino_t		lino,
+	enum xr_ino_type	type)
+{
+	struct ino_tree_node	*irec = find_inode_rec(mp, agno, agino);
+	int			off = get_inode_offset(mp, lino, irec);
+
+	set_inode_is_meta(irec, off);
+
+	switch (type) {
+	case XR_INO_RTBITMAP:
+	case XR_INO_RTSUM:
+		/*
+		 * RT bitmap and summary files are always recreated when
+		 * rtgroups are enabled.  For older filesystems, they exist at
+		 * fixed locations and cannot be zapped.
+		 */
+		if (xfs_has_rtgroups(mp))
+			return true;
+		return false;
+	case XR_INO_UQUOTA:
+	case XR_INO_GQUOTA:
+	case XR_INO_PQUOTA:
+		/*
+		 * Quota checking and repair doesn't happen until phase7, so
+		 * preserve quota inodes and their contents for later.
+		 */
+		return false;
+	default:
+		return true;
+	}
+}
+
 /*
  * returns 0 if the inode is ok, 1 if the inode is corrupt
  * check_dups can be set to 1 *only* when called by the
@@ -3565,48 +3611,9 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 	/* Does this inode think it was metadata? */
 	if (dino->di_version >= 3 &&
 	    (dino->di_flags2 & cpu_to_be64(XFS_DIFLAG2_METADATA))) {
-		struct ino_tree_node	*irec;
-		int			off;
-
-		irec = find_inode_rec(mp, agno, ino);
-		off = get_inode_offset(mp, lino, irec);
-		set_inode_is_meta(irec, off);
 		is_meta = true;
-
-		/*
-		 * We always rebuild the metadata directory tree during phase
-		 * 6, so we use this flag to get all the directory blocks
-		 * marked as free, and any other metadata files whose contents
-		 * we don't want to save.
-		 *
-		 * Currently, there are no metadata files that use xattrs, so
-		 * we always drop the xattr blocks of metadata files.  Parent
-		 * pointers will be rebuilt during phase 6.
-		 */
-		switch (type) {
-		case XR_INO_RTBITMAP:
-		case XR_INO_RTSUM:
-			/*
-			 * rt bitmap and summary files are always recreated
-			 * when rtgroups are enabled.  For older filesystems,
-			 * they exist at fixed locations and cannot be zapped.
-			 */
-			if (xfs_has_rtgroups(mp))
-				zap_metadata = true;
-			break;
-		case XR_INO_UQUOTA:
-		case XR_INO_GQUOTA:
-		case XR_INO_PQUOTA:
-			/*
-			 * Quota checking and repair doesn't happen until
-			 * phase7, so preserve quota inodes and their contents
-			 * for later.
-			 */
-			break;
-		default:
+		if (process_dinode_metafile(mp, agno, ino, lino, type))
 			zap_metadata = true;
-			break;
-		}
 	}
 
 	/*
-- 
2.47.3

repair tidyups for metadir handling v3

[Christoph Hellwig]

Hi all,

I recently played with adding another metafile type, and of course
messed up my first attempt.  This series enhances the metadir
repair code a bit based on things I found while debugging that,
and in the last patch changes repair to warn about unexpected
metafile types.

Changes since v2:
 - more gettext!

Changes since v1:
 - add N_() annotations for gettext
 - add a build-time size assert for the inode type names
 - fix a comment typo

[PATCH 3/4] repair: factor out a process_dinode_metafile helper

[Christoph Hellwig]

Split the metafile logic from process_dinode_int into a separate
helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 repair/dinode.c | 87 ++++++++++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 40 deletions(-)

diff --git a/repair/dinode.c b/repair/dinode.c
index 7f987c38d2e4..2e8728594ead 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2950,6 +2950,52 @@ _("Bad CoW extent size hint %u on inode %" PRIu64 ", "),
 	}
 }
 
+/*
+ * We always rebuild the metadata directory tree during phase 6, so we mark all
+ * directory blocks and other metadata files whose contents we don't want to
+ * save to be zapped.
+ *
+ * Currently, there are no metadata files that use xattrs, so we always drop the
+ * xattr blocks of metadata files.  Parent pointers will be rebuilt during
+ * phase 6.
+ */
+static bool
+process_dinode_metafile(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		agno,
+	xfs_agino_t		agino,
+	xfs_ino_t		lino,
+	enum xr_ino_type	type)
+{
+	struct ino_tree_node	*irec = find_inode_rec(mp, agno, agino);
+	int			off = get_inode_offset(mp, lino, irec);
+
+	set_inode_is_meta(irec, off);
+
+	switch (type) {
+	case XR_INO_RTBITMAP:
+	case XR_INO_RTSUM:
+		/*
+		 * RT bitmap and summary files are always recreated when
+		 * rtgroups are enabled.  For older filesystems, they exist at
+		 * fixed locations and cannot be zapped.
+		 */
+		if (xfs_has_rtgroups(mp))
+			return true;
+		return false;
+	case XR_INO_UQUOTA:
+	case XR_INO_GQUOTA:
+	case XR_INO_PQUOTA:
+		/*
+		 * Quota checking and repair doesn't happen until phase7, so
+		 * preserve quota inodes and their contents for later.
+		 */
+		return false;
+	default:
+		return true;
+	}
+}
+
 /*
  * returns 0 if the inode is ok, 1 if the inode is corrupt
  * check_dups can be set to 1 *only* when called by the
@@ -3565,48 +3611,9 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 	/* Does this inode think it was metadata? */
 	if (dino->di_version >= 3 &&
 	    (dino->di_flags2 & cpu_to_be64(XFS_DIFLAG2_METADATA))) {
-		struct ino_tree_node	*irec;
-		int			off;
-
-		irec = find_inode_rec(mp, agno, ino);
-		off = get_inode_offset(mp, lino, irec);
-		set_inode_is_meta(irec, off);
 		is_meta = true;
-
-		/*
-		 * We always rebuild the metadata directory tree during phase
-		 * 6, so we use this flag to get all the directory blocks
-		 * marked as free, and any other metadata files whose contents
-		 * we don't want to save.
-		 *
-		 * Currently, there are no metadata files that use xattrs, so
-		 * we always drop the xattr blocks of metadata files.  Parent
-		 * pointers will be rebuilt during phase 6.
-		 */
-		switch (type) {
-		case XR_INO_RTBITMAP:
-		case XR_INO_RTSUM:
-			/*
-			 * rt bitmap and summary files are always recreated
-			 * when rtgroups are enabled.  For older filesystems,
-			 * they exist at fixed locations and cannot be zapped.
-			 */
-			if (xfs_has_rtgroups(mp))
-				zap_metadata = true;
-			break;
-		case XR_INO_UQUOTA:
-		case XR_INO_GQUOTA:
-		case XR_INO_PQUOTA:
-			/*
-			 * Quota checking and repair doesn't happen until
-			 * phase7, so preserve quota inodes and their contents
-			 * for later.
-			 */
-			break;
-		default:
+		if (process_dinode_metafile(mp, agno, ino, lino, type))
 			zap_metadata = true;
-			break;
-		}
 	}
 
 	/*
-- 
2.47.3