[Ocfs2-devel] [PATCH v1 4/5] ocfs2: ocfs2_mount_volume does cleanup job before return error

[heming.zhao at suse.com]

On 4/9/22 21:46, Joseph Qi wrote:
> 
> 
> On 4/8/22 6:30 PM, Heming Zhao wrote:
>> After this patch, when error, ocfs2_fill_super doesn't take care to
>> release resources which are allocated in ocfs2_mount_volume.
>>
>> Signed-off-by: Heming Zhao <heming.zhao at suse.com>
>> ---
>>   fs/ocfs2/super.c | 32 ++++++++++++++++++++------------
>>   1 file changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>> index 8443ba031dec..d4b7a29cb364 100644
>> --- a/fs/ocfs2/super.c
>> +++ b/fs/ocfs2/super.c
>> @@ -1803,11 +1803,10 @@ static int ocfs2_get_sector(struct super_block *sb,
>>   static int ocfs2_mount_volume(struct super_block *sb)
>>   {
>>   	int status = 0;
>> -	int unlock_super = 0;
>>   	struct ocfs2_super *osb = OCFS2_SB(sb);
>>   
>>   	if (ocfs2_is_hard_readonly(osb))
>> -		goto leave;
>> +		goto out;
>>   
>>   	mutex_init(&osb->obs_trim_fs_mutex);
>>   
>> @@ -1817,44 +1816,53 @@ static int ocfs2_mount_volume(struct super_block *sb)
>>   		if (status == -EBADR && ocfs2_userspace_stack(osb))
>>   			mlog(ML_ERROR, "couldn't mount because cluster name on"
>>   			" disk does not match the running cluster name.\n");
>> -		goto leave;
>> +		goto out;
>>   	}
>>   
>>   	status = ocfs2_super_lock(osb, 1);
>>   	if (status < 0) {
>>   		mlog_errno(status);
>> -		goto leave;
>> +		goto out_dlm;
>>   	}
>> -	unlock_super = 1;
>>   
>>   	/* This will load up the node map and add ourselves to it. */
>>   	status = ocfs2_find_slot(osb);
>>   	if (status < 0) {
>>   		mlog_errno(status);
>> -		goto leave;
>> +		goto out_super_lock;
>>   	}
>>   
>>   	/* load all node-local system inodes */
>>   	status = ocfs2_init_local_system_inodes(osb);
>>   	if (status < 0) {
>>   		mlog_errno(status);
>> -		goto leave;
>> +		goto out_super_lock;
>>   	}
>>   
>>   	status = ocfs2_check_volume(osb);
>>   	if (status < 0) {
>>   		mlog_errno(status);
>> -		goto leave;
>> +		goto out_system_inodes;
>>   	}
>>   
>>   	status = ocfs2_truncate_log_init(osb);
>> -	if (status < 0)
>> +	if (status < 0) {
>>   		mlog_errno(status);
>> +		goto out_journal_shutdown;
> 
> ocfs2_check_volume() not only does journal initialization,
> but also local alloc.

I agree and already handled this case. please read following explanation.




call flow:



ocfs2_fill_super

  ocfs2_mount_volume

   + ocfs2_init_local_system_inodes //[1]

   + ocfs2_check_volume

      ocfs2_load_local_alloc //may fail



If ocfs2_load_local_alloc fails, only "//local_alloc:%04d" is bad.

And other alloced inodes (from [1]) still exist.


also note, the sys inodes release function is ocfs2_release_system_inodes(),

which could handle both osb->global_system_inodes[] & osb->local_system_inodes
[]
are NULL pointer case.



the cleanup job will be done in both places:
- ocfs2_mount_volume (patch 4/5)

- ocfs2_fill_super (patch 5/5)



related code with patch 4/5:



```ocfs2_mount_volume()


     ... ...

     status = ocfs2_check_volume(osb);

     if (status < 0) {

         mlog_errno(status);



         //need to release global/local sys inodes (by ocfs2_release_system_inodes())

         // which alloced by above [1].


         goto out_system_inodes;

     }

     status = ocfs2_truncate_log_init(osb);

     if (status < 0) {

         mlog_errno(status);



         //need to release journal: by ocfs2_journal_shutdown()

         //need to release global/local sys inodes: by ocfs2_release_system_inodes()

         goto out_journal_shutdown;

     }

     ocfs2_super_unlock(osb, 1);

     return 0;


out_journal_shutdown:

     ocfs2_journal_shutdown(osb); //through to label "out_system_inodes".

out_system_inodes:

     ocfs2_release_system_inodes(osb);

out_super_lock:

     ocfs2_super_unlock(osb, 1);

out_dlm:

     ocfs2_dlm_shutdown(osb, 0);

out:
     return status;

}

```

> 
>> +	}
>>   
>> -leave:
>> -	if (unlock_super)
>> -		ocfs2_super_unlock(osb, 1);
>> +	ocfs2_super_unlock(osb, 1);
>> +	return status;
> Explicitly return 0 may be better, which means this is the
> normal path.

OK

Thanks,
Heming

> 
>>
>> +out_journal_shutdown:
>> +	ocfs2_journal_shutdown(osb);
>> +out_system_inodes:
>> +	ocfs2_release_system_inodes(osb);
>> +out_super_lock:
>> +	ocfs2_super_unlock(osb, 1);
>> +out_dlm:
>> +	ocfs2_dlm_shutdown(osb, 0);
>> +out:
>>   	return status;
>>   }
>>   
>