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@xxxxxxxxxx> > --- > 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@xxxxxxxxxx> > + */ > +#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? > + /* fast symlink (following ext4) */ This actually originates in FFS. But it is so common that the comment seems a little pointless. > + 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. > + /* inline symlink data shouldn't across page boundary as well */ ... should not cross .. > + 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. > + 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 > + err = read_inode(inode, data + ofs); > + if (!err) { if (err) goto out_unlock; .. and save one level of indentation. > + 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. > + 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. > +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.
