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 03/24] erofs: add super block operations
Date: Thu, 29 Aug 2019 03:15:45 -0700
Message-ID: <20190829101545.GC20598@infradead.org> (raw)
In-Reply-To: <20190802125347.166018-4-gaoxiang25@huawei.com>

On Fri, Aug 02, 2019 at 08:53:26PM +0800, Gao Xiang wrote:
> +static int __init erofs_init_inode_cache(void)
> +{
> +	erofs_inode_cachep = kmem_cache_create("erofs_inode",
> +					       sizeof(struct erofs_vnode), 0,
> +					       SLAB_RECLAIM_ACCOUNT,
> +					       init_once);
> +
> +	return erofs_inode_cachep ? 0 : -ENOMEM;

Please just use normal if/else.  Also having this function seems
entirely pointless.

> +static void erofs_exit_inode_cache(void)
> +{
> +	kmem_cache_destroy(erofs_inode_cachep);
> +}

Same for this one.

> +static void free_inode(struct inode *inode)

Please use an erofs_ prefix for all your functions.

> +{
> +	struct erofs_vnode *vi = EROFS_V(inode);

Why is this called vnode instead of inode?  That seems like a rather
odd naming for a Linux file system.

> +
> +	/* be careful RCU symlink path (see ext4_inode_info->i_data)! */
> +	if (is_inode_fast_symlink(inode))
> +		kfree(inode->i_link);

is_inode_fast_symlink only shows up in a later patch.  And really
obsfucates the check here in the only caller as you can just do an
unconditional kfree here - i_link will be NULL except for the case
where you explicitly set it.

Also this code is nothing like ext4, so the code seems a little confusing.

> +static bool check_layout_compatibility(struct super_block *sb,
> +				       struct erofs_super_block *layout)
> +{
> +	const unsigned int requirements = le32_to_cpu(layout->requirements);

Why is the variable name for the on-disk subperblock layout?  We usually
still calls this something with sb in the name, e.g. dsb. for disk
super block.

> +	EROFS_SB(sb)->requirements = requirements;
> +
> +	/* check if current kernel meets all mandatory requirements */
> +	if (requirements & (~EROFS_ALL_REQUIREMENTS)) {
> +		errln("unidentified requirements %x, please upgrade kernel version",
> +		      requirements & ~EROFS_ALL_REQUIREMENTS);
> +		return false;
> +	}
> +	return true;

Note that normally we call this features, but that doesn't really
matter too much.

> +static int superblock_read(struct super_block *sb)
> +{
> +	struct erofs_sb_info *sbi;
> +	struct buffer_head *bh;
> +	struct erofs_super_block *layout;
> +	unsigned int blkszbits;
> +	int ret;
> +
> +	bh = sb_bread(sb, 0);

Is there any good reasons to use buffer heads like this in new code
vs directly using bios?

> +
> +	sbi->blocks = le32_to_cpu(layout->blocks);
> +	sbi->meta_blkaddr = le32_to_cpu(layout->meta_blkaddr);
> +	sbi->islotbits = ffs(sizeof(struct erofs_inode_v1)) - 1;
> +	sbi->root_nid = le16_to_cpu(layout->root_nid);
> +	sbi->inos = le64_to_cpu(layout->inos);
> +
> +	sbi->build_time = le64_to_cpu(layout->build_time);
> +	sbi->build_time_nsec = le32_to_cpu(layout->build_time_nsec);
> +
> +	memcpy(&sb->s_uuid, layout->uuid, sizeof(layout->uuid));
> +	memcpy(sbi->volume_name, layout->volume_name,
> +	       sizeof(layout->volume_name));

s_uuid should preferably be a uuid_t (assuming it is a real BE uuid,
if it is le it should be a guid_t).

> +/* set up default EROFS parameters */
> +static void default_options(struct erofs_sb_info *sbi)
> +{
> +}

No need to add an empty function.

> +static int erofs_fill_super(struct super_block *sb, void *data, int silent)
> +{
> +	struct inode *inode;
> +	struct erofs_sb_info *sbi;
> +	int err;
> +
> +	infoln("fill_super, device -> %s", sb->s_id);
> +	infoln("options -> %s", (char *)data);

That is some very verbose debug info.  We usually don't add that and
let people trace the function instead.  Also you should probably
implement the new mount API.
new mount API.

> +static void erofs_kill_sb(struct super_block *sb)
> +{
> +	struct erofs_sb_info *sbi;
> +
> +	WARN_ON(sb->s_magic != EROFS_SUPER_MAGIC);
> +	infoln("unmounting for %s", sb->s_id);
> +
> +	kill_block_super(sb);
> +
> +	sbi = EROFS_SB(sb);
> +	if (!sbi)
> +		return;
> +	kfree(sbi);
> +	sb->s_fs_info = NULL;
> +}

Why is this needed?  You can just free your sb privatte information in
->put_super and wire up kill_block_super as the ->kill_sb method
directly.

  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
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 [this message]
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=20190829101545.GC20598@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