LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Gao Xiang <gaoxiang25@huawei.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Stephen Rothwell <sfr@canb.auug.org.au>,
	Theodore Ts'o <tytso@mit.edu>, Pavel Machek <pavel@denx.de>,
	David Sterba <dsterba@suse.cz>,
	Amir Goldstein <amir73il@gmail.com>,
	Christoph Hellwig <hch@infradead.org>,
	"Darrick J . Wong" <darrick.wong@oracle.com>,
	Dave Chinner <david@fromorbit.com>,
	Jaegeuk Kim <jaegeuk@kernel.org>, Jan Kara <jack@suse.cz>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-fsdevel@vger.kernel.org, devel@driverdev.osuosl.org,
	LKML <linux-kernel@vger.kernel.org>,
	linux-erofs@lists.ozlabs.org, Chao Yu <yuchao0@huawei.com>,
	Miao Xie <miaoxie@huawei.com>,
	Li Guifu <bluce.liguifu@huawei.com>,
	Fang Wei <fangwei1@huawei.com>
Subject: Re: [PATCH v6 01/24] erofs: add on-disk layout
Date: Thu, 29 Aug 2019 02:59:54 -0700
Message-ID: <20190829095954.GB20598@infradead.org> (raw)
In-Reply-To: <20190802125347.166018-2-gaoxiang25@huawei.com>

> --- /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.

> +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.

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

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

> +	EROFS_INODE_FLAT_COMPRESSION_LEGACY,

why are we adding a legacy field to a brand new file system?

> +	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.

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.

> +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;

> +/* 28 */__le32 i_reserved2;
> +} __packed;

Sane comment as above.

