Re: [PATCH 02/50] fs: make the i_state flags an enum

[Sun YangKai <sunk67188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3396 bytes --]

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.

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)		\
+	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

[-- Attachment #2: x-macro.patch --]
[-- Type: text/x-patch, Size: 2946 bytes --]

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)		\
+	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 */