Hi Christoph,
On Fri, Aug 30, 2019 at 09:39:10AM -0700, Christoph Hellwig wrote:
> On Thu, Aug 29, 2019 at 06:50:48PM +0800, Gao Xiang wrote:
> > > Please use an erofs_ prefix for all your functions.
> >
> > It is already a static function, I have no idea what is wrong here.
>
> Which part of all wasn't clear? Have you looked at the prefixes for
> most functions in the various other big filesystems?
I will add erofs prefix to free_inode as you said.
At least, all non-prefix functions in erofs are all static functions,
it won't pollute namespace... I will add "erofs_" to other meaningful
callbacks...And as you can see...
cifs/cifsfs.c
1303:cifs_init_inodecache(void)
1509: rc = cifs_init_inodecache();
hpfs/super.c
254:static int init_inodecache(void)
771: int err = init_inodecache();
minix/inode.c
84:static int __init init_inodecache(void)
665: int err = init_inodecache();
isofs/inode.c
88:static int __init init_inodecache(void)
1580: int err = init_inodecache();
bfs/inode.c
261:static int __init init_inodecache(void)
468: int err = init_inodecache();
ext4/super.c
1144:static int __init init_inodecache(void)
6115: err = init_inodecache();
reiserfs/super.c
666:static int __init init_inodecache(void)
2606: ret = init_inodecache();
squashfs/super.c
406:static int __init init_inodecache(void)
430: int err = init_inodecache();
udf/super.c
177:static int __init init_inodecache(void)
232: err = init_inodecache();
qnx4/inode.c
358:static int init_inodecache(void)
399: err = init_inodecache();
ufs/super.c
1463:static int __init init_inodecache(void)
1517: int err = init_inodecache();
qnx6/inode.c
618:static int init_inodecache(void)
659: err = init_inodecache();
f2fs/super.c
3540:static int __init init_inodecache(void)
3572: err = init_inodecache();
>
> > > > + /* 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.
> >
> > I cannot fully understand your point (sorry about my English),
> > I will reply you about this later.
>
> With that I mean that you should:
>
> 1) remove is_inode_fast_symlink and just opencode it in the few places
> using it
> 2) remove the check in this place entirely as it is not needed
> 3) remove the comment quoted above as it is more confusing than not
> having the comment
Got it, thanks!
>
> > > Is there any good reasons to use buffer heads like this in new code
> > > vs directly using bios?
> >
> > This page can save in bdev page cache, it contains not only the erofs
> > superblock so it can be fetched in page cache later.
>
> If you want it in the page cache why not use read_mapping_page or similar?
It's reasonable, I will change as you suggested.
(The difference is whether it has some buffer_head to the sb page or not...)
>
> > > > +/* set up default EROFS parameters */
> > > > +static void default_options(struct erofs_sb_info *sbi)
> > > > +{
> > > > +}
> > >
> > > No need to add an empty function.
> >
> > Later patch will fill this function.
>
> Please only add the function in the patch actually adding the
> functionality.
That was my fault when spilting patches...considering
it's an >7KLOC filesystem (maybe spilting the whole xfs or
ext4 properly is more harder)... Anyway, that is my fault.
>
> > > > +}
> > >
> > > 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.
> >
> > See Al's comments,
> > https://lore.kernel.org/r/20190720224955.GD17978@xxxxxxxxxxxxxxxxxx/
>
> With that code it makes sense. In this paticular patch it does not.
> So please add it only when actually needed.
Same as above...
Thanks,
Gao Xiang
