Hi Christoph,
Sorry about my first response, sincerely...
Here is my redo-ed comments to all your suggestions...
On Thu, Aug 29, 2019 at 02:59:54AM -0700, Christoph Hellwig wrote:
> > --- /dev/null
> > +++ b/fs/erofs/erofs_fs.h
> > @@ -0,0 +1,316 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only OR Apache-2.0 */
> > +/*
> > + * linux/fs/erofs/erofs_fs.h
>
> Please remove the pointless file names in the comment headers.
Has already fixed in the latest version, and I will resend
the whole v9 addressing all suggestions from you these days...
However it's somewhat hard to spilt the whole code prefectly
since erofs is ~7KLOC code and linux-fsdevel mailing list
have some limitation, I have spilted it in the form of features...
>
> > +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.
fixed in
https://lore.kernel.org/linux-fsdevel/20190901055130.30572-2-hsiangkao@xxxxxxx/
>
> > +/* 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.
fixed in
https://lore.kernel.org/linux-fsdevel/20190901055130.30572-5-hsiangkao@xxxxxxx/
>
> > +/*
> > + * erofs inode data mapping:
> > + * 0 - inode plain without inline data A:
> > + * inode, [xattrs], ... | ... | no-holed data
> > + * 1 - inode VLE compression B (legacy):
> > + * inode, [xattrs], extents ... | ...
> > + * 2 - inode plain with inline data C:
> > + * inode, [xattrs], last_inline_data, ... | ... | no-holed data
> > + * 3 - inode compression D:
> > + * inode, [xattrs], map_header, extents ... | ...
> > + * 4~7 - reserved
> > + */
> > +enum {
> > + EROFS_INODE_FLAT_PLAIN,
>
> This one doesn't actually seem to be used.
It could be better has a name though, because 1) erofs.mkfs uses this
definition explicitly, and we keep this on-disk definition erofs_fs.h
file up with erofs-utils.
2) For kernel use, first we have,
datamode < EROFS_INODE_LAYOUT_MAX; and
!erofs_inode_is_data_compressed, so there are only two mode here,
1) EROFS_INODE_FLAT_INLINE,
2) EROFS_INODE_FLAT_PLAIN
if its datamode isn't EROFS_INODE_FLAT_INLINE (tail-end block packing),
it should be EROFS_INODE_FLAT_PLAIN.
The detailed logic in erofs_read_inode and
erofs_map_blocks_flatmode....
>
> > + EROFS_INODE_FLAT_COMPRESSION_LEGACY,
>
> why are we adding a legacy field to a brand new file system?
The difference is just EROFS_INODE_FLAT_COMPRESSION_LEGACY doesn't
have z_erofs_map_header, so it only supports default (4k clustersize)
fixed-sized output compression rather than per-file setting, nothing
special at all...
>
> > + EROFS_INODE_FLAT_INLINE,
> > + EROFS_INODE_FLAT_COMPRESSION,
> > + EROFS_INODE_LAYOUT_MAX
>
> It seems like these come from the on-disk format, in which case they
> should have explicit values assigned to them.
Fixed in
https://lore.kernel.org/linux-fsdevel/20190901055130.30572-3-hsiangkao@xxxxxxx/
>
> Btw, I think it generally helps file system implementation quality
> if you use a separate header for the on-disk structures vs in-memory
> structures, as that keeps it clear in everyones mind what needs to
> stay persistent and what can be chenged easily.
All fields in this file are on-disk representation by design
(no logic for in-memory presentation).
>
> > +static bool erofs_inode_is_data_compressed(unsigned int datamode)
> > +{
> > + if (datamode == EROFS_INODE_FLAT_COMPRESSION)
> > + return true;
> > + return datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY;
> > +}
>
> This looks like a really obsfucated way to write:
>
> return datamode == EROFS_INODE_FLAT_COMPRESSION ||
> datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY;
Fixed in
https://lore.kernel.org/linux-fsdevel/20190901055130.30572-6-hsiangkao@xxxxxxx/
>
> > +/* 28 */__le32 i_reserved2;
> > +} __packed;
>
> Sane comment as above.
Fixed in
https://lore.kernel.org/linux-fsdevel/20190901055130.30572-5-hsiangkao@xxxxxxx/
>
> > +
> > +/* 32 bytes on-disk inode */
> > +#define EROFS_INODE_LAYOUT_V1 0
> > +/* 64 bytes on-disk inode */
> > +#define EROFS_INODE_LAYOUT_V2 1
> > +
> > +struct erofs_inode_v2 {
> > +/* 0 */__le16 i_advise;
>
> Why do we have two inode version in a newly added file system?
There is no new or old, both can be used for the current EROFS in one image.
v2 is an exhanced on-disk inode form, it has 64 bytes,
v1 is more reduced one, which is already suitable for Android use case.
>
> > +#define ondisk_xattr_ibody_size(count) ({\
> > + u32 __count = le16_to_cpu(count); \
> > + ((__count) == 0) ? 0 : \
> > + sizeof(struct erofs_xattr_ibody_header) + \
> > + sizeof(__u32) * ((__count) - 1); })
>
> This would be much more readable as a function.
Fixed in
https://lore.kernel.org/linux-fsdevel/20190901055130.30572-4-hsiangkao@xxxxxxx/
>
> > +#define EROFS_XATTR_ENTRY_SIZE(entry) EROFS_XATTR_ALIGN( \
> > + sizeof(struct erofs_xattr_entry) + \
> > + (entry)->e_name_len + le16_to_cpu((entry)->e_value_size))
>
> Same here.
Fixed in
https://lore.kernel.org/linux-fsdevel/20190901055130.30572-4-hsiangkao@xxxxxxx/
>
> > +/* available compression algorithm types */
> > +enum {
> > + Z_EROFS_COMPRESSION_LZ4,
> > + Z_EROFS_COMPRESSION_MAX
> > +};
>
> Seems like an on-disk value again that should use explicitly assigned
> numbers.
Fixed in
https://lore.kernel.org/linux-fsdevel/20190901055130.30572-3-hsiangkao@xxxxxxx/
Thanks,
Gao Xiang
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
