Skip to contentSkip to conversation

ext4: use ext4_fc_tl_mem in fast-commit replay path

PATCH Applied Eric Biggers 3 participants 5 messages Dec 17, 2022 – Feb 9, 2023 ext4 file system

lore
Share & tools
lore

Eric Biggers

author
#0 lore raw

PATCH 1/1 ext4: use ext4_fc_tl_mem in fast-commit replay path

From: Eric Biggers <[email protected]>

To avoid 'sparse' warnings about missing endianness conversions, don't store native endianness values into struct ext4_fc_tl. Instead, use a separate struct type, ext4_fc_tl_mem.

Fixes dcc5827484d6 ("ext4: factor out ext4_fc_get_tl()")
Cc Ye Bin <[email protected]>
Signed-off-by Eric Biggers <[email protected]>
---
 fs/ext4/fast_commit.c | 44 +++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)
fs/ext4/fast_commit.c +26 -18
index 4594b62f147b..b06de728b3b6 100644
@@ -1332,8 +1332,14 @@ struct dentry_info_args {
 	char *dname;
 };
 
+/* Same as struct ext4_fc_tl, but uses native endianness fields */
+struct ext4_fc_tl_mem {
+	u16 fc_tag;
+	u16 fc_len;
+};
+
 static inline void tl_to_darg(struct dentry_info_args *darg,
-			      struct ext4_fc_tl *tl, u8 *val)
+			      struct ext4_fc_tl_mem *tl, u8 *val)
 {
 	struct ext4_fc_dentry_info fcd;
 

@@ -1345,16 +1351,18 @@ static inline void tl_to_darg(struct dentry_info_args *darg,
 	darg->dname_len = tl->fc_len - sizeof(struct ext4_fc_dentry_info);
 }
 
-static inline void ext4_fc_get_tl(struct ext4_fc_tl *tl, u8 *val)
+static inline void ext4_fc_get_tl(struct ext4_fc_tl_mem *tl, u8 *val)
 {
-	memcpy(tl, val, EXT4_FC_TAG_BASE_LEN);
-	tl->fc_len = le16_to_cpu(tl->fc_len);
-	tl->fc_tag = le16_to_cpu(tl->fc_tag);
+	struct ext4_fc_tl tl_disk;
+
+	memcpy(&tl_disk, val, EXT4_FC_TAG_BASE_LEN);
+	tl->fc_len = le16_to_cpu(tl_disk.fc_len);
+	tl->fc_tag = le16_to_cpu(tl_disk.fc_tag);
 }
 
 /* Unlink replay function */
-static int ext4_fc_replay_unlink(struct super_block *sb, struct ext4_fc_tl *tl,
-				 u8 *val)
+static int ext4_fc_replay_unlink(struct super_block *sb,
+				 struct ext4_fc_tl_mem *tl, u8 *val)
 {
 	struct inode *inode, *old_parent;
 	struct qstr entry;

@@ -1451,8 +1459,8 @@ static int ext4_fc_replay_link_internal(struct super_block *sb,
 }
 
 /* Link replay function */
-static int ext4_fc_replay_link(struct super_block *sb, struct ext4_fc_tl *tl,
-			       u8 *val)
+static int ext4_fc_replay_link(struct super_block *sb,
+			       struct ext4_fc_tl_mem *tl, u8 *val)
 {
 	struct inode *inode;
 	struct dentry_info_args darg;

@@ -1506,8 +1514,8 @@ static int ext4_fc_record_modified_inode(struct super_block *sb, int ino)
 /*
  * Inode replay function
  */
-static int ext4_fc_replay_inode(struct super_block *sb, struct ext4_fc_tl *tl,
-				u8 *val)
+static int ext4_fc_replay_inode(struct super_block *sb,
+				struct ext4_fc_tl_mem *tl, u8 *val)
 {
 	struct ext4_fc_inode fc_inode;
 	struct ext4_inode *raw_inode;

@@ -1609,8 +1617,8 @@ static int ext4_fc_replay_inode(struct super_block *sb, struct ext4_fc_tl *tl,
  * inode for which we are trying to create a dentry here, should already have
  * been replayed before we start here.
  */
-static int ext4_fc_replay_create(struct super_block *sb, struct ext4_fc_tl *tl,
-				 u8 *val)
+static int ext4_fc_replay_create(struct super_block *sb,
+				 struct ext4_fc_tl_mem *tl, u8 *val)
 {
 	int ret = 0;
 	struct inode *inode = NULL;

@@ -1708,7 +1716,7 @@ int ext4_fc_record_regions(struct super_block *sb, int ino,
 
 /* Replay add range tag */
 static int ext4_fc_replay_add_range(struct super_block *sb,
-				    struct ext4_fc_tl *tl, u8 *val)
+				    struct ext4_fc_tl_mem *tl, u8 *val)
 {
 	struct ext4_fc_add_range fc_add_ex;
 	struct ext4_extent newex, *ex;

@@ -1828,8 +1836,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb,
 
 /* Replay DEL_RANGE tag */
 static int
-ext4_fc_replay_del_range(struct super_block *sb, struct ext4_fc_tl *tl,
-			 u8 *val)
+ext4_fc_replay_del_range(struct super_block *sb,
+			 struct ext4_fc_tl_mem *tl, u8 *val)
 {
 	struct inode *inode;
 	struct ext4_fc_del_range lrange;

@@ -2025,7 +2033,7 @@ static int ext4_fc_replay_scan(journal_t *journal,
 	struct ext4_fc_replay_state *state;
 	int ret = JBD2_FC_REPLAY_CONTINUE;
 	struct ext4_fc_add_range ext;
-	struct ext4_fc_tl tl;
+	struct ext4_fc_tl_mem tl;
 	struct ext4_fc_tail tail;
 	__u8 *start, *end, *cur, *val;
 	struct ext4_fc_head head;

@@ -2144,7 +2152,7 @@ static int ext4_fc_replay(journal_t *journal, struct buffer_head *bh,
 {
 	struct super_block *sb = journal->j_private;
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
-	struct ext4_fc_tl tl;
+	struct ext4_fc_tl_mem tl;
 	__u8 *start, *end, *cur, *val;
 	int ret = JBD2_FC_REPLAY_CONTINUE;
 	struct ext4_fc_replay_state *state = &sbi->s_fc_replay_state;

base-commit: 77856d911a8c8724ee8e2b09d55979fc1de8f1c0

Signature · 1 line
2.38.1
reviewer
#1 lore raw
Quoted 9 lines · Eric Biggers

From: Eric Biggers <[email protected]>

To avoid 'sparse' warnings about missing endianness conversions, don't store native endianness values into struct ext4_fc_tl. Instead, use a separate struct type, ext4_fc_tl_mem.

Fixes dcc5827484d6 ("ext4: factor out ext4_fc_get_tl()")
Cc Ye Bin <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Looks good to me. Just one nit below:

fs/ext4/fast_commit.c static inline void tl_to_darg(struct dentry_info_args *darg, · line 1361

Quoted 12 lines
fs/ext4/fast_commit.c +6 -4
-static inline void ext4_fc_get_tl(struct ext4_fc_tl *tl, u8 *val)
+static inline void ext4_fc_get_tl(struct ext4_fc_tl_mem *tl, u8 *val)
 {
-	memcpy(tl, val, EXT4_FC_TAG_BASE_LEN);
-	tl->fc_len = le16_to_cpu(tl->fc_len);
-	tl->fc_tag = le16_to_cpu(tl->fc_tag);
+	struct ext4_fc_tl tl_disk;
+
+	memcpy(&tl_disk, val, EXT4_FC_TAG_BASE_LEN);
+	tl->fc_len = le16_to_cpu(tl_disk.fc_len);
+	tl->fc_tag = le16_to_cpu(tl_disk.fc_tag);
 }

So why not just:

	struct ext4_fc_tl *tl_disk = (struct ext4_fc_tl *)val;

instead of memcpy?

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
author
#2 lore raw
Quoted 28 lines · Jan Kara
Quoted 9 lines · Eric Biggers

From: Eric Biggers <[email protected]>

To avoid 'sparse' warnings about missing endianness conversions, don't store native endianness values into struct ext4_fc_tl. Instead, use a separate struct type, ext4_fc_tl_mem.

Fixes dcc5827484d6 ("ext4: factor out ext4_fc_get_tl()")
Cc Ye Bin <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Looks good to me. Just one nit below:

fs/ext4/fast_commit.c static inline void tl_to_darg(struct dentry_info_args *darg, · line 1361

Quoted 12 lines
fs/ext4/fast_commit.c +6 -4
-static inline void ext4_fc_get_tl(struct ext4_fc_tl *tl, u8 *val)
+static inline void ext4_fc_get_tl(struct ext4_fc_tl_mem *tl, u8 *val)
 {
-	memcpy(tl, val, EXT4_FC_TAG_BASE_LEN);
-	tl->fc_len = le16_to_cpu(tl->fc_len);
-	tl->fc_tag = le16_to_cpu(tl->fc_tag);
+	struct ext4_fc_tl tl_disk;
+
+	memcpy(&tl_disk, val, EXT4_FC_TAG_BASE_LEN);
+	tl->fc_len = le16_to_cpu(tl_disk.fc_len);
+	tl->fc_tag = le16_to_cpu(tl_disk.fc_tag);
 }

So why not just:

Quoted 3 lines
	struct ext4_fc_tl *tl_disk = (struct ext4_fc_tl *)val;

instead of memcpy?

That would result in unaligned memory accesses.

- Eric

reviewer Reviewed-by
#3 lore raw
Quoted 29 lines · Eric Biggers
Quoted 28 lines · Jan Kara
Quoted 9 lines · Eric Biggers

From: Eric Biggers <[email protected]>

To avoid 'sparse' warnings about missing endianness conversions, don't store native endianness values into struct ext4_fc_tl. Instead, use a separate struct type, ext4_fc_tl_mem.

Fixes dcc5827484d6 ("ext4: factor out ext4_fc_get_tl()")
Cc Ye Bin <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Looks good to me. Just one nit below:

fs/ext4/fast_commit.c static inline void tl_to_darg(struct dentry_info_args *darg, · line 1361

Quoted 12 lines
fs/ext4/fast_commit.c +6 -4
-static inline void ext4_fc_get_tl(struct ext4_fc_tl *tl, u8 *val)
+static inline void ext4_fc_get_tl(struct ext4_fc_tl_mem *tl, u8 *val)
 {
-	memcpy(tl, val, EXT4_FC_TAG_BASE_LEN);
-	tl->fc_len = le16_to_cpu(tl->fc_len);
-	tl->fc_tag = le16_to_cpu(tl->fc_tag);
+	struct ext4_fc_tl tl_disk;
+
+	memcpy(&tl_disk, val, EXT4_FC_TAG_BASE_LEN);
+	tl->fc_len = le16_to_cpu(tl_disk.fc_len);
+	tl->fc_tag = le16_to_cpu(tl_disk.fc_tag);
 }

So why not just:

Quoted 5 lines
Quoted 3 lines
	struct ext4_fc_tl *tl_disk = (struct ext4_fc_tl *)val;

instead of memcpy?

That would result in unaligned memory accesses.

Indeed. Thanks for explanation! Feel free to add:

Reviewed-by Jan Kara <[email protected]>

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
maintainer Applied
#4 lore raw
In reply to Eric Biggers
Quoted 8 lines · Eric Biggers

From: Eric Biggers <[email protected]>

To avoid 'sparse' warnings about missing endianness conversions, don't store native endianness values into struct ext4_fc_tl. Instead, use a separate struct type, ext4_fc_tl_mem.

[...]

Applied, thanks!

[1/1] ext4: use ext4_fc_tl_mem in fast-commit replay path

      commit: 11768cfd98136dd8399480c60b7a5d3d3c7b109b

Best regards,

Signature · 1 line
Theodore Ts'o <[email protected]>