diff mbox series

[v1,4/5] ocfs2: ocfs2_mount_volume does cleanup job before return error

Message ID 20220408103012.1419-5-heming.zhao@suse.com (mailing list archive)
State New, archived
Headers show
Series rewrite error handling during mounting stage | expand

Commit Message

Heming Zhao April 8, 2022, 10:30 a.m. UTC
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@suse.com>
---
 fs/ocfs2/super.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Comments

Joseph Qi April 9, 2022, 1:46 p.m. UTC | #1
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@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.

> +	}
>  
> -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.

Thanks,
Joseph

>
> +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;
>  }
>
David Sterba via Ocfs2-devel April 10, 2022, 3:58 a.m. UTC | #2
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@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;
>>   }
>>   
>
Heming Zhao April 12, 2022, 8:22 a.m. UTC | #3
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@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;
}
``` 

> > +	}
> >  
> > -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.

- 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;
> >  }
> >  
>
Joseph Qi April 12, 2022, 11:54 a.m. UTC | #4
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@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
Heming Zhao April 12, 2022, 4:46 p.m. UTC | #5
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@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
diff mbox series

Patch

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;
+	}
 
-leave:
-	if (unlock_super)
-		ocfs2_super_unlock(osb, 1);
+	ocfs2_super_unlock(osb, 1);
+	return status;
 
+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;
 }