Hi!
> > +struct erofs_super_block {
> > +/* 0 */__le32 magic; /* in the little endian */
> > +/* 4 */__le32 checksum; /* crc32c(super_block) */
> > +/* 8 */__le32 features; /* (aka. feature_compat) */
> > +/* 12 */__u8 blkszbits; /* support block_size == PAGE_SIZE only */
>
> Please remove all the byte offset comments. That is something that can
> easily be checked with gdb or pahole.
I don't think I agree. gdb will tell you byte offsets _on one
architecture_. But filesystem is supposed to be portable between them.
> > +/* 64 */__u8 volume_name[16]; /* volume name */
> > +/* 80 */__le32 requirements; /* (aka. feature_incompat) */
> > +
> > +/* 84 */__u8 reserved2[44];
> > +} __packed; /* 128 bytes */
>
> Please don't add __packed. In this case I think you don't need it
> (but double check with pahole), but even if you would need it using
> proper padding fields and making sure all fields are naturally aligned
> will give you much better code generation on architectures that don't
> support native unaligned access.
This is on-disk structure, right?
drivers/staging/erofs/super.c: struct erofs_super_block *layout;
drivers/staging/erofs/super.c: layout = (struct erofs_super_block
*)((u8 *)bh->b_data
So __packed is right thing to do. If architecture accesses that
slowly, that's ungood, but different structures between architectures
would be really bad.
Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Attachment:
signature.asc
Description: Digital signature
