linux-ext4 - Re: [PATCH 02/50] make the i

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3846996.MHq7AAxBmi@saltykitkat>
Date: Fri, 22 Aug 2025 20:11:59 +0800
From: Sun YangKai <sunk67188@...il.com>
To: mmpgouride@...il.com
Cc: brauner@...nel.org, josef@...icpanda.com, kernel-team@...com,
 linux-btrfs@...r.kernel.org, linux-ext4@...r.kernel.org,
 linux-fsdevel@...r.kernel.org, linux-xfs@...r.kernel.org,
 sunk67188@...il.com, viro@...iv.linux.org.uk
Subject: Re: [PATCH 02/50] make the i_state flags an enum

> On Aug 22, 2025, at 19:18, Sun YangKai <sunk67188@...il.com> 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