Re: [PATCH 11/22] staging: erofs: kill all failure handling in fill_super()
On 2019/7/29 14:51, Gao Xiang wrote: > .kill_sb() will do that instead in order to remove duplicated code. > > Note that the initialzation of managed_cache is now moved > after s_root is assigned since it's more preferred to iput() > in .put_super() and all inodes should be evicted before > the end of generic_shutdown_super(sb). > > Suggested-by: Al Viro > Signed-off-by: Gao Xiang > --- > drivers/staging/erofs/super.c | 121 +++--- > 1 file changed, 53 insertions(+), 68 deletions(-) > > diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c > index bfb6e1e09781..af5d87793e4d 100644 > --- a/drivers/staging/erofs/super.c > +++ b/drivers/staging/erofs/super.c > @@ -343,51 +343,52 @@ static const struct address_space_operations > managed_cache_aops = { > .invalidatepage = managed_cache_invalidatepage, > }; > > -static struct inode *erofs_init_managed_cache(struct super_block *sb) > +static int erofs_init_managed_cache(struct super_block *sb) > { > - struct inode *inode = new_inode(sb); > + struct erofs_sb_info *const sbi = EROFS_SB(sb); > + struct inode *const inode = new_inode(sb); > > if (unlikely(!inode)) > - return ERR_PTR(-ENOMEM); > + return -ENOMEM; > > set_nlink(inode, 1); > inode->i_size = OFFSET_MAX; > > inode->i_mapping->a_ops = &managed_cache_aops; > mapping_set_gfp_mask(inode->i_mapping, > - GFP_NOFS | __GFP_HIGHMEM | > - __GFP_MOVABLE | __GFP_NOFAIL); > - return inode; > + GFP_NOFS | __GFP_HIGHMEM | __GFP_MOVABLE); It looks above change is not belong to this patch? Otherwise, it looks good to me. Reviewed-by: Chao Yu Thanks, > + sbi->managed_cache = inode; > + return 0; > } > - > +#else > +static int erofs_init_managed_cache(struct super_block *sb) { return 0; } > #endif > > static int erofs_fill_super(struct super_block *sb, void *data, int silent) > { > struct inode *inode; > struct erofs_sb_info *sbi; > - int err = -EINVAL; > + int err; > > infoln("fill_super, device -> %s", sb->s_id); > infoln("options -> %s", (char *)data); > > + sb->s_magic = EROFS_SUPER_MAGIC; > + > if (unlikely(!sb_set_blocksize(sb, EROFS_BLKSIZ))) { > errln("failed to set erofs blksize"); > - goto err; > + return -EINVAL; > } > > sbi = kzalloc(sizeof(*sbi), GFP_KERNEL); > - if (unlikely(!sbi)) { > - err = -ENOMEM; > - goto err; > - } > - sb->s_fs_info = sbi; > + if (unlikely(!sbi)) > + return -ENOMEM; > > + sb->s_fs_info = sbi; > err = superblock_read(sb); > if (err) > - goto err_sbread; > + return err; > > - sb->s_magic = EROFS_SUPER_MAGIC; > sb->s_flags |= SB_RDONLY | SB_NOATIME; > sb->s_maxbytes = MAX_LFS_FILESIZE; > sb->s_time_gran = 1; > @@ -397,13 +398,12 @@ static int erofs_fill_super(struct super_block *sb, > void *data, int silent) > #ifdef CONFIG_EROFS_FS_XATTR > sb->s_xattr = erofs_xattr_handlers; > #endif > - > /* set erofs default mount options */ > default_options(sbi); > > err = parse_options(sb, data); > - if (err) > - goto err_parseopt; > + if (unlikely(err)) > + return err; > > if (!silent) > infoln("root inode @ nid %llu", ROOT_NID(sbi)); > @@ -417,93 +417,78 @@ static int erofs_fill_super(struct super_block *sb, > void *data, int silent) > INIT_RADIX_TREE(&sbi->workstn_tree, GFP_ATOMIC); > #endif > > -#ifdef EROFS_FS_HAS_MANAGED_CACHE > - sbi->managed_cache = erofs_init_managed_cache(sb); > - if (IS_ERR(sbi->managed_cache)) { > - err = PTR_ERR(sbi->managed_cache); > - goto err_init_managed_cache; > - } > -#endif > - > /* get the root inode */ > inode = erofs_iget(sb, ROOT_NID(sbi), true); > - if (IS_ERR(inode)) { > - err = PTR_ERR(inode); > - goto err_iget; > - } > + if (IS_ERR(inode)) > + return PTR_ERR(inode); > > - if (!S_ISDIR(inode->i_mode)) { > + if (unlikely(!S_ISDIR(inode->i_mode))) { > errln("rootino(nid %llu) is not a directory(i_mode %o)", > ROOT_NID(sbi), inode->i_mode); > - err = -EINVAL; > iput(inode); > - goto err_iget; > + return -EINVAL; > } > > sb->s_root = d_make_root(inode); > - if (!sb->s_root) { > - err = -ENOMEM; > - goto err_iget; > - } > + if (unlikely(!sb->s_root)) > + return -ENOMEM; > > erofs_shrinker_register(sb); > + /* sb->s_umount is already locked, SB_ACTIVE and SB_BORN are not set */ > + err = erofs_init_managed_cache(sb); > + if (unlikely(err)) > + retu
Re: [PATCH 11/22] staging: erofs: kill all failure handling in fill_super()
Hi Chao, On 2019/7/31 16:15, Chao Yu wrote: > On 2019/7/29 14:51, Gao Xiang wrote: >> .kill_sb() will do that instead in order to remove duplicated code. >> >> Note that the initialzation of managed_cache is now moved >> after s_root is assigned since it's more preferred to iput() >> in .put_super() and all inodes should be evicted before >> the end of generic_shutdown_super(sb). >> >> Suggested-by: Al Viro >> Signed-off-by: Gao Xiang >> --- >> drivers/staging/erofs/super.c | 121 +++--- >> 1 file changed, 53 insertions(+), 68 deletions(-) >> >> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c >> index bfb6e1e09781..af5d87793e4d 100644 >> --- a/drivers/staging/erofs/super.c >> +++ b/drivers/staging/erofs/super.c >> @@ -343,51 +343,52 @@ static const struct address_space_operations >> managed_cache_aops = { >> .invalidatepage = managed_cache_invalidatepage, >> }; >> >> -static struct inode *erofs_init_managed_cache(struct super_block *sb) >> +static int erofs_init_managed_cache(struct super_block *sb) >> { >> -struct inode *inode = new_inode(sb); >> +struct erofs_sb_info *const sbi = EROFS_SB(sb); >> +struct inode *const inode = new_inode(sb); >> >> if (unlikely(!inode)) >> -return ERR_PTR(-ENOMEM); >> +return -ENOMEM; >> >> set_nlink(inode, 1); >> inode->i_size = OFFSET_MAX; >> >> inode->i_mapping->a_ops = &managed_cache_aops; >> mapping_set_gfp_mask(inode->i_mapping, >> - GFP_NOFS | __GFP_HIGHMEM | >> - __GFP_MOVABLE | __GFP_NOFAIL); >> -return inode; >> + GFP_NOFS | __GFP_HIGHMEM | __GFP_MOVABLE); Seems so, I will add a new patch addressing on it if needed. Thanks, Gao Xiang > > It looks above change is not belong to this patch? > > Otherwise, it looks good to me. > > Reviewed-by: Chao Yu > > Thanks, > >> +sbi->managed_cache = inode; >> +return 0; >> } >> - >> +#else >> +static int erofs_init_managed_cache(struct super_block *sb) { return 0; } >> #endif >> >> static int erofs_fill_super(struct super_block *sb, void *data, int silent) >> { >> struct inode *inode; >> struct erofs_sb_info *sbi; >> -int err = -EINVAL; >> +int err; >> >> infoln("fill_super, device -> %s", sb->s_id); >> infoln("options -> %s", (char *)data); >> >> +sb->s_magic = EROFS_SUPER_MAGIC; >> + >> if (unlikely(!sb_set_blocksize(sb, EROFS_BLKSIZ))) { >> errln("failed to set erofs blksize"); >> -goto err; >> +return -EINVAL; >> } >> >> sbi = kzalloc(sizeof(*sbi), GFP_KERNEL); >> -if (unlikely(!sbi)) { >> -err = -ENOMEM; >> -goto err; >> -} >> -sb->s_fs_info = sbi; >> +if (unlikely(!sbi)) >> +return -ENOMEM; >> >> +sb->s_fs_info = sbi; >> err = superblock_read(sb); >> if (err) >> -goto err_sbread; >> +return err; >> >> -sb->s_magic = EROFS_SUPER_MAGIC; >> sb->s_flags |= SB_RDONLY | SB_NOATIME; >> sb->s_maxbytes = MAX_LFS_FILESIZE; >> sb->s_time_gran = 1; >> @@ -397,13 +398,12 @@ static int erofs_fill_super(struct super_block *sb, >> void *data, int silent) >> #ifdef CONFIG_EROFS_FS_XATTR >> sb->s_xattr = erofs_xattr_handlers; >> #endif >> - >> /* set erofs default mount options */ >> default_options(sbi); >> >> err = parse_options(sb, data); >> -if (err) >> -goto err_parseopt; >> +if (unlikely(err)) >> +return err; >> >> if (!silent) >> infoln("root inode @ nid %llu", ROOT_NID(sbi)); >> @@ -417,93 +417,78 @@ static int erofs_fill_super(struct super_block *sb, >> void *data, int silent) >> INIT_RADIX_TREE(&sbi->workstn_tree, GFP_ATOMIC); >> #endif >> >> -#ifdef EROFS_FS_HAS_MANAGED_CACHE >> -sbi->managed_cache = erofs_init_managed_cache(sb); >> -if (IS_ERR(sbi->managed_cache)) { >> -err = PTR_ERR(sbi->managed_cache); >> -goto err_init_managed_cache; >> -} >> -#endif >> - >> /* get the root inode */ >> inode = erofs_iget(sb, ROOT_NID(sbi), true); >> -if (IS_ERR(inode)) { >> -err = PTR_ERR(inode); >> -goto err_iget; >> -} >> +if (IS_ERR(inode)) >> +return PTR_ERR(inode); >> >> -if (!S_ISDIR(inode->i_mode)) { >> +if (unlikely(!S_ISDIR(inode->i_mode))) { >> errln("rootino(nid %llu) is not a directory(i_mode %o)", >>ROOT_NID(sbi), inode->i_mode); >> -err = -EINVAL; >> iput(inode); >> -goto err_iget; >> +return -EINVAL; >> } >> >> sb->s_root = d_make_root(inode); >> -if (!sb->s_root) { >> -err = -ENOMEM; >> -goto err_iget; >> -} >> +if (unlikely(!sb->s_root)) >> +return -ENOMEM; >> >>