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(-)
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;
base-commit: 77856d911a8c8724ee8e2b09d55979fc1de8f1c0
--
2.38.1
On Fri 16-12-22 21:02:12, Eric Biggers wrote:
> 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:
> -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
--
Jan Kara <[email protected]>
SUSE Labs, CR
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 <[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:
>
> > -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 <[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:
> >
> > > -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 <[email protected]>
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Fri, 16 Dec 2022 21:02:12 -0800, Eric Biggers wrote:
> 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,
--
Theodore Ts'o <[email protected]>