| Message ID | 20221217050212.150665-1-ebiggers@kernel.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | ext4: use ext4_fc_tl_mem in fast-commit replay path | expand |
On Fri 16-12-22 21:02:12, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > 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 <yebin10@huawei.com> > Signed-off-by: Eric Biggers <ebiggers@google.com> Looks good to me. Just one nit below: > -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
On Wed, Jan 11, 2023 at 03:43:27PM +0100, Jan Kara wrote: > On Fri 16-12-22 21:02:12, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > 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 <yebin10@huawei.com> > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > Looks good to me. Just one nit below: > > > -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? That would result in unaligned memory accesses. - Eric
On Wed 11-01-23 10:30:19, Eric Biggers wrote: > On Wed, Jan 11, 2023 at 03:43:27PM +0100, Jan Kara wrote: > > On Fri 16-12-22 21:02:12, Eric Biggers wrote: > > > From: Eric Biggers <ebiggers@google.com> > > > > > > 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 <yebin10@huawei.com> > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > > > Looks good to me. Just one nit below: > > > > > -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? > > That would result in unaligned memory accesses. Indeed. Thanks for explanation! Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza
On Fri, 16 Dec 2022 21:02:12 -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > 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,
diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c index 4594b62f147b..b06de728b3b6 100644 --- a/fs/ext4/fast_commit.c +++ b/fs/ext4/fast_commit.c @@ -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;