Re: [PATCH v6 01/24] erofs: add on-disk layout

Pavel Machek
2019-09-02T08:40:24Z

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

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]