xfs: enums don't work with __print_symbolic
cover xfs: enums don't work with __print_symbolic
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
From: Dave Chinner <[email protected]>
__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.
--- fs/xfs/xfs_aops.h | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
fs/xfs/xfs_aops.h +5 -7
@@ -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" }, \
Signature · 1 line
2.19.1
PATCH 2/3 xfs: remove xfs_lookup_t enum because tracing
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
__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.
--- 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(-)
fs/xfs/libxfs/xfs_btree.c +1 -1
@@ -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 */
fs/xfs/libxfs/xfs_btree.h +4 -4
@@ -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 *);
fs/xfs/libxfs/xfs_ialloc.c +1 -1
@@ -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;
fs/xfs/libxfs/xfs_ialloc.h +1 -1
@@ -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.
fs/xfs/libxfs/xfs_types.h +0 -4
@@ -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
fs/xfs/xfs_trace.h +3 -3
@@ -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 */
Signature · 1 line
2.19.1
PATCH 3/3 xfs: remove xfs_extst_t enum because tracing
In reply to 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.
--- 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(-)
fs/xfs/libxfs/xfs_bmap.c +1 -1
@@ -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;
fs/xfs/libxfs/xfs_bmap.h +1 -1
@@ -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,
fs/xfs/libxfs/xfs_rmap.c +1 -1
@@ -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;
fs/xfs/libxfs/xfs_rmap.h +1 -1
@@ -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,
fs/xfs/libxfs/xfs_types.h +3 -4
@@ -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 1typedef 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 ;/*
fs/xfs/xfs_bmap_item.c +1 -1
@@ -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;
fs/xfs/xfs_rmap_item.c +1 -1
@@ -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 ;
fs/xfs/xfs_trace.h +6 -6
@@ -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);
fs/xfs/xfs_trans.h +2 -2
@@ -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__ */
fs/xfs/xfs_trans_bmap.c +2 -2
@@ -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) {
fs/xfs/xfs_trans_rmap.c +2 -2
@@ -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;
Signature · 1 line
2.19.1
Quoted 25 lines · 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_OVERWRITEis a string, not an integer value of 3. Hence__print_symbolicnever 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:
Quoted 6 lines
__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??))
Quoted 5 lines
Cheers,
Dave.
Quoted 38 lines · Darrick J. Wong
Quoted 25 lines · 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_OVERWRITEis a string, not an integer value of 3. Hence__print_symbolicnever 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:
Quoted 6 lines
__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:
Quoted 5 lines
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?
Quoted 3 lines
I'd rather add that weird and unexpectedly documented kludge to
xfs_trace.hthan go changing things treewide. Frankly, why not just moveXFS_IO_TYPEStoxfs_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....
Quoted 2 lines
((Seriously, why are key ftrace calls documented only in the sample code??))
Code is all the docmentation you need, right? :P
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]