> On Aug 22, 2025, at 19:18, Sun YangKai <sunk67188@xxxxxxxxx> wrote:
> > Hi Josef,
> >
> > Sorry for the bothering, and I hope this isn't too far off-topic for the
> > current patch series discussion.
> >
> > I recently learned about the x-macro trick and was wondering if it might
> > be
> > suitable for use in this context since we are rewriting this. I'd
> > appreciate any thoughts or feedback on whether this approach could be
> > applied here.
> I think x-macro is easy to write, but hard to read or grep.
I agree.
However, in this particular case, it's not very difficult to read and it does
help reduce some copy-pasting. So it really comes down to what we value more
in our codebase: straightforward implementation, or adhering to DRY
principles.
>
> > Thanks in advance for your insights!
> >
> > Below is the patch for reference:
> >
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index d7ab4f96d705..6a766aaa457e 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2576,28 +2576,36 @@ static inline void kiocb_clone(struct kiocb
> > *kiocb,
> > struct kiocb *kiocb_src,
> >
> > * __I_{SYNC,NEW,LRU_ISOLATING} are used to derive unique addresses to
> > wait
> > * upon. There's one free address left.
> > */
> >
> > -#define __I_NEW 0
> > -#define I_NEW (1 << __I_NEW)
> > -#define __I_SYNC 1
> > -#define I_SYNC (1 << __I_SYNC)
> > -#define __I_LRU_ISOLATING 2
> > -#define I_LRU_ISOLATING (1 << __I_LRU_ISOLATING)
> > -
> > -#define I_DIRTY_SYNC (1 << 3)
> > -#define I_DIRTY_DATASYNC (1 << 4)
> > -#define I_DIRTY_PAGES (1 << 5)
> > -#define I_WILL_FREE (1 << 6)
> > -#define I_FREEING (1 << 7)
> > -#define I_CLEAR (1 << 8)
> > -#define I_REFERENCED (1 << 9)
> > -#define I_LINKABLE (1 << 10)
> > -#define I_DIRTY_TIME (1 << 11)
> > -#define I_WB_SWITCH (1 << 12)
> > -#define I_OVL_INUSE (1 << 13)
> > -#define I_CREATING (1 << 14)
> > -#define I_DONTCACHE (1 << 15)
> > -#define I_SYNC_QUEUED (1 << 16)
> > -#define I_PINNING_NETFS_WB (1 << 17)
> > +#define INODE_STATE(X) \
>
> And it should be INODE_STATE().
Yes, but I defined X as a parameter so that other names can be used as well.
This is particularly important in the file /include/trace/events/writeback.h,
where the macro X must be consistently defined wherever show_inode_state is
used. Therefore, it’s better to use a more meaningful name—such as
inode_state_name—instead of X. The situation is slightly more complex than a
typical x-macro, since we cannot undefine the helper macro (whether it's called
X or, in this case, inode_state_name).
>
> > + X(I_NEW), \
> > + X(I_SYNC), \
> > + X(I_LRU_ISOLATING), \
> > + X(I_DIRTY_SYNC), \
> > + X(I_DIRTY_DATASYNC), \
> > + X(I_DIRTY_PAGES), \
> > + X(I_WILL_FREE), \
> > + X(I_FREEING), \
> > + X(I_CLEAR), \
> > + X(I_REFERENCED), \
> > + X(I_LINKABLE), \
> > + X(I_DIRTY_TIME), \
> > + X(I_WB_SWITCH), \
> > + X(I_OVL_INUSE), \
> > + X(I_CREATING), \
> > + X(I_DONTCACHE), \
> > + X(I_SYNC_QUEUED), \
> > + X(I_PINNING_NETFS_WB)
> > +
> > +enum __inode_state_bits {
> > + #define X(state) __##state
> > + INODE_STATE(X)
> > + #undef X
> > +};
> > +enum inode_state_bits {
> > + #define X(state) state = (1 << __##state)
> > + INODE_STATE(X)
> > + #undef X
> > +};
> >
> > #define I_DIRTY_INODE (I_DIRTY_SYNC | I_DIRTY_DATASYNC)
> > #define I_DIRTY (I_DIRTY_INODE | I_DIRTY_PAGES)
> > diff --git a/include/trace/events/writeback.h b/include/trace/events/
> > writeback.h
> > index 1e23919c0da9..4c545c72c40a 100644
> > --- a/include/trace/events/writeback.h
> > +++ b/include/trace/events/writeback.h
> > @@ -9,26 +9,10 @@
> > #include <linux/backing-dev.h>
> > #include <linux/writeback.h>
> >
> > -#define show_inode_state(state) \
> > - __print_flags(state, "|", \
> > - {I_DIRTY_SYNC, "I_DIRTY_SYNC"}, \
> > - {I_DIRTY_DATASYNC, "I_DIRTY_DATASYNC"}, \
> > - {I_DIRTY_PAGES, "I_DIRTY_PAGES"}, \
> > - {I_NEW, "I_NEW"}, \
> > - {I_WILL_FREE, "I_WILL_FREE"}, \
> > - {I_FREEING, "I_FREEING"}, \
> > - {I_CLEAR, "I_CLEAR"}, \
> > - {I_SYNC, "I_SYNC"}, \
> > - {I_DIRTY_TIME, "I_DIRTY_TIME"}, \
> > - {I_REFERENCED, "I_REFERENCED"}, \
> > - {I_LINKABLE, "I_LINKABLE"}, \
> > - {I_WB_SWITCH, "I_WB_SWITCH"}, \
> > - {I_OVL_INUSE, "I_OVL_INUSE"}, \
> > - {I_CREATING, "I_CREATING"}, \
> > - {I_DONTCACHE, "I_DONTCACHE"}, \
> > - {I_SYNC_QUEUED, "I_SYNC_QUEUED"}, \
> > - {I_PINNING_NETFS_WB, "I_PINNING_NETFS_WB"}, \
> > - {I_LRU_ISOLATING, "I_LRU_ISOLATING"} \
> > +#define inode_state_name(s) { s, #s }
> > +#define show_inode_state(state) \
> > + __print_flags(state, "|", \
> > + INODE_STATE(inode_state_name) \
> > )
> >
> > /* enums need to be exported to user space */
> >
> > Best regards,
> > Sun YangKai
> > <x-macro.patch>
Thanks,
Sun YangKai
