Re: [PATCH v6 05/24] erofs: add inode operations - 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 05/24] erofs: add inode operations
Date: Sun, 1 Sep 2019 17:34:00 +0800
Message-ID: <20190901093326.GA6267@hsiangkao-HP-ZHAN-66-Pro-G1> (raw)
In-Reply-To: <20190829102426.GE20598@infradead.org>

Hi Christoph,

Here are redo-ed comments of your suggestions...

On Thu, Aug 29, 2019 at 03:24:26AM -0700, Christoph Hellwig wrote:
> On Fri, Aug 02, 2019 at 08:53:28PM +0800, Gao Xiang wrote:
> > This adds core functions to get, read an inode.
> > It adds statx support as well.
> > 
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> > ---
> >  fs/erofs/inode.c | 291 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 291 insertions(+)
> >  create mode 100644 fs/erofs/inode.c
> > 
> > diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> > new file mode 100644
> > index 000000000000..b6ea997bc4ae
> > --- /dev/null
> > +++ b/fs/erofs/inode.c
> > @@ -0,0 +1,291 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * linux/fs/erofs/inode.c
> > + *
> > + * Copyright (C) 2017-2018 HUAWEI, Inc.
> > + *             http://www.huawei.com/
> > + * Created by Gao Xiang <gaoxiang25@huawei.com>
> > + */
> > +#include "internal.h"
> > +
> > +#include <trace/events/erofs.h>
> > +
> > +/* no locking */
> > +static int read_inode(struct inode *inode, void *data)
> > +{
> > +	struct erofs_vnode *vi = EROFS_V(inode);
> > +	struct erofs_inode_v1 *v1 = data;
> > +	const unsigned int advise = le16_to_cpu(v1->i_advise);
> > +	erofs_blk_t nblks = 0;
> > +
> > +	vi->datamode = __inode_data_mapping(advise);
> 
> What is the deal with these magic underscores here and various
> other similar helpers?

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

underscores means 'internal' in my thought, it seems somewhat
some common practice of Linux kernel, or some recent discussions
about it?... I didn't notice these discussions...

> 
> > +	/* fast symlink (following ext4) */
> 
> This actually originates in FFS.  But it is so common that the comment
> seems a little pointless.

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

> 
> > +	if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) {
> > +		char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL);
> 
> Please just use plain kmalloc everywhere and let the normal kernel
> error injection code take care of injeting any errors.

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

> 
> > +		/* inline symlink data shouldn't across page boundary as well */
> 
> ... should not cross ..

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

> 
> > +		if (unlikely(m_pofs + inode->i_size > PAGE_SIZE)) {
> > +			DBG_BUGON(1);
> > +			kfree(lnk);
> > +			return -EIO;
> > +		}
> > +
> > +		/* get in-page inline data */
> 
> s/get/copy/, but the comment seems rather pointless.

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

> 
> > +		memcpy(lnk, data + m_pofs, inode->i_size);
> > +		lnk[inode->i_size] = '\0';
> > +
> > +		inode->i_link = lnk;
> > +		set_inode_fast_symlink(inode);
> 
> Please just set the ops directly instead of obsfucating that in a single
> caller, single line inline function.  And please set it instead of the
> normal symlink iops in the same place where you also set those.:w

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

> 
> > +	err = read_inode(inode, data + ofs);
> > +	if (!err) {
> 
> 	if (err)
> 		goto out_unlock;
> 
> .. and save one level of indentation.

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

> 
> > +		if (is_inode_layout_compression(inode)) {
> 
> The name of this helper is a little odd.  But I think just
> opencoding it seems generally cleaner anyway.

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

> 
> 
> > +			err = -ENOTSUPP;
> > +			goto out_unlock;
> > +		}
> > +
> > +		inode->i_mapping->a_ops = &erofs_raw_access_aops;
> > +
> > +		/* fill last page if inline data is available */
> > +		err = fill_inline_data(inode, data, ofs);
> 
> Well, I think you should move the is_inode_flat_inline and
> (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) checks from that
> helper here, as otherwise you make everyone wonder why you'd always
> fill out the inline data.

fill_inline_data is killed, and the similar function turns into
erofs_fill_symlink which is called at erofs_fill_inode():

 		case S_IFLNK:
-			/* by default, page_get_link is used for symlink */
-			inode->i_op = &erofs_symlink_iops;
+			err = erofs_fill_symlink(inode, data, ofs);
+			if (err)
+				goto out_unlock;
 			inode_nohighmem(inode);
 			break;

> 
> > +static inline struct inode *erofs_iget_locked(struct super_block *sb,
> > +					      erofs_nid_t nid)
> > +{
> > +	const unsigned long hashval = erofs_inode_hash(nid);
> > +
> > +#if BITS_PER_LONG >= 64
> > +	/* it is safe to use iget_locked for >= 64-bit platform */
> > +	return iget_locked(sb, hashval);
> > +#else
> > +	return iget5_locked(sb, hashval, erofs_ilookup_test_actor,
> > +		erofs_iget_set_actor, &nid);
> > +#endif
> 
> Just use the slightly more complicated 32-bit version everywhere so that
> you have a single actually tested code path.  And then remove this
> helper.

As I said before, 64-bit platforms is common currently,
I think iget_locked is enough.
https://lore.kernel.org/r/20190830184606.GA175612@architecture4/

Thanks,
Gao Xiang