| Message ID | 20220413082957.28774-5-heming.zhao@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rewrite error handling during mounting stage | expand |
On 4/13/22 4:29 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 | 42 +++++++++++++++++++++++++++--------------- > 1 file changed, 27 insertions(+), 15 deletions(-) > > diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c > index 4302c3e9598c..5e860d7162d7 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,54 @@ 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_system_inodes; > + } > > -leave: > - if (unlock_super) > - ocfs2_super_unlock(osb, 1); > + ocfs2_super_unlock(osb, 1); > + return 0; > > +out_system_inodes: > + if (osb->local_alloc_state == OCFS2_LA_ENABLED) > + ocfs2_shutdown_local_alloc(osb); > + ocfs2_release_system_inodes(osb); > + ocfs2_journal_shutdown(osb); > +out_super_lock: > + ocfs2_super_unlock(osb, 1); > +out_dlm: > + ocfs2_dlm_shutdown(osb, 0); > +out: > return status; > } > > @@ -2393,14 +2402,17 @@ static int ocfs2_verify_volume(struct ocfs2_dinode *di, > return status; > } > > +/* > + * If this function returns failure, caller responds to release > + * here alloced resources. > + */ > static int ocfs2_check_volume(struct ocfs2_super *osb) > { > int status; > int dirty; > int local; > - struct ocfs2_dinode *local_alloc = NULL; /* only used if we > - * recover > - * ourselves. */ > + /* only used if we recover ourselves. */ Malformed. Actually I don't think we have to touch this function. Thanks, Joseph > + struct ocfs2_dinode *local_alloc = NULL; > > /* Init our journal object. */ > status = ocfs2_journal_init(osb, &dirty);
On 4/13/22 19:25, Joseph Qi wrote: > > > On 4/13/22 4:29 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 | 42 +++++++++++++++++++++++++++--------------- >> 1 file changed, 27 insertions(+), 15 deletions(-) >> ... ... >> -leave: >> - if (unlock_super) >> - ocfs2_super_unlock(osb, 1); >> + ocfs2_super_unlock(osb, 1); >> + return 0; >> >> +out_system_inodes: >> + if (osb->local_alloc_state == OCFS2_LA_ENABLED) >> + ocfs2_shutdown_local_alloc(osb); >> + ocfs2_release_system_inodes(osb); >> + ocfs2_journal_shutdown(osb); >> +out_super_lock: >> + ocfs2_super_unlock(osb, 1); >> +out_dlm: >> + ocfs2_dlm_shutdown(osb, 0); >> +out: >> return status; >> } >> >> @@ -2393,14 +2402,17 @@ static int ocfs2_verify_volume(struct ocfs2_dinode *di, >> return status; >> } >> >> +/* >> + * If this function returns failure, caller responds to release >> + * here alloced resources. >> + */ >> static int ocfs2_check_volume(struct ocfs2_super *osb) >> { >> int status; >> int dirty; >> int local; >> - struct ocfs2_dinode *local_alloc = NULL; /* only used if we >> - * recover >> - * ourselves. */ >> + /* only used if we recover ourselves. */ > > Malformed. Actually I don't think we have to touch this function. > OK. Will roll back my changes. My original intention is to add comment at front of this function to info caller. To remind them to don't forget to release the alloced resources when fails. - Heming
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c index 4302c3e9598c..5e860d7162d7 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,54 @@ 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_system_inodes; + } -leave: - if (unlock_super) - ocfs2_super_unlock(osb, 1); + ocfs2_super_unlock(osb, 1); + return 0; +out_system_inodes: + if (osb->local_alloc_state == OCFS2_LA_ENABLED) + ocfs2_shutdown_local_alloc(osb); + ocfs2_release_system_inodes(osb); + ocfs2_journal_shutdown(osb); +out_super_lock: + ocfs2_super_unlock(osb, 1); +out_dlm: + ocfs2_dlm_shutdown(osb, 0); +out: return status; } @@ -2393,14 +2402,17 @@ static int ocfs2_verify_volume(struct ocfs2_dinode *di, return status; } +/* + * If this function returns failure, caller responds to release + * here alloced resources. + */ static int ocfs2_check_volume(struct ocfs2_super *osb) { int status; int dirty; int local; - struct ocfs2_dinode *local_alloc = NULL; /* only used if we - * recover - * ourselves. */ + /* only used if we recover ourselves. */ + struct ocfs2_dinode *local_alloc = NULL; /* Init our journal object. */ status = ocfs2_journal_init(osb, &dirty);
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 | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-)