> +
> +/* 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?

> +#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.

> +#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.

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

  reply index

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-02 12:53 [PATCH v6 00/24] erofs: promote erofs from staging Gao Xiang
2019-08-02 12:53 ` [PATCH v6 01/24] erofs: add on-disk layout Gao Xiang
2019-08-29  9:59   ` Christoph Hellwig [this message]
2019-08-29 10:32     ` Gao Xiang
2019-08-29 10:36       ` Christoph Hellwig
2019-08-29 10:58         ` Gao Xiang
2019-08-29 15:58       ` Joe Perches
2019-08-29 17:26         ` Gao Xiang
2019-08-30 12:07         ` David Sterba
2019-08-30 12:18           ` Gao Xiang
2019-09-02  8:43           ` Pavel Machek
2019-09-02 14:07             ` David Sterba
2019-09-03 11:27               ` Pavel Machek
2019-08-29 15:41     ` Gao Xiang
2019-09-01  7:54     ` Gao Xiang
2019-09-02 12:45       ` Christoph Hellwig
2019-09-02 13:02         ` Gao Xiang
2019-09-02  8:40     ` Pavel Machek
2019-09-02 10:35       ` Gao Xiang
2019-08-02 12:53 ` [PATCH v6 02/24] erofs: add erofs in-memory stuffs Gao Xiang
2019-08-02 12:53 ` [PATCH v6 03/24] erofs: add super block operations Gao Xiang
2019-08-29 10:15   ` Christoph Hellwig
2019-08-29 10:50     ` Gao Xiang
2019-08-30 16:39       ` Christoph Hellwig
2019-08-30 17:15         ` Gao Xiang
2019-08-31  0:54           ` Gao Xiang
2019-08-31  6:34           ` Amir Goldstein
2019-08-31  6:48             ` Gao Xiang
2019-09-01  8:54     ` Gao Xiang
2019-09-02 12:51       ` Christoph Hellwig
2019-09-02 14:43         ` Gao Xiang
2019-09-02 15:19           ` Christoph Hellwig
2019-09-02 15:24             ` Gao Xiang
2019-08-02 12:53 ` [PATCH v6 04/24] erofs: add raw address_space operations Gao Xiang
2019-08-29 10:17   ` Christoph Hellwig
2019-08-29 11:46     ` Gao Xiang
2019-08-30 16:40       ` Christoph Hellwig
2019-08-30 17:23         ` Gao Xiang
2019-08-02 12:53 ` [PATCH v6 05/24] erofs: add inode operations Gao Xiang
2019-08-29 10:24   ` Christoph Hellwig
2019-08-29 11:59     ` Gao Xiang
2019-08-30 16:42       ` Christoph Hellwig
2019-08-30 18:46         ` Gao Xiang
2019-09-01  9:34     ` Gao Xiang
2019-09-02 12:53       ` Christoph Hellwig
2019-09-02 13:43       ` David Sterba
2019-09-02 13:55         ` Gao Xiang
2019-08-02 12:53 ` [PATCH v6 06/24] erofs: support special inode Gao Xiang
2019-08-29 10:25   ` Christoph Hellwig
2019-09-01  9:39     ` Gao Xiang
2019-08-02 12:53 ` [PATCH v6 07/24] erofs: add directory operations Gao Xiang
2019-08-02 12:53 ` [PATCH v6 08/24] erofs: add namei functions Gao Xiang
2019-08-29 10:28   ` Christoph Hellwig
2019-08-29 11:28     ` Gao Xiang
2019-08-02 12:53 ` [PATCH v6 09/24] erofs: support tracepoint Gao Xiang
2019-08-02 12:53 ` [PATCH v6 10/24] erofs: update Kconfig and Makefile Gao Xiang
2019-08-02 12:53 ` [PATCH v6 11/24] erofs: introduce xattr & posixacl support Gao Xiang
2019-08-02 12:53 ` [PATCH v6 12/24] erofs: introduce tagged pointer Gao Xiang
2019-08-02 12:53 ` [PATCH v6 13/24] erofs: add compression indexes support Gao Xiang
2019-08-02 12:53 ` [PATCH v6 14/24] erofs: introduce superblock registration Gao Xiang
2019-08-02 12:53 ` [PATCH v6 15/24] erofs: introduce erofs shrinker Gao Xiang
2019-08-02 12:53 ` [PATCH v6 16/24] erofs: introduce workstation for decompression Gao Xiang
2019-08-02 12:53 ` [PATCH v6 17/24] erofs: introduce per-CPU buffers implementation Gao Xiang
2019-08-02 12:53 ` [PATCH v6 18/24] erofs: introduce pagevec for decompression subsystem Gao Xiang
2019-08-02 12:53 ` [PATCH v6 19/24] erofs: add erofs_allocpage() Gao Xiang
2019-08-02 12:53 ` [PATCH v6 20/24] erofs: introduce generic decompression backend Gao Xiang
2019-08-02 12:53 ` [PATCH v6 21/24] erofs: introduce LZ4 decompression inplace Gao Xiang
2019-08-02 12:53 ` [PATCH v6 22/24] erofs: introduce the decompression frontend Gao Xiang
2019-08-02 12:53 ` [PATCH v6 23/24] erofs: introduce cached decompression Gao Xiang
2019-08-02 12:53 ` [PATCH v6 24/24] erofs: add document Gao Xiang

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190829095954.GB20598@infradead.org \
    --to=hch@infradead.org \
    --cc=akpm@linux-foundation.org \
    --cc=amir73il@gmail.com \
    --cc=bluce.liguifu@huawei.com \
    --cc=darrick.wong@oracle.com \
    --cc=david@fromorbit.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=dsterba@suse.cz \
    --cc=fangwei1@huawei.com \
    --cc=gaoxiang25@huawei.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jack@suse.cz \
    --cc=jaegeuk@kernel.org \
    --cc=linux-erofs@lists.ozlabs.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=miaoxie@huawei.com \
    --cc=pavel@denx.de \
    --cc=sfr@canb.auug.org.au \
    --cc=torvalds@linux-foundation.org \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    --cc=yuchao0@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git