[PATCH 0/3] xfs: enums don't work with __print_symbolic

[PATCH 0/3] xfs: enums don't work with __print_symbolic

[Dave Chinner]

Hi folks,

When trying to read traces from fsx failurs, I kept coming across
fields in the traces that had no output what-so-ever. The common
theme was that they all were tables that were parsed by
__print_symbolic() and the value definitions were enums.

Unfortunately, __print_symbolic() does some crazy stuff involving
stringification of the table that is passed to it, which means
the enums are turned into strings and so their never get treated as
enums after pre-processing. The result is a format string that looks
like:

# cat /sys/kernel/debug/tracing/events/xfs/xfs_iomap_alloc/format
.....
print fmt: ..... __print_symbolic(REC->type, { XFS_IO_INVALID, "invalid" }, { XFS_IO_DELALLOC, "delalloc" }, { XFS_IO_UNWRITTEN, "unwritten" }, { XFS_IO_OVERWRITE, "overwrite" }, { XFS_IO_COW, "CoW" }, { XFS_IO_HOLE, "hole" }), ....
#

And, well, XFS_IO_OVERWRITE is a string, not an integer value of 3.
Hence __print_symbolic never prints out the correct value.

The way to fix this is to turn all the enums into defined macros,
that way the preprocessor still substitutes the value for the
stringified table as the it does string matches. The result is:

__print_symbolic(REC->type, { 0, "hole" }, { 1, "delalloc" }, { 2, "unwritten" }, { 3, "overwrite" }, { 4, "CoW" })

And the trace events now print the type correctly in the trace.

This series fixes the cases I noticed by removing the various enums
that end up in trace format tables.

Cheers,

Dave.

[PATCH 1/3] xfs: make iomap type tracing work

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

__print_symbolic does crazy things and stringifies the tables
that are used to map values to strings. Hence if we use enums
for the values, it just doesn't work because XFS_IO_COW != 3,
it's a string.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_aops.h | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_aops.h b/fs/xfs/xfs_aops.h
index 494b4338446e..810ad0362431 100644
--- a/fs/xfs/xfs_aops.h
+++ b/fs/xfs/xfs_aops.h
@@ -11,13 +11,11 @@ extern struct bio_set xfs_ioend_bioset;
 /*
  * Types of I/O for bmap clustering and I/O completion tracking.
  */
-enum {
-	XFS_IO_HOLE,		/* covers region without any block allocation */
-	XFS_IO_DELALLOC,	/* covers delalloc region */
-	XFS_IO_UNWRITTEN,	/* covers allocated but uninitialized data */
-	XFS_IO_OVERWRITE,	/* covers already allocated extent */
-	XFS_IO_COW,		/* covers copy-on-write extent */
-};
+#define XFS_IO_HOLE		0 /* covers region with no block allocation */
+#define XFS_IO_DELALLOC		1 /* covers delalloc region */
+#define XFS_IO_UNWRITTEN	2 /* covers allocated but uninitialized data */
+#define XFS_IO_OVERWRITE	3 /* covers already allocated extent */
+#define XFS_IO_COW		4 /* covers copy-on-write extent */
 
 #define XFS_IO_TYPES \
 	{ XFS_IO_HOLE,			"hole" },	\
-- 
2.19.1

[PATCH 2/3] xfs: remove xfs_lookup_t enum because tracing

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

__print_symbolic does crazy things and stringifies the tables
that are used to map values to strings. Hence if we use enums
for the values, it just doesn't work because XFS_LOOKUP_EQi != 0,
it's a string.

