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;
>>
>>
