Joseph Qi
2022-Apr-13 11:37 UTC
[Ocfs2-devel] [PATCH v2 5/5] ocfs2: rewrite error handling of ocfs2_fill_super
It mostly looks good to me. Have you done error injection test after this patchset applied? I'm afraid ocfs2-test can't cover this as of now. Thanks, Joseph On 4/13/22 4:29 PM, Heming Zhao wrote:> Current ocfs2_fill_super() uses one goto label "read_super_error" to > handle all error cases. And with previous serial patches, the error > handling should fork more branches to handle different error cases. > This patch rewrite the error handling of ocfs2_fill_super. > > Signed-off-by: Heming Zhao <heming.zhao at suse.com> > --- > fs/ocfs2/super.c | 67 +++++++++++++++++++++++------------------------- > 1 file changed, 32 insertions(+), 35 deletions(-) > > diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c > index 5e860d7162d7..72673d40d29c 100644 > --- a/fs/ocfs2/super.c > +++ b/fs/ocfs2/super.c > @@ -989,28 +989,27 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > if (!ocfs2_parse_options(sb, data, &parsed_options, 0)) { > status = -EINVAL; > - goto read_super_error; > + goto out; > } > > /* probe for superblock */ > status = ocfs2_sb_probe(sb, &bh, §or_size, &stats); > if (status < 0) { > mlog(ML_ERROR, "superblock probe failed!\n"); > - goto read_super_error; > + goto out; > } > > status = ocfs2_initialize_super(sb, bh, sector_size, &stats); > - osb = OCFS2_SB(sb); > - if (status < 0) { > - mlog_errno(status); > - goto read_super_error; > - } > brelse(bh); > bh = NULL; > + if (status < 0) > + goto out; > + > + osb = OCFS2_SB(sb); > > if (!ocfs2_check_set_options(sb, &parsed_options)) { > status = -EINVAL; > - goto read_super_error; > + goto out_super; > } > osb->s_mount_opt = parsed_options.mount_opt; > osb->s_atime_quantum = parsed_options.atime_quantum; > @@ -1027,7 +1026,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > status = ocfs2_verify_userspace_stack(osb, &parsed_options); > if (status) > - goto read_super_error; > + goto out_super; > > sb->s_magic = OCFS2_SUPER_MAGIC; > > @@ -1041,7 +1040,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > status = -EACCES; > mlog(ML_ERROR, "Readonly device detected but readonly " > "mount was not specified.\n"); > - goto read_super_error; > + goto out_super; > } > > /* You should not be able to start a local heartbeat > @@ -1050,7 +1049,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > status = -EROFS; > mlog(ML_ERROR, "Local heartbeat specified on readonly " > "device.\n"); > - goto read_super_error; > + goto out_super; > } > > status = ocfs2_check_journals_nolocks(osb); > @@ -1059,9 +1058,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > mlog(ML_ERROR, "Recovery required on readonly " > "file system, but write access is " > "unavailable.\n"); > - else > - mlog_errno(status); > - goto read_super_error; > + goto out_super; > } > > ocfs2_set_ro_flag(osb, 1); > @@ -1077,10 +1074,8 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > } > > status = ocfs2_verify_heartbeat(osb); > - if (status < 0) { > - mlog_errno(status); > - goto read_super_error; > - } > + if (status < 0) > + goto out_super; > > osb->osb_debug_root = debugfs_create_dir(osb->uuid_str, > ocfs2_debugfs_root); > @@ -1094,15 +1089,14 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > status = ocfs2_mount_volume(sb); > if (status < 0) > - goto read_super_error; > + goto out_debugfs; > > if (osb->root_inode) > inode = igrab(osb->root_inode); > > if (!inode) { > status = -EIO; > - mlog_errno(status); > - goto read_super_error; > + goto out_dismount; > } > > osb->osb_dev_kset = kset_create_and_add(sb->s_id, NULL, > @@ -1110,7 +1104,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > if (!osb->osb_dev_kset) { > status = -ENOMEM; > mlog(ML_ERROR, "Unable to create device kset %s.\n", sb->s_id); > - goto read_super_error; > + goto out_dismount; > } > > /* Create filecheck sysfs related directories/files at > @@ -1119,14 +1113,13 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > status = -ENOMEM; > mlog(ML_ERROR, "Unable to create filecheck sysfs directory at " > "/sys/fs/ocfs2/%s/filecheck.\n", sb->s_id); > - goto read_super_error; > + goto out_dismount; > } > > root = d_make_root(inode); > if (!root) { > status = -ENOMEM; > - mlog_errno(status); > - goto read_super_error; > + goto out_dismount; > } > > sb->s_root = root; > @@ -1178,17 +1171,21 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > return status; > > -read_super_error: > - brelse(bh); > - > - if (status) > - mlog_errno(status); > +out_dismount: > + atomic_set(&osb->vol_state, VOLUME_DISABLED); > + wake_up(&osb->osb_mount_event); > + ocfs2_dismount_volume(sb, 1); > + goto out; > > - if (osb) { > - atomic_set(&osb->vol_state, VOLUME_DISABLED); > - wake_up(&osb->osb_mount_event); > - ocfs2_dismount_volume(sb, 1); > - } > +out_debugfs: > + debugfs_remove_recursive(osb->osb_debug_root); > +out_super: > + ocfs2_release_system_inodes(osb); > + kfree(osb->recovery_map); > + ocfs2_delete_osb(osb); > + kfree(osb); > +out: > + mlog_errno(status); > > return status; > }
Heming Zhao
2022-Apr-13 13:11 UTC
[Ocfs2-devel] [PATCH v2 5/5] ocfs2: rewrite error handling of ocfs2_fill_super
On Wed, Apr 13, 2022 at 07:37:59PM +0800, Joseph Qi wrote:> It mostly looks good to me. > Have you done error injection test after this patchset applied? > I'm afraid ocfs2-test can't cover this as of now. >I plan to write a debugfs api for error injection test. The code only for these patches test My idea: echo N > /sys/kernel/debug/ocfs2/xxx/error N: 1,2,3,4,... "echo N" will make ocfs2 trigger Nth error point become true: ``` status = ocfs2_initialize_super(sb, bh, sector_size, &stats); brelse(bh); bh = NULL; - if (status < 0) + if (error_inject(n)) //eg. "echo 1 > xx" will trigger there "if()" becomes true. goto out; ``` What's your opinion about my idea? Or do you have other method to test? - Heming> Thanks, > Joseph > > On 4/13/22 4:29 PM, Heming Zhao wrote: > > Current ocfs2_fill_super() uses one goto label "read_super_error" to > > handle all error cases. And with previous serial patches, the error > > handling should fork more branches to handle different error cases. > > This patch rewrite the error handling of ocfs2_fill_super. > > > > Signed-off-by: Heming Zhao <heming.zhao at suse.com> > > --- > > fs/ocfs2/super.c | 67 +++++++++++++++++++++++------------------------- > > 1 file changed, 32 insertions(+), 35 deletions(-) > > > > diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c > > index 5e860d7162d7..72673d40d29c 100644 > > --- a/fs/ocfs2/super.c > > +++ b/fs/ocfs2/super.c > > @@ -989,28 +989,27 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > > > if (!ocfs2_parse_options(sb, data, &parsed_options, 0)) { > > status = -EINVAL; > > - goto read_super_error; > > + goto out; > > } > > > > /* probe for superblock */ > > status = ocfs2_sb_probe(sb, &bh, §or_size, &stats); > > if (status < 0) { > > mlog(ML_ERROR, "superblock probe failed!\n"); > > - goto read_super_error; > > + goto out; > > } > > > > status = ocfs2_initialize_super(sb, bh, sector_size, &stats); > > - osb = OCFS2_SB(sb); > > - if (status < 0) { > > - mlog_errno(status); > > - goto read_super_error; > > - } > > brelse(bh); > > bh = NULL; > > + if (status < 0) > > + goto out; > > + > > + osb = OCFS2_SB(sb); > > > > if (!ocfs2_check_set_options(sb, &parsed_options)) { > > status = -EINVAL; > > - goto read_super_error; > > + goto out_super; > > } > > osb->s_mount_opt = parsed_options.mount_opt; > > osb->s_atime_quantum = parsed_options.atime_quantum; > > @@ -1027,7 +1026,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > > > status = ocfs2_verify_userspace_stack(osb, &parsed_options); > > if (status) > > - goto read_super_error; > > + goto out_super; > > > > sb->s_magic = OCFS2_SUPER_MAGIC; > > > > @@ -1041,7 +1040,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > status = -EACCES; > > mlog(ML_ERROR, "Readonly device detected but readonly " > > "mount was not specified.\n"); > > - goto read_super_error; > > + goto out_super; > > } > > > > /* You should not be able to start a local heartbeat > > @@ -1050,7 +1049,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > status = -EROFS; > > mlog(ML_ERROR, "Local heartbeat specified on readonly " > > "device.\n"); > > - goto read_super_error; > > + goto out_super; > > } > > > > status = ocfs2_check_journals_nolocks(osb); > > @@ -1059,9 +1058,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > mlog(ML_ERROR, "Recovery required on readonly " > > "file system, but write access is " > > "unavailable.\n"); > > - else > > - mlog_errno(status); > > - goto read_super_error; > > + goto out_super; > > } > > > > ocfs2_set_ro_flag(osb, 1); > > @@ -1077,10 +1074,8 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > } > > > > status = ocfs2_verify_heartbeat(osb); > > - if (status < 0) { > > - mlog_errno(status); > > - goto read_super_error; > > - } > > + if (status < 0) > > + goto out_super; > > > > osb->osb_debug_root = debugfs_create_dir(osb->uuid_str, > > ocfs2_debugfs_root); > > @@ -1094,15 +1089,14 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > > > status = ocfs2_mount_volume(sb); > > if (status < 0) > > - goto read_super_error; > > + goto out_debugfs; > > > > if (osb->root_inode) > > inode = igrab(osb->root_inode); > > > > if (!inode) { > > status = -EIO; > > - mlog_errno(status); > > - goto read_super_error; > > + goto out_dismount; > > } > > > > osb->osb_dev_kset = kset_create_and_add(sb->s_id, NULL, > > @@ -1110,7 +1104,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > if (!osb->osb_dev_kset) { > > status = -ENOMEM; > > mlog(ML_ERROR, "Unable to create device kset %s.\n", sb->s_id); > > - goto read_super_error; > > + goto out_dismount; > > } > > > > /* Create filecheck sysfs related directories/files at > > @@ -1119,14 +1113,13 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > status = -ENOMEM; > > mlog(ML_ERROR, "Unable to create filecheck sysfs directory at " > > "/sys/fs/ocfs2/%s/filecheck.\n", sb->s_id); > > - goto read_super_error; > > + goto out_dismount; > > } > > > > root = d_make_root(inode); > > if (!root) { > > status = -ENOMEM; > > - mlog_errno(status); > > - goto read_super_error; > > + goto out_dismount; > > } > > > > sb->s_root = root; > > @@ -1178,17 +1171,21 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > > > return status; > > > > -read_super_error: > > - brelse(bh); > > - > > - if (status) > > - mlog_errno(status); > > +out_dismount: > > + atomic_set(&osb->vol_state, VOLUME_DISABLED); > > + wake_up(&osb->osb_mount_event); > > + ocfs2_dismount_volume(sb, 1); > > + goto out; > > > > - if (osb) { > > - atomic_set(&osb->vol_state, VOLUME_DISABLED); > > - wake_up(&osb->osb_mount_event); > > - ocfs2_dismount_volume(sb, 1); > > - } > > +out_debugfs: > > + debugfs_remove_recursive(osb->osb_debug_root); > > +out_super: > > + ocfs2_release_system_inodes(osb); > > + kfree(osb->recovery_map); > > + ocfs2_delete_osb(osb); > > + kfree(osb); > > +out: > > + mlog_errno(status); > > > > return status; > > } >