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

[Heming Zhao]

On Tue, Apr 12, 2022 at 07:54:35PM +0800, Joseph Qi wrote:
> 
> 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.
> 
 
You are right, my mistake.

I also found another mistake in this patch: "goto out_journal_shutdown" is wrong.
After ocfs2_truncate_log_init() failure, code firstly calls ocfs2_journal_shutdown()
to clean journal then calls ocfs2_release_system_inodes(), it will trigger crash.

- Heming