Re: [PATCH v6 01/24] erofs: add on-disk layout - Gao Xiang via Linux-erofs

From: Gao Xiang via Linux-erofs <linux-erofs@lists.ozlabs.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: devel@driverdev.osuosl.org,
	Stephen Rothwell <sfr@canb.auug.org.au>,
	linux-erofs@lists.ozlabs.org, Jan Kara <jack@suse.cz>,
	"Darrick J . Wong" <darrick.wong@oracle.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Amir Goldstein <amir73il@gmail.com>,
	Dave Chinner <david@fromorbit.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Theodore Ts'o <tytso@mit.edu>, Pavel Machek <pavel@denx.de>,
	David Sterba <dsterba@suse.cz>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Jaegeuk Kim <jaegeuk@kernel.org>,
	linux-fsdevel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Miao Xie <miaoxie@huawei.com>
Subject: Re: [PATCH v6 01/24] erofs: add on-disk layout
Date: Sun, 1 Sep 2019 15:54:11 +0800
Message-ID: <20190901075240.GA2938@hsiangkao-HP-ZHAN-66-Pro-G1> (raw)
In-Reply-To: <20190829095954.GB20598@infradead.org>

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@aol.com/

> 
> > +/* 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@aol.com/

> 
> > +/*
> > + * 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@aol.com/

> 
> 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@aol.com/

> 
> > +/* 28 */__le32 i_reserved2;
> > +} __packed;
> 
> Sane comment as above.

Fixed in
https://lore.kernel.org/linux-fsdevel/20190901055130.30572-5-hsiangkao@aol.com/

> 
> > +
> > +/* 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@aol.com/
> 
> > +#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@aol.com/

> 
> > +/* 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@aol.com/

Thanks,
Gao Xiang