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

[Joseph Qi]

On 4/12/22 4:22 PM, Heming Zhao wrote:
> My last reply (on 4.10) had messy format, because thunderbird automatically
> add '\r' after '\n'. For easy review, I use neomutt to resend my replay.
> Please check my comments in below.
> 
> On Sat, Apr 09, 2022 at 09:46:19PM +0800, 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 after patched 4/5:
> (please check my comment start with "hm:")
> ```
> static int ocfs2_mount_volume(struct super_block *sb)
> {
> 	... ...
> 	
> 	status = ocfs2_check_volume(osb);
> 	if (status < 0) {
> 		mlog_errno(status);
> 		
> 		//hm:
> 		//need to release global/local sys inodes (by //ocfs2_release_system_inodes())
> 		//which are alloced by above [1].
> 		goto out_system_inodes;
> 	}
> 	
> 	status = ocfs2_truncate_log_init(osb);
> 	if (status < 0) {
> 		mlog_errno(status);
> 		
> 		//hm:
> 		//need to release journal: by ocfs2_journal_shutdown()
> 		//need to release global/local sys inodes: byocfs2_release_system_inodes()
> 		goto out_journal_shutdown;
> 	}
> 	
> 	ocfs2_super_unlock(osb, 1);
> 	return status; //<== hm: will change to 0 under next comment
> 	
> out_journal_shutdown:
> 	ocfs2_journal_shutdown(osb); //hm: 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;
> }
> ``` 
I mean we may need call ocfs2_shutdown_local_alloc() to do the cleanup.
In ocfs2_fill_super(), you don't do that.

Thanks,
Joseph