Convert to macros and remove the xfs_lookup_t enum as it's not
useful anymore.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_btree.c  | 2 +-
 fs/xfs/libxfs/xfs_btree.h  | 8 ++++----
 fs/xfs/libxfs/xfs_ialloc.c | 2 +-
 fs/xfs/libxfs/xfs_ialloc.h | 2 +-
 fs/xfs/libxfs/xfs_types.h  | 4 ----
 fs/xfs/xfs_trace.h         | 6 +++---
 6 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 34c6d7bd4d18..85b70893781c 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -1864,7 +1864,7 @@ xfs_lookup_get_search_key(
 int					/* error */
 xfs_btree_lookup(
 	struct xfs_btree_cur	*cur,	/* btree cursor */
-	xfs_lookup_t		dir,	/* <=, ==, or >= */
+	unsigned		dir,	/* <=, ==, or >= */
 	int			*stat)	/* success/failure */
 {
 	struct xfs_btree_block	*block;	/* current btree block */
diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
index e3b3e9dce5da..802088f48ef4 100644
--- a/fs/xfs/libxfs/xfs_btree.h
+++ b/fs/xfs/libxfs/xfs_btree.h
@@ -51,9 +51,9 @@ union xfs_btree_rec {
 /*
  * This nonsense is to make -wlint happy.
  */
-#define	XFS_LOOKUP_EQ	((xfs_lookup_t)XFS_LOOKUP_EQi)
-#define	XFS_LOOKUP_LE	((xfs_lookup_t)XFS_LOOKUP_LEi)
-#define	XFS_LOOKUP_GE	((xfs_lookup_t)XFS_LOOKUP_GEi)
+#define	XFS_LOOKUP_EQ	0
+#define	XFS_LOOKUP_LE	1
+#define	XFS_LOOKUP_GE	2
 
 #define	XFS_BTNUM_BNO	((xfs_btnum_t)XFS_BTNUM_BNOi)
 #define	XFS_BTNUM_CNT	((xfs_btnum_t)XFS_BTNUM_CNTi)
@@ -402,7 +402,7 @@ xfs_btree_init_block_int(
  */
 int xfs_btree_increment(struct xfs_btree_cur *, int, int *);
 int xfs_btree_decrement(struct xfs_btree_cur *, int, int *);
-int xfs_btree_lookup(struct xfs_btree_cur *, xfs_lookup_t, int *);
+int xfs_btree_lookup(struct xfs_btree_cur *, unsigned, int *);
 int xfs_btree_update(struct xfs_btree_cur *, union xfs_btree_rec *);
 int xfs_btree_new_iroot(struct xfs_btree_cur *, int *, int *);
 int xfs_btree_insert(struct xfs_btree_cur *, int *);
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index a8f6db735d5d..5555fb64b7bf 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -52,7 +52,7 @@ int					/* error */
 xfs_inobt_lookup(
 	struct xfs_btree_cur	*cur,	/* btree cursor */
 	xfs_agino_t		ino,	/* starting inode of chunk */
-	xfs_lookup_t		dir,	/* <=, >=, == */
+	unsigned		dir,	/* <=, >=, == */
 	int			*stat)	/* success/failure */
 {
 	cur->bc_rec.i.ir_startino = ino;
diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
index e936b7cc9389..f0557038297f 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -135,7 +135,7 @@ xfs_ialloc_pagi_init(
  * Lookup a record by ino in the btree given by cur.
  */
 int xfs_inobt_lookup(struct xfs_btree_cur *cur, xfs_agino_t ino,
-		xfs_lookup_t dir, int *stat);
+		unsigned dir, int *stat);
 
 /*
  * Get the data from the pointed-to record.
diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
index b9e6c89284c3..a73cd80c2439 100644
--- a/fs/xfs/libxfs/xfs_types.h
+++ b/fs/xfs/libxfs/xfs_types.h
@@ -100,10 +100,6 @@ typedef void *		xfs_failaddr_t;
  */
 #define MAXNAMELEN	256
 
-typedef enum {
-	XFS_LOOKUP_EQi, XFS_LOOKUP_LEi, XFS_LOOKUP_GEi
-} xfs_lookup_t;
-
 typedef enum {
 	XFS_BTNUM_BNOi, XFS_BTNUM_CNTi, XFS_BTNUM_RMAPi, XFS_BTNUM_BMAPi,
 	XFS_BTNUM_INOi, XFS_BTNUM_FINOi, XFS_BTNUM_REFCi, XFS_BTNUM_MAX
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 8a6532aae779..145412e91a70 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -2616,13 +2616,13 @@ DEFINE_AG_ERROR_EVENT(xfs_ag_resv_init_error);
 	{ XFS_LOOKUP_GE,	"ge" }
 DECLARE_EVENT_CLASS(xfs_ag_btree_lookup_class,
 	TP_PROTO(struct xfs_mount *mp, xfs_agnumber_t agno,
-		 xfs_agblock_t agbno, xfs_lookup_t dir),
+		 xfs_agblock_t agbno, unsigned dir),
 	TP_ARGS(mp, agno, agbno, dir),
 	TP_STRUCT__entry(
 		__field(dev_t, dev)
 		__field(xfs_agnumber_t, agno)
 		__field(xfs_agblock_t, agbno)
-		__field(xfs_lookup_t, dir)
+		__field(unsigned, dir)
 	),
 	TP_fast_assign(
 		__entry->dev = mp->m_super->s_dev;
@@ -2641,7 +2641,7 @@ DECLARE_EVENT_CLASS(xfs_ag_btree_lookup_class,
 #define DEFINE_AG_BTREE_LOOKUP_EVENT(name) \
 DEFINE_EVENT(xfs_ag_btree_lookup_class, name, \
 	TP_PROTO(struct xfs_mount *mp, xfs_agnumber_t agno, \
-		 xfs_agblock_t agbno, xfs_lookup_t dir), \
+		 xfs_agblock_t agbno, unsigned dir), \
 	TP_ARGS(mp, agno, agbno, dir))
 
 /* single-rcext tracepoint class */
-- 
2.19.1

[PATCH 3/3] xfs: remove xfs_extst_t enum because tracing

[Dave Chinner]

__print_symbolic does crazy things and stringifies the tables
that are used to map values to strings. Hence if we use enums
for the values, it just doesn't work because XFS_UNWRITTEN != 1,
it's a string.

Convert to macros and remove the xfs_extst_t enum as it's not
useful anymore.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c  |  2 +-
 fs/xfs/libxfs/xfs_bmap.h  |  2 +-
 fs/xfs/libxfs/xfs_rmap.c  |  2 +-
 fs/xfs/libxfs/xfs_rmap.h  |  2 +-
 fs/xfs/libxfs/xfs_types.h |  7 +++----
 fs/xfs/xfs_bmap_item.c    |  2 +-
 fs/xfs/xfs_rmap_item.c    |  2 +-
 fs/xfs/xfs_trace.h        | 12 ++++++------
 fs/xfs/xfs_trans.h        |  4 ++--
 fs/xfs/xfs_trans_bmap.c   |  4 ++--
 fs/xfs/xfs_trans_rmap.c   |  4 ++--
 11 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 19e921d1586f..39eaa2b86060 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -6062,7 +6062,7 @@ xfs_bmap_finish_one(
 	xfs_fileoff_t			startoff,
 	xfs_fsblock_t			startblock,
 	xfs_filblks_t			*blockcount,
-	xfs_exntst_t			state)
+	unsigned			state)
 {
 	int				error = 0;
 
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 488dc8860fd7..fd5c9916881d 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -255,7 +255,7 @@ struct xfs_bmap_intent {
 int	xfs_bmap_finish_one(struct xfs_trans *tp, struct xfs_inode *ip,
 		enum xfs_bmap_intent_type type, int whichfork,
 		xfs_fileoff_t startoff, xfs_fsblock_t startblock,
-		xfs_filblks_t *blockcount, xfs_exntst_t state);
+		xfs_filblks_t *blockcount, unsigned state);
 int	xfs_bmap_map_extent(struct xfs_trans *tp, struct xfs_inode *ip,
 		struct xfs_bmbt_irec *imap);
 int	xfs_bmap_unmap_extent(struct xfs_trans *tp, struct xfs_inode *ip,
diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
index 245af452840e..34dc7470c14f 100644
--- a/fs/xfs/libxfs/xfs_rmap.c
+++ b/fs/xfs/libxfs/xfs_rmap.c
@@ -2165,7 +2165,7 @@ xfs_rmap_finish_one(
 	xfs_fileoff_t			startoff,
 	xfs_fsblock_t			startblock,
 	xfs_filblks_t			blockcount,
-	xfs_exntst_t			state,
+	unsigned			state,
 	struct xfs_btree_cur		**pcur)
 {
 	struct xfs_mount		*mp = tp->t_mountp;
diff --git a/fs/xfs/libxfs/xfs_rmap.h b/fs/xfs/libxfs/xfs_rmap.h
index 157dc722ad35..5b72c288ef48 100644
--- a/fs/xfs/libxfs/xfs_rmap.h
+++ b/fs/xfs/libxfs/xfs_rmap.h
@@ -202,7 +202,7 @@ void xfs_rmap_finish_one_cleanup(struct xfs_trans *tp,
 int xfs_rmap_finish_one(struct xfs_trans *tp, enum xfs_rmap_intent_type type,
 		uint64_t owner, int whichfork, xfs_fileoff_t startoff,
 		xfs_fsblock_t startblock, xfs_filblks_t blockcount,
-		xfs_exntst_t state, struct xfs_btree_cur **pcur);
+		unsigned state, struct xfs_btree_cur **pcur);
 
 int xfs_rmap_find_left_neighbor(struct xfs_btree_cur *cur, xfs_agblock_t bno,
 		uint64_t owner, uint64_t offset, unsigned int flags,
diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
index a73cd80c2439..6c1e0cbbb6fb 100644
--- a/fs/xfs/libxfs/xfs_types.h
+++ b/fs/xfs/libxfs/xfs_types.h
@@ -131,16 +131,15 @@ struct xfs_iext_cursor {
 	int			pos;
 };
 
-typedef enum {
-	XFS_EXT_NORM, XFS_EXT_UNWRITTEN,
-} xfs_exntst_t;
+#define XFS_EXT_NORM		0
+#define XFS_EXT_UNWRITTEN	1
 
 typedef struct xfs_bmbt_irec
 {
 	xfs_fileoff_t	br_startoff;	/* starting file offset */
 	xfs_fsblock_t	br_startblock;	/* starting block number */
 	xfs_filblks_t	br_blockcount;	/* number of blocks */
-	xfs_exntst_t	br_state;	/* extent state */
+	unsigned	br_state;	/* extent state */
 } xfs_bmbt_irec_t;
 
 /*
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index ce45f066995e..124dc5d6e5a0 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -388,7 +388,7 @@ xfs_bui_recover(
 	struct xfs_bud_log_item		*budp;
 	enum xfs_bmap_intent_type	type;
 	int				whichfork;
-	xfs_exntst_t			state;
+	unsigned			state;
 	struct xfs_trans		*tp;
 	struct xfs_inode		*ip = NULL;
 	struct xfs_bmbt_irec		irec;
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 127dc9c32a54..6122c3a80be9 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -412,7 +412,7 @@ xfs_rui_recover(
 	struct xfs_rud_log_item		*rudp;
 	enum xfs_rmap_intent_type	type;
 	int				whichfork;
-	xfs_exntst_t			state;
+	unsigned			state;
 	struct xfs_trans		*tp;
 	struct xfs_btree_cur		*rcur = NULL;
 
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 145412e91a70..c11dfa709bc7 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -218,7 +218,7 @@ DECLARE_EVENT_CLASS(xfs_bmap_class,
 		__field(xfs_fileoff_t, startoff)
 		__field(xfs_fsblock_t, startblock)
 		__field(xfs_filblks_t, blockcount)
-		__field(xfs_exntst_t, state)
+		__field(unsigned, state)
 		__field(int, bmap_state)
 		__field(unsigned long, caller_ip)
 	),
@@ -240,7 +240,7 @@ DECLARE_EVENT_CLASS(xfs_bmap_class,
 		__entry->caller_ip = caller_ip;
 	),
 	TP_printk("dev %d:%d ino 0x%llx state %s cur %p/%d "
-		  "offset %lld block %lld count %lld flag %d caller %pS",
+		  "offset %lld block %lld count %lld flag %u caller %pS",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->ino,
 		  __print_flags(__entry->bmap_state, "|", XFS_BMAP_EXT_FLAGS),
@@ -2334,7 +2334,7 @@ DECLARE_EVENT_CLASS(xfs_map_extent_deferred_class,
 		 int whichfork,
 		 xfs_fileoff_t offset,
 		 xfs_filblks_t len,
-		 xfs_exntst_t state),
+		 unsigned state),
 	TP_ARGS(mp, agno, op, agbno, ino, whichfork, offset, len, state),
 	TP_STRUCT__entry(
 		__field(dev_t, dev)
@@ -2344,7 +2344,7 @@ DECLARE_EVENT_CLASS(xfs_map_extent_deferred_class,
 		__field(int, whichfork)
 		__field(xfs_fileoff_t, l_loff)
 		__field(xfs_filblks_t, l_len)
-		__field(xfs_exntst_t, l_state)
+		__field(unsigned, l_state)
 		__field(int, op)
 	),
 	TP_fast_assign(
@@ -2358,7 +2358,7 @@ DECLARE_EVENT_CLASS(xfs_map_extent_deferred_class,
 		__entry->l_state = state;
 		__entry->op = op;
 	),
-	TP_printk("dev %d:%d op %d agno %u agbno %u owner %lld %s offset %llu len %llu state %d",
+	TP_printk("dev %d:%d op %d agno %u agbno %u owner %lld %s offset %llu len %llu state %u",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->op,
 		  __entry->agno,
@@ -2378,7 +2378,7 @@ DEFINE_EVENT(xfs_map_extent_deferred_class, name, \
 		 int whichfork, \
 		 xfs_fileoff_t offset, \
 		 xfs_filblks_t len, \
-		 xfs_exntst_t state), \
+		 unsigned state), \
 	TP_ARGS(mp, agno, op, agbno, ino, whichfork, offset, len, state))
 
 DEFINE_DEFER_EVENT(xfs_defer_cancel);
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index a0c5dbda18aa..cbb2494ce8e7 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -255,7 +255,7 @@ int xfs_trans_log_finish_rmap_update(struct xfs_trans *tp,
 		struct xfs_rud_log_item *rudp, enum xfs_rmap_intent_type type,
 		uint64_t owner, int whichfork, xfs_fileoff_t startoff,
 		xfs_fsblock_t startblock, xfs_filblks_t blockcount,
-		xfs_exntst_t state, struct xfs_btree_cur **pcur);
+		unsigned state, struct xfs_btree_cur **pcur);
 
 /* refcount updates */
 enum xfs_refcount_intent_type;
@@ -279,6 +279,6 @@ int xfs_trans_log_finish_bmap_update(struct xfs_trans *tp,
 		struct xfs_bud_log_item *rudp, enum xfs_bmap_intent_type type,
 		struct xfs_inode *ip, int whichfork, xfs_fileoff_t startoff,
 		xfs_fsblock_t startblock, xfs_filblks_t *blockcount,
-		xfs_exntst_t state);
+		unsigned state);
 
 #endif	/* __XFS_TRANS_H__ */
diff --git a/fs/xfs/xfs_trans_bmap.c b/fs/xfs/xfs_trans_bmap.c
index 741c558b2179..332c0b4cec61 100644
--- a/fs/xfs/xfs_trans_bmap.c
+++ b/fs/xfs/xfs_trans_bmap.c
@@ -49,7 +49,7 @@ xfs_trans_log_finish_bmap_update(
 	xfs_fileoff_t			startoff,
 	xfs_fsblock_t			startblock,
 	xfs_filblks_t			*blockcount,
-	xfs_exntst_t			state)
+	unsigned			state)
 {
 	int				error;
 
@@ -111,7 +111,7 @@ xfs_trans_set_bmap_flags(
 	struct xfs_map_extent		*bmap,
 	enum xfs_bmap_intent_type	type,
 	int				whichfork,
-	xfs_exntst_t			state)
+	unsigned			state)
 {
 	bmap->me_flags = 0;
 	switch (type) {
diff --git a/fs/xfs/xfs_trans_rmap.c b/fs/xfs/xfs_trans_rmap.c
index 05b00e40251f..7c2e43ed7243 100644
--- a/fs/xfs/xfs_trans_rmap.c
+++ b/fs/xfs/xfs_trans_rmap.c
@@ -23,7 +23,7 @@ xfs_trans_set_rmap_flags(
 	struct xfs_map_extent		*rmap,
 	enum xfs_rmap_intent_type	type,
 	int				whichfork,
-	xfs_exntst_t			state)
+	unsigned			state)
 {
 	rmap->me_flags = 0;
 	if (state == XFS_EXT_UNWRITTEN)
@@ -87,7 +87,7 @@ xfs_trans_log_finish_rmap_update(
 	xfs_fileoff_t			startoff,
 	xfs_fsblock_t			startblock,
 	xfs_filblks_t			blockcount,
-	xfs_exntst_t			state,
+	unsigned			state,
 	struct xfs_btree_cur		**pcur)
 {
 	int				error;
-- 
2.19.1

Re: [PATCH 0/3] xfs: enums don't work with __print_symbolic

[Darrick J. Wong]

On Thu, Nov 22, 2018 at 09:59:55AM +1100, Dave Chinner wrote:
> Hi folks,
> 
> When trying to read traces from fsx failurs, I kept coming across
> fields in the traces that had no output what-so-ever. The common
> theme was that they all were tables that were parsed by
> __print_symbolic() and the value definitions were enums.
> 
> Unfortunately, __print_symbolic() does some crazy stuff involving
> stringification of the table that is passed to it, which means
> the enums are turned into strings and so their never get treated as
> enums after pre-processing. The result is a format string that looks
> like:
> 
> # cat /sys/kernel/debug/tracing/events/xfs/xfs_iomap_alloc/format
> .....
> print fmt: ..... __print_symbolic(REC->type, { XFS_IO_INVALID, "invalid" }, { XFS_IO_DELALLOC, "delalloc" }, { XFS_IO_UNWRITTEN, "unwritten" }, { XFS_IO_OVERWRITE, "overwrite" }, { XFS_IO_COW, "CoW" }, { XFS_IO_HOLE, "hole" }), ....
> #
> 
> And, well, XFS_IO_OVERWRITE is a string, not an integer value of 3.
> Hence __print_symbolic never prints out the correct value.
> 
> The way to fix this is to turn all the enums into defined macros,
> that way the preprocessor still substitutes the value for the
> stringified table as the it does string matches. The result is:
> 
> __print_symbolic(REC->type, { 0, "hole" }, { 1, "delalloc" }, { 2, "unwritten" }, { 3, "overwrite" }, { 4, "CoW" })
> 
> And the trace events now print the type correctly in the trace.
> 
> This series fixes the cases I noticed by removing the various enums
> that end up in trace format tables.

According to the documentation provided in
samples/trace_events/trace-events-sample.h, you're supposed to wrap all
the enum members in the trace header file so that they're encoded in the
trace output:

TRACE_DEFINE_ENUM(XFS_IO_HOLE);
TRACE_DEFINE_ENUM(XFS_IO_DELALLOC);
TRACE_DEFINE_ENUM(XFS_IO_UNWRITTEN);
TRACE_DEFINE_ENUM(XFS_IO_OVERWRITE);
TRACE_DEFINE_ENUM(XFS_IO_COW);

And then the trace output is decoded properly:

$ grep fmt /sys/kernel/debug/tracing/events/xfs/xfs_iomap_alloc/format
print fmt: "dev %d:%d ino 0x%llx size 0x%llx offset 0x%llx count %zd
  type %s startoff 0x%llx startblock %lld blockcount 0x%llx", ((unsigned
  int) ((REC->dev) >> 20)), ((unsigned int) ((REC->dev) & ((1U << 20) -
  1))), REC->ino, REC->size, REC->offset, REC->count,

  __print_symbolic(REC->type, { 0, "hole" }, { 1, "delalloc" },
  { 2, "unwritten" }, { 3, "overwrite" }, { 4, "CoW" }),

  REC->startoff, (int64_t)REC->startblock, REC->blockcount

(I added the line wrapping and extra newlines to make it obvious).

I'd rather add that weird and unexpectedly documented kludge to
xfs_trace.h than go changing things treewide.  Frankly, why not just
move XFS_IO_TYPES to xfs_trace.h?

(Maybe I'll do that and add enum prettyprinting back to the scrub
tracepoints.)

--D

((Seriously, why are key ftrace calls documented only in the sample
code??))

> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH 0/3] xfs: enums don't work with __print_symbolic

[Dave Chinner]

On Wed, Nov 21, 2018 at 03:42:53PM -0800, Darrick J. Wong wrote:
> On Thu, Nov 22, 2018 at 09:59:55AM +1100, Dave Chinner wrote:
> > Hi folks,
> > 
> > When trying to read traces from fsx failurs, I kept coming across
> > fields in the traces that had no output what-so-ever. The common
> > theme was that they all were tables that were parsed by
> > __print_symbolic() and the value definitions were enums.
> > 
> > Unfortunately, __print_symbolic() does some crazy stuff involving
> > stringification of the table that is passed to it, which means
> > the enums are turned into strings and so their never get treated as
> > enums after pre-processing. The result is a format string that looks
> > like:
> > 
> > # cat /sys/kernel/debug/tracing/events/xfs/xfs_iomap_alloc/format
> > .....
> > print fmt: ..... __print_symbolic(REC->type, { XFS_IO_INVALID, "invalid" }, { XFS_IO_DELALLOC, "delalloc" }, { XFS_IO_UNWRITTEN, "unwritten" }, { XFS_IO_OVERWRITE, "overwrite" }, { XFS_IO_COW, "CoW" }, { XFS_IO_HOLE, "hole" }), ....
> > #
> > 
> > And, well, XFS_IO_OVERWRITE is a string, not an integer value of 3.
> > Hence __print_symbolic never prints out the correct value.
> > 
> > The way to fix this is to turn all the enums into defined macros,
> > that way the preprocessor still substitutes the value for the
> > stringified table as the it does string matches. The result is:
> > 
> > __print_symbolic(REC->type, { 0, "hole" }, { 1, "delalloc" }, { 2, "unwritten" }, { 3, "overwrite" }, { 4, "CoW" })
> > 
> > And the trace events now print the type correctly in the trace.
> > 
> > This series fixes the cases I noticed by removing the various enums
> > that end up in trace format tables.
> 
> According to the documentation provided in
> samples/trace_events/trace-events-sample.h, you're supposed to wrap all
> the enum members in the trace header file so that they're encoded in the
> trace output:
> 
> TRACE_DEFINE_ENUM(XFS_IO_HOLE);
> TRACE_DEFINE_ENUM(XFS_IO_DELALLOC);
> TRACE_DEFINE_ENUM(XFS_IO_UNWRITTEN);
> TRACE_DEFINE_ENUM(XFS_IO_OVERWRITE);
> TRACE_DEFINE_ENUM(XFS_IO_COW);

That's horrible. Essentially, we have to declare all enums twice,
and they can't be in the same place?

> I'd rather add that weird and unexpectedly documented kludge to
> xfs_trace.h than go changing things treewide.  Frankly, why not just
> move XFS_IO_TYPES to xfs_trace.h?

Well, we originally put all these type arrays next to the place
where there are defined so it's easy to remind ourselves that
whenever we add a new enum/type we have to update the tracing output
array, too....

> ((Seriously, why are key ftrace calls documented only in the sample
> code??))

Code is all the docmentation you need, right? :P

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com