[Ocfs2-devel] [PATCH v1 4/5] ocfs2: ocfs2_mount_volume does cleanup job before return error
Heming Zhao
heming.zhao at suse.com
Tue Apr 12 08:22:48 UTC 2022
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;
}
```
> > + }
> >
> > -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;
> > }
> >
>
More information about the Ocfs2-devel
mailing list