[Ocfs2-devel] [PATCH v3 3/5] ocfs2: ocfs2_initialize_super does cleanup job before return error
heming.zhao at suse.com
heming.zhao at suse.com
Thu Apr 28 14:59:11 UTC 2022
On 4/28/22 19:37, Joseph Qi wrote:
>
>
> On 4/24/22 9:09 PM, Heming Zhao wrote:
>> After this patch, when error, ocfs2_fill_super doesn't take care to
>> release resources which are allocated in ocfs2_initialize_super.
>>
>> Reviewed-by: Joseph Qi <joseph.qi at linux.alibaba.com>
>> Signed-off-by: Heming Zhao <heming.zhao at suse.com>
>> ---
>> fs/ocfs2/super.c | 59 +++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 41 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>> index 8014c690ef72..758ea3313f88 100644
>> --- a/fs/ocfs2/super.c
>> +++ b/fs/ocfs2/super.c
>> @@ -2022,7 +2022,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> if (!osb) {
>> status = -ENOMEM;
>> mlog_errno(status);
>> - goto bail;
>> + goto out;
>> }
>>
>> sb->s_fs_info = osb;
>> @@ -2083,7 +2083,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> mlog(ML_ERROR, "Invalid number of node slots (%u)\n",
>> osb->max_slots);
>> status = -EINVAL;
>> - goto bail;
>> + goto out;
>> }
>>
>> ocfs2_orphan_scan_init(osb);
>> @@ -2092,7 +2092,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> if (status) {
>> mlog(ML_ERROR, "Unable to initialize recovery state\n");
>> mlog_errno(status);
>> - goto bail;
>> + goto out;
>> }
>>
>> init_waitqueue_head(&osb->checkpoint_event);
>> @@ -2116,7 +2116,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> if (!osb->vol_label) {
>> mlog(ML_ERROR, "unable to alloc vol label\n");
>> status = -ENOMEM;
>> - goto bail;
>> + goto out_recovery_map;
>> }
>>
>> osb->slot_recovery_generations =
>> @@ -2125,7 +2125,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> if (!osb->slot_recovery_generations) {
>> status = -ENOMEM;
>> mlog_errno(status);
>> - goto bail;
>> + goto out_vol_label;
>> }
>>
>> init_waitqueue_head(&osb->osb_wipe_event);
>> @@ -2135,7 +2135,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> if (!osb->osb_orphan_wipes) {
>> status = -ENOMEM;
>> mlog_errno(status);
>> - goto bail;
>> + goto out_slot_recovery_gen;
>> }
>>
>> osb->osb_rf_lock_tree = RB_ROOT;
>> @@ -2151,13 +2151,13 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> mlog(ML_ERROR, "couldn't mount because of unsupported "
>> "optional features (%x).\n", i);
>> status = -EINVAL;
>> - goto bail;
>> + goto out_orphan_wipes;
>> }
>> if (!sb_rdonly(osb->sb) && (i = OCFS2_HAS_RO_COMPAT_FEATURE(osb->sb, ~OCFS2_FEATURE_RO_COMPAT_SUPP))) {
>> mlog(ML_ERROR, "couldn't mount RDWR because of "
>> "unsupported optional features (%x).\n", i);
>> status = -EINVAL;
>> - goto bail;
>> + goto out_orphan_wipes;
>> }
>>
>> if (ocfs2_clusterinfo_valid(osb)) {
>> @@ -2178,7 +2178,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> "cluster stack label (%s) \n",
>> osb->osb_cluster_stack);
>> status = -EINVAL;
>> - goto bail;
>> + goto out_orphan_wipes;
>> }
>> memcpy(osb->osb_cluster_name,
>> OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster,
>> @@ -2198,7 +2198,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> */
>> status = ocfs2_journal_alloc(osb);
>> if (status < 0)
>> - goto bail;
>> + goto out_orphan_wipes;
>>
>> INIT_WORK(&osb->dquot_drop_work, ocfs2_drop_dquot_refs);
>> init_llist_head(&osb->dquot_drop_list);
>> @@ -2213,7 +2213,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> mlog(ML_ERROR, "Volume has invalid cluster size (%d)\n",
>> osb->s_clustersize);
>> status = -EINVAL;
>> - goto bail;
>> + goto out_journal;
>> }
>>
>> total_blocks = ocfs2_clusters_to_blocks(osb->sb,
>> @@ -2225,14 +2225,14 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> mlog(ML_ERROR, "Volume too large "
>> "to mount safely on this system");
>> status = -EFBIG;
>> - goto bail;
>> + goto out_journal;
>> }
>>
>> if (ocfs2_setup_osb_uuid(osb, di->id2.i_super.s_uuid,
>> sizeof(di->id2.i_super.s_uuid))) {
>> mlog(ML_ERROR, "Out of memory trying to setup our uuid.\n");
>> status = -ENOMEM;
>> - goto bail;
>> + goto out_journal;
>> }
>>
>> strlcpy(osb->vol_label, di->id2.i_super.s_label,
>> @@ -2252,7 +2252,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> if (!osb->osb_dlm_debug) {
>> status = -ENOMEM;
>> mlog_errno(status);
>> - goto bail;
>> + goto out_uuid_str;
>> }
>>
>> atomic_set(&osb->vol_state, VOLUME_INIT);
>> @@ -2261,7 +2261,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> status = ocfs2_init_global_system_inodes(osb);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto bail;
>> + goto out_dlm_out;
>> }
>>
>> /*
>> @@ -2272,7 +2272,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> if (!inode) {
>> status = -EINVAL;
>> mlog_errno(status);
>> - goto bail;
>> + goto out_system_inodes;
>> }
>>
>> osb->bitmap_blkno = OCFS2_I(inode)->ip_blkno;
>> @@ -2285,16 +2285,39 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> status = ocfs2_init_slot_info(osb);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto bail;
>> + goto out_system_inodes;
>> }
>>
>> osb->ocfs2_wq = alloc_ordered_workqueue("ocfs2_wq", WQ_MEM_RECLAIM);
>> if (!osb->ocfs2_wq) {
>> status = -ENOMEM;
>> mlog_errno(status);
>> + goto out_slot_info;
>> }
>>
>> -bail:
>> + return status;
>> +
>> +out_slot_info:
>> + ocfs2_free_slot_info(osb);
>> +out_system_inodes:
>> + ocfs2_release_system_inodes(osb);
>> +out_dlm_out:
>> + ocfs2_put_dlm_debug(osb->osb_dlm_debug);
>> +out_uuid_str:
>> + kfree(osb->uuid_str);
>> +out_journal:
>> + kfree(osb->journal);
>> +out_orphan_wipes:
>> + kfree(osb->osb_orphan_wipes);
>> +out_slot_recovery_gen:
>> + kfree(osb->slot_recovery_generations);
>> +out_vol_label:
>> + kfree(osb->vol_label);
>> +out_recovery_map:
>> + kfree(osb->recovery_map);
>> +out:
>> + kfree(osb);
>> + sb->s_fs_info = NULL;
>
> Or just set "osb" to NULL is enough to keep code consistency.
> Other looks good. Once it is resolved, feel free to add:
>
> Reviewed-by: Joseph Qi <joseph.qi at linux.alibaba.com>
In the front of ocfs2_initialize_super, the sb->s_fs_info is set to osb.
So set osb to NULL is not enough, sb->s_fs_info still keep wild pointer.
The caller (ocfs2_fill_super) will fetch the wild value from OCFS2_SB(sb).
Another question: do I need to resend v3 patch with "Reviewed-by: Joseph Qi ..."
Thanks,
Heming
>
>
>> return status;
>> }
>>
>
More information about the Ocfs2-devel
mailing list