* [Ocfs2-devel] [PATCH] ocfs2: fix error handling during mounting stage
@ 2022-04-04 16:40 Heming Zhao via Ocfs2-devel
2022-04-05 10:26 ` heming.zhao--- via Ocfs2-devel
2022-04-06 9:27 ` Joseph Qi via Ocfs2-devel
0 siblings, 2 replies; 4+ messages in thread
From: Heming Zhao via Ocfs2-devel @ 2022-04-04 16:40 UTC (permalink / raw)
To: ocfs2-devel, joseph.qi
This is draft version, It only passed compiling & mount/umount test.
I want to know if my idea is acceptable for maintainers.
There left one issue: mounting crash when no dlm connection. I still
can't find out a better solution without directly free inode memory.
So I marked the related function (ocfs2_release_system_inodes) with
comment: "/* FIXME crash when no journal */"
Signed-off-by: Heming Zhao <heming.zhao@suse.com>
---
fs/ocfs2/reservations.c | 4 +-
fs/ocfs2/reservations.h | 2 +-
fs/ocfs2/super.c | 156 +++++++++++++++++++++++++---------------
3 files changed, 99 insertions(+), 63 deletions(-)
diff --git a/fs/ocfs2/reservations.c b/fs/ocfs2/reservations.c
index 769e466887b0..a9d1296d736d 100644
--- a/fs/ocfs2/reservations.c
+++ b/fs/ocfs2/reservations.c
@@ -198,7 +198,7 @@ void ocfs2_resv_set_type(struct ocfs2_alloc_reservation *resv,
resv->r_flags |= flags;
}
-int ocfs2_resmap_init(struct ocfs2_super *osb,
+void ocfs2_resmap_init(struct ocfs2_super *osb,
struct ocfs2_reservation_map *resmap)
{
memset(resmap, 0, sizeof(*resmap));
@@ -207,8 +207,6 @@ int ocfs2_resmap_init(struct ocfs2_super *osb,
resmap->m_reservations = RB_ROOT;
/* m_bitmap_len is initialized to zero by the above memset. */
INIT_LIST_HEAD(&resmap->m_lru);
-
- return 0;
}
static void ocfs2_resv_mark_lru(struct ocfs2_reservation_map *resmap,
diff --git a/fs/ocfs2/reservations.h b/fs/ocfs2/reservations.h
index 677c50663595..6e5559d6940d 100644
--- a/fs/ocfs2/reservations.h
+++ b/fs/ocfs2/reservations.h
@@ -81,7 +81,7 @@ void ocfs2_resv_discard(struct ocfs2_reservation_map *resmap,
* Only possible return value other than '0' is -ENOMEM for failure to
* allocation mirror bitmap.
*/
-int ocfs2_resmap_init(struct ocfs2_super *osb,
+void ocfs2_resmap_init(struct ocfs2_super *osb,
struct ocfs2_reservation_map *resmap);
/**
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 477cdf94122e..04d7bc7e1110 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -112,6 +112,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
struct buffer_head *bh,
int sector_size,
struct ocfs2_blockcheck_stats *stats);
+static void ocfs2_uninitialize_super(struct ocfs2_super *osb);
static int ocfs2_get_sector(struct super_block *sb,
struct buffer_head **bh,
int block,
@@ -458,6 +459,7 @@ static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb)
continue;
new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
if (!new) {
+ /* FIXME crash when no journal */
ocfs2_release_system_inodes(osb);
status = ocfs2_is_soft_readonly(osb) ? -EROFS : -EINVAL;
mlog_errno(status);
@@ -488,6 +490,7 @@ static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb)
continue;
new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
if (!new) {
+ /* FIXME crash when no journal */
ocfs2_release_system_inodes(osb);
status = ocfs2_is_soft_readonly(osb) ? -EROFS : -EINVAL;
mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
@@ -989,28 +992,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 finally;
}
/* 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 finally;
}
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 finally;
+ }
+ osb = OCFS2_SB(sb);
if (!ocfs2_check_set_options(sb, &parsed_options)) {
status = -EINVAL;
- goto read_super_error;
+ goto super_out;
}
osb->s_mount_opt = parsed_options.mount_opt;
osb->s_atime_quantum = parsed_options.atime_quantum;
@@ -1027,7 +1029,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 super_out;
sb->s_magic = OCFS2_SUPER_MAGIC;
@@ -1041,7 +1043,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 super_out;
}
/* You should not be able to start a local heartbeat
@@ -1050,7 +1052,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 super_out;
}
status = ocfs2_check_journals_nolocks(osb);
@@ -1061,7 +1063,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent)
"unavailable.\n");
else
mlog_errno(status);
- goto read_super_error;
+ goto super_out;
}
ocfs2_set_ro_flag(osb, 1);
@@ -1079,7 +1081,7 @@ 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;
+ goto super_out;
}
osb->osb_debug_root = debugfs_create_dir(osb->uuid_str,
@@ -1094,15 +1096,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 debugfs_out;
if (osb->root_inode)
inode = igrab(osb->root_inode);
if (!inode) {
status = -EIO;
- mlog_errno(status);
- goto read_super_error;
+ goto journal_out;
}
osb->osb_dev_kset = kset_create_and_add(sb->s_id, NULL,
@@ -1110,7 +1111,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 journal_out;
}
/* Create filecheck sysfs related directories/files at
@@ -1119,14 +1120,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 journal_out;
}
root = d_make_root(inode);
if (!root) {
status = -ENOMEM;
- mlog_errno(status);
- goto read_super_error;
+ goto journal_out;
}
sb->s_root = root;
@@ -1178,17 +1178,22 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent)
return status;
-read_super_error:
- brelse(bh);
+journal_out:
+ mlog_errno(status);
+ atomic_set(&osb->vol_state, VOLUME_DISABLED);
+ wake_up(&osb->osb_mount_event);
+ ocfs2_dismount_volume(sb, 1);
- if (status)
- mlog_errno(status);
+ return status;
- if (osb) {
- atomic_set(&osb->vol_state, VOLUME_DISABLED);
- wake_up(&osb->osb_mount_event);
- ocfs2_dismount_volume(sb, 1);
- }
+debugfs_out:
+ debugfs_remove_recursive(osb->osb_debug_root);
+super_out:
+ /* FIXME crash when no journal */
+ ocfs2_release_system_inodes(osb);
+ ocfs2_uninitialize_super(osb);
+finally:
+ mlog_errno(status);
return status;
}
@@ -1803,11 +1808,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 finally;
mutex_init(&osb->obs_trim_fs_mutex);
@@ -1817,44 +1821,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 finally;
}
status = ocfs2_super_lock(osb, 1);
if (status < 0) {
mlog_errno(status);
- goto leave;
+ goto dlm_out;
}
- 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 super_lock;
}
/* load all node-local system inodes */
status = ocfs2_init_local_system_inodes(osb);
if (status < 0) {
mlog_errno(status);
- goto leave;
+ goto super_lock;
}
status = ocfs2_check_volume(osb);
if (status < 0) {
mlog_errno(status);
- goto leave;
+ goto system_inodes;
}
status = ocfs2_truncate_log_init(osb);
- if (status < 0)
+ if (status < 0) {
mlog_errno(status);
+ goto journal_shutdown;
+ }
-leave:
- if (unlock_super)
- ocfs2_super_unlock(osb, 1);
+ ocfs2_super_unlock(osb, 1);
+ return status;
+journal_shutdown:
+ ocfs2_journal_shutdown(osb);
+system_inodes:
+ /* FIXME crash when no journal */
+ ocfs2_release_system_inodes(osb);
+super_lock:
+ ocfs2_super_unlock(osb, 1);
+dlm_out:
+ ocfs2_dlm_shutdown(osb, 0);
+finally:
return status;
}
@@ -2110,17 +2124,13 @@ static int ocfs2_initialize_super(struct super_block *sb,
init_waitqueue_head(&osb->osb_mount_event);
- status = ocfs2_resmap_init(osb, &osb->osb_la_resmap);
- if (status) {
- mlog_errno(status);
- goto bail;
- }
+ ocfs2_resmap_init(osb, &osb->osb_la_resmap);
osb->vol_label = kmalloc(OCFS2_MAX_VOL_LABEL_LEN, GFP_KERNEL);
if (!osb->vol_label) {
mlog(ML_ERROR, "unable to alloc vol label\n");
status = -ENOMEM;
- goto bail;
+ goto recovery_map;
}
osb->slot_recovery_generations =
@@ -2129,7 +2139,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
if (!osb->slot_recovery_generations) {
status = -ENOMEM;
mlog_errno(status);
- goto bail;
+ goto vol_label;
}
init_waitqueue_head(&osb->osb_wipe_event);
@@ -2139,7 +2149,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
if (!osb->osb_orphan_wipes) {
status = -ENOMEM;
mlog_errno(status);
- goto bail;
+ goto slot_recovery_gen;
}
osb->osb_rf_lock_tree = RB_ROOT;
@@ -2155,13 +2165,13 @@ static int ocfs2_initialize_super(struct super_block *sb,
mlog(ML_ERROR, "couldn't mount because of unsupported "
"optional features (%x).\n", i);
status = -EINVAL;
- goto bail;
+ goto orphan_wipes;
}
if (!sb_rdonly(osb->sb) && (i = OCFS2_HAS_RO_COMPAT_FEATURE(osb->sb, ~OCFS2_FEATURE_RO_COMPAT_SUPP))) {
mlog(ML_ERROR, "couldn't mount RDWR because of "
"unsupported optional features (%x).\n", i);
status = -EINVAL;
- goto bail;
+ goto orphan_wipes;
}
if (ocfs2_clusterinfo_valid(osb)) {
@@ -2182,7 +2192,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
"cluster stack label (%s) \n",
osb->osb_cluster_stack);
status = -EINVAL;
- goto bail;
+ goto orphan_wipes;
}
memcpy(osb->osb_cluster_name,
OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster,
@@ -2208,7 +2218,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
mlog(ML_ERROR, "Volume has invalid cluster size (%d)\n",
osb->s_clustersize);
status = -EINVAL;
- goto bail;
+ goto orphan_wipes;
}
total_blocks = ocfs2_clusters_to_blocks(osb->sb,
@@ -2220,14 +2230,14 @@ static int ocfs2_initialize_super(struct super_block *sb,
mlog(ML_ERROR, "Volume too large "
"to mount safely on this system");
status = -EFBIG;
- goto bail;
+ goto orphan_wipes;
}
if (ocfs2_setup_osb_uuid(osb, di->id2.i_super.s_uuid,
sizeof(di->id2.i_super.s_uuid))) {
mlog(ML_ERROR, "Out of memory trying to setup our uuid.\n");
status = -ENOMEM;
- goto bail;
+ goto orphan_wipes;
}
strlcpy(osb->vol_label, di->id2.i_super.s_label,
@@ -2247,7 +2257,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
if (!osb->osb_dlm_debug) {
status = -ENOMEM;
mlog_errno(status);
- goto bail;
+ goto uuid_str;
}
atomic_set(&osb->vol_state, VOLUME_INIT);
@@ -2256,7 +2266,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
status = ocfs2_init_global_system_inodes(osb);
if (status < 0) {
mlog_errno(status);
- goto bail;
+ goto dlm_out;
}
/*
@@ -2267,7 +2277,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
if (!inode) {
status = -EINVAL;
mlog_errno(status);
- goto bail;
+ goto system_inodes;
}
osb->bitmap_blkno = OCFS2_I(inode)->ip_blkno;
@@ -2280,19 +2290,47 @@ static int ocfs2_initialize_super(struct super_block *sb,
status = ocfs2_init_slot_info(osb);
if (status < 0) {
mlog_errno(status);
- goto bail;
+ goto system_inodes;
}
osb->ocfs2_wq = alloc_ordered_workqueue("ocfs2_wq", WQ_MEM_RECLAIM);
if (!osb->ocfs2_wq) {
status = -ENOMEM;
mlog_errno(status);
+ goto slot_info;
}
+ return status;
+
+slot_info:
+ ocfs2_free_slot_info(osb);
+system_inodes:
+ /* FIXME crash when no journal */
+ ocfs2_release_system_inodes(osb);
+dlm_out:
+ ocfs2_put_dlm_debug(osb->osb_dlm_debug);
+uuid_str:
+ kfree(osb->uuid_str);
+orphan_wipes:
+ kfree(osb->osb_orphan_wipes);
+slot_recovery_gen:
+ kfree(osb->slot_recovery_generations);
+vol_label:
+ kfree(osb->vol_label);
+recovery_map:
+ kfree(osb->recovery_map);
bail:
+ kfree(osb);
return status;
}
+static void ocfs2_uninitialize_super(struct ocfs2_super *osb)
+{
+ kfree(osb->recovery_map);
+ ocfs2_delete_osb(osb);
+ kfree(osb);
+}
+
/*
* will return: -EAGAIN if it is ok to keep searching for superblocks
* -EINVAL if there is a bad superblock
--
2.35.1
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: fix error handling during mounting stage
2022-04-04 16:40 [Ocfs2-devel] [PATCH] ocfs2: fix error handling during mounting stage Heming Zhao via Ocfs2-devel
@ 2022-04-05 10:26 ` heming.zhao--- via Ocfs2-devel
2022-04-06 9:27 ` Joseph Qi via Ocfs2-devel
1 sibling, 0 replies; 4+ messages in thread
From: heming.zhao--- via Ocfs2-devel @ 2022-04-05 10:26 UTC (permalink / raw)
To: ocfs2-devel, joseph.qi
On 4/5/22 00:40, Heming Zhao wrote:
> This is draft version, It only passed compiling & mount/umount test.
> I want to know if my idea is acceptable for maintainers.
>
> There left one issue: mounting crash when no dlm connection. I still
> can't find out a better solution without directly free inode memory.
> So I marked the related function (ocfs2_release_system_inodes) with
> comment: "/* FIXME crash when no journal */"
>
> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
> ---
> fs/ocfs2/reservations.c | 4 +-
> fs/ocfs2/reservations.h | 2 +-
> fs/ocfs2/super.c | 156 +++++++++++++++++++++++++---------------
> 3 files changed, 99 insertions(+), 63 deletions(-)
>
For the issue: free inode trigger kernel crash when journal un-init.
There are three solutions:
1> Revert commit da5e7c87827e8
For avoiding kernel crash, this make sense for us. I only concerned whether there
has any non-system inode access before dlm init. And my answer is NO.
all journal
replay/recovery handling happen after dlm & journal init done.
So this method is
not graceful but workable.
2> My v1 & v2 patch, keep da5e7c87827e8, and do some checks (osb->journal) in
ocfs2_clear_inode.
tThe fix code is special for mounting phase, but it will continue working after
mounting phase. In another word, this method adds useless code in normal inode
free flow.
3> My v3 patch, directly free inode in mounting phase, but maintainer didn't like.
If maintainer didn't like solutions <1> & <3>, Do we choose solution <1>?
At last, all above three solutons had been test (without crash) based on my latest
patch.
Thanks,
Heming
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: fix error handling during mounting stage
2022-04-04 16:40 [Ocfs2-devel] [PATCH] ocfs2: fix error handling during mounting stage Heming Zhao via Ocfs2-devel
2022-04-05 10:26 ` heming.zhao--- via Ocfs2-devel
@ 2022-04-06 9:27 ` Joseph Qi via Ocfs2-devel
2022-04-06 15:14 ` heming.zhao--- via Ocfs2-devel
1 sibling, 1 reply; 4+ messages in thread
From: Joseph Qi via Ocfs2-devel @ 2022-04-06 9:27 UTC (permalink / raw)
To: Heming Zhao, ocfs2-devel
On 4/5/22 12:40 AM, Heming Zhao wrote:
> This is draft version, It only passed compiling & mount/umount test.
> I want to know if my idea is acceptable for maintainers.
>
> There left one issue: mounting crash when no dlm connection. I still
> can't find out a better solution without directly free inode memory.
> So I marked the related function (ocfs2_release_system_inodes) with
> comment: "/* FIXME crash when no journal */"
>
> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
> ---
> fs/ocfs2/reservations.c | 4 +-
> fs/ocfs2/reservations.h | 2 +-
> fs/ocfs2/super.c | 156 +++++++++++++++++++++++++---------------
> 3 files changed, 99 insertions(+), 63 deletions(-)
>
> diff --git a/fs/ocfs2/reservations.c b/fs/ocfs2/reservations.c
> index 769e466887b0..a9d1296d736d 100644
> --- a/fs/ocfs2/reservations.c
> +++ b/fs/ocfs2/reservations.c
> @@ -198,7 +198,7 @@ void ocfs2_resv_set_type(struct ocfs2_alloc_reservation *resv,
> resv->r_flags |= flags;
> }
>
> -int ocfs2_resmap_init(struct ocfs2_super *osb,
> +void ocfs2_resmap_init(struct ocfs2_super *osb,
> struct ocfs2_reservation_map *resmap)
> {
> memset(resmap, 0, sizeof(*resmap));
> @@ -207,8 +207,6 @@ int ocfs2_resmap_init(struct ocfs2_super *osb,
> resmap->m_reservations = RB_ROOT;
> /* m_bitmap_len is initialized to zero by the above memset. */
> INIT_LIST_HEAD(&resmap->m_lru);
> -
> - return 0;
> }
>
> static void ocfs2_resv_mark_lru(struct ocfs2_reservation_map *resmap,
> diff --git a/fs/ocfs2/reservations.h b/fs/ocfs2/reservations.h
> index 677c50663595..6e5559d6940d 100644
> --- a/fs/ocfs2/reservations.h
> +++ b/fs/ocfs2/reservations.h
> @@ -81,7 +81,7 @@ void ocfs2_resv_discard(struct ocfs2_reservation_map *resmap,
> * Only possible return value other than '0' is -ENOMEM for failure to
> * allocation mirror bitmap.
> */
> -int ocfs2_resmap_init(struct ocfs2_super *osb,
> +void ocfs2_resmap_init(struct ocfs2_super *osb,
> struct ocfs2_reservation_map *resmap);
>
> /**
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index 477cdf94122e..04d7bc7e1110 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -112,6 +112,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
> struct buffer_head *bh,
> int sector_size,
> struct ocfs2_blockcheck_stats *stats);
> +static void ocfs2_uninitialize_super(struct ocfs2_super *osb);
> static int ocfs2_get_sector(struct super_block *sb,
> struct buffer_head **bh,
> int block,
> @@ -458,6 +459,7 @@ static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb)
> continue;
> new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
> if (!new) {
> + /* FIXME crash when no journal */
I don't have better idea at moment, so we may factor out part of journal init
and put bofore.
> ocfs2_release_system_inodes(osb);
> status = ocfs2_is_soft_readonly(osb) ? -EROFS : -EINVAL;
> mlog_errno(status);
> @@ -488,6 +490,7 @@ static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb)
> continue;
> new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
> if (!new) {
> + /* FIXME crash when no journal */
> ocfs2_release_system_inodes(osb);
> status = ocfs2_is_soft_readonly(osb) ? -EROFS : -EINVAL;
> mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
> @@ -989,28 +992,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 finally;
> }
>
> /* 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 finally;
> }
>
> 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 finally;
> + }
> + osb = OCFS2_SB(sb);
>
> if (!ocfs2_check_set_options(sb, &parsed_options)) {
> status = -EINVAL;
> - goto read_super_error;
> + goto super_out;
> }
> osb->s_mount_opt = parsed_options.mount_opt;
> osb->s_atime_quantum = parsed_options.atime_quantum;
> @@ -1027,7 +1029,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 super_out;
>
> sb->s_magic = OCFS2_SUPER_MAGIC;
>
> @@ -1041,7 +1043,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 super_out;
> }
>
> /* You should not be able to start a local heartbeat
> @@ -1050,7 +1052,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 super_out;
> }
>
> status = ocfs2_check_journals_nolocks(osb);
> @@ -1061,7 +1063,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent)
> "unavailable.\n");
> else
> mlog_errno(status);
> - goto read_super_error;
> + goto super_out;
> }
>
> ocfs2_set_ro_flag(osb, 1);
> @@ -1079,7 +1081,7 @@ 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;
> + goto super_out;
> }
>
> osb->osb_debug_root = debugfs_create_dir(osb->uuid_str,
> @@ -1094,15 +1096,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 debugfs_out;
>
> if (osb->root_inode)
> inode = igrab(osb->root_inode);
>
> if (!inode) {
> status = -EIO;
> - mlog_errno(status);
> - goto read_super_error;
> + goto journal_out;
> }
>
> osb->osb_dev_kset = kset_create_and_add(sb->s_id, NULL,
> @@ -1110,7 +1111,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 journal_out;
> }
>
> /* Create filecheck sysfs related directories/files at
> @@ -1119,14 +1120,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 journal_out;
> }
>
> root = d_make_root(inode);
> if (!root) {
> status = -ENOMEM;
> - mlog_errno(status);
> - goto read_super_error;
> + goto journal_out;
> }
>
> sb->s_root = root;
> @@ -1178,17 +1178,22 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent)
>
> return status;
>
> -read_super_error:
> - brelse(bh);
> +journal_out:
> + mlog_errno(status);
> + atomic_set(&osb->vol_state, VOLUME_DISABLED);
> + wake_up(&osb->osb_mount_event);
> + ocfs2_dismount_volume(sb, 1);
>
> - if (status)
> - mlog_errno(status);
> + return status;
>
> - if (osb) {
> - atomic_set(&osb->vol_state, VOLUME_DISABLED);
> - wake_up(&osb->osb_mount_event);
> - ocfs2_dismount_volume(sb, 1);
> - }
> +debugfs_out:
> + debugfs_remove_recursive(osb->osb_debug_root);
> +super_out:
> + /* FIXME crash when no journal */
> + ocfs2_release_system_inodes(osb);
> + ocfs2_uninitialize_super(osb);
> +finally:
Missing brelse(bh) since it will goto here if ocfs2_sb_probe()
fails.
Suggest we name the label as out_xxx, and I have to take more
time to review the above changes.
> + mlog_errno(status);
>
> return status;
> }
> @@ -1803,11 +1808,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 finally;
>
> mutex_init(&osb->obs_trim_fs_mutex);
>
> @@ -1817,44 +1821,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 finally;
> }
>
> status = ocfs2_super_lock(osb, 1);
> if (status < 0) {
> mlog_errno(status);
> - goto leave;
> + goto dlm_out;
> }
> - 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 super_lock;
> }
>
> /* load all node-local system inodes */
> status = ocfs2_init_local_system_inodes(osb);
> if (status < 0) {
> mlog_errno(status);
> - goto leave;
> + goto super_lock;
> }
>
> status = ocfs2_check_volume(osb);
> if (status < 0) {
> mlog_errno(status);
> - goto leave;
> + goto system_inodes;
> }
>
> status = ocfs2_truncate_log_init(osb);
> - if (status < 0)
> + if (status < 0) {
> mlog_errno(status);
> + goto journal_shutdown;
> + }
>
> -leave:
> - if (unlock_super)
> - ocfs2_super_unlock(osb, 1);
> + ocfs2_super_unlock(osb, 1);
> + return status;
>
> +journal_shutdown:
> + ocfs2_journal_shutdown(osb);
> +system_inodes:
> + /* FIXME crash when no journal */
> + ocfs2_release_system_inodes(osb);
> +super_lock:
> + ocfs2_super_unlock(osb, 1);
> +dlm_out:
> + ocfs2_dlm_shutdown(osb, 0);
> +finally:
> return status;
> }
>
> @@ -2110,17 +2124,13 @@ static int ocfs2_initialize_super(struct super_block *sb,
>
> init_waitqueue_head(&osb->osb_mount_event);
>
> - status = ocfs2_resmap_init(osb, &osb->osb_la_resmap);
> - if (status) {
> - mlog_errno(status);
> - goto bail;
> - }
> + ocfs2_resmap_init(osb, &osb->osb_la_resmap);
>
> osb->vol_label = kmalloc(OCFS2_MAX_VOL_LABEL_LEN, GFP_KERNEL);
> if (!osb->vol_label) {
> mlog(ML_ERROR, "unable to alloc vol label\n");
> status = -ENOMEM;
> - goto bail;
> + goto recovery_map;
> }
>
> osb->slot_recovery_generations =
> @@ -2129,7 +2139,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
> if (!osb->slot_recovery_generations) {
> status = -ENOMEM;
> mlog_errno(status);
> - goto bail;
> + goto vol_label;
> }
>
> init_waitqueue_head(&osb->osb_wipe_event);
> @@ -2139,7 +2149,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
> if (!osb->osb_orphan_wipes) {
> status = -ENOMEM;
> mlog_errno(status);
> - goto bail;
> + goto slot_recovery_gen;
> }
>
> osb->osb_rf_lock_tree = RB_ROOT;
> @@ -2155,13 +2165,13 @@ static int ocfs2_initialize_super(struct super_block *sb,
> mlog(ML_ERROR, "couldn't mount because of unsupported "
> "optional features (%x).\n", i);
> status = -EINVAL;
> - goto bail;
> + goto orphan_wipes;
> }
> if (!sb_rdonly(osb->sb) && (i = OCFS2_HAS_RO_COMPAT_FEATURE(osb->sb, ~OCFS2_FEATURE_RO_COMPAT_SUPP))) {
> mlog(ML_ERROR, "couldn't mount RDWR because of "
> "unsupported optional features (%x).\n", i);
> status = -EINVAL;
> - goto bail;
> + goto orphan_wipes;
> }
>
> if (ocfs2_clusterinfo_valid(osb)) {
> @@ -2182,7 +2192,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
> "cluster stack label (%s) \n",
> osb->osb_cluster_stack);
> status = -EINVAL;
> - goto bail;
> + goto orphan_wipes;
> }
> memcpy(osb->osb_cluster_name,
> OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster,
> @@ -2208,7 +2218,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
> mlog(ML_ERROR, "Volume has invalid cluster size (%d)\n",
> osb->s_clustersize);
> status = -EINVAL;
> - goto bail;
> + goto orphan_wipes;
> }
>
> total_blocks = ocfs2_clusters_to_blocks(osb->sb,
> @@ -2220,14 +2230,14 @@ static int ocfs2_initialize_super(struct super_block *sb,
> mlog(ML_ERROR, "Volume too large "
> "to mount safely on this system");
> status = -EFBIG;
> - goto bail;
> + goto orphan_wipes;
> }
>
> if (ocfs2_setup_osb_uuid(osb, di->id2.i_super.s_uuid,
> sizeof(di->id2.i_super.s_uuid))) {
> mlog(ML_ERROR, "Out of memory trying to setup our uuid.\n");
> status = -ENOMEM;
> - goto bail;
> + goto orphan_wipes;
> }
>
> strlcpy(osb->vol_label, di->id2.i_super.s_label,
> @@ -2247,7 +2257,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
> if (!osb->osb_dlm_debug) {
> status = -ENOMEM;
> mlog_errno(status);
> - goto bail;
> + goto uuid_str;
> }
>
> atomic_set(&osb->vol_state, VOLUME_INIT);
> @@ -2256,7 +2266,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
> status = ocfs2_init_global_system_inodes(osb);
> if (status < 0) {
> mlog_errno(status);
> - goto bail;
> + goto dlm_out;
> }
>
> /*
> @@ -2267,7 +2277,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
> if (!inode) {
> status = -EINVAL;
> mlog_errno(status);
> - goto bail;
> + goto system_inodes;
> }
>
> osb->bitmap_blkno = OCFS2_I(inode)->ip_blkno;
> @@ -2280,19 +2290,47 @@ static int ocfs2_initialize_super(struct super_block *sb,
> status = ocfs2_init_slot_info(osb);
> if (status < 0) {
> mlog_errno(status);
> - goto bail;
> + goto system_inodes;
> }
>
> osb->ocfs2_wq = alloc_ordered_workqueue("ocfs2_wq", WQ_MEM_RECLAIM);
> if (!osb->ocfs2_wq) {
> status = -ENOMEM;
> mlog_errno(status);
> + goto slot_info;
> }
>
> + return status;
> +
> +slot_info:
> + ocfs2_free_slot_info(osb);
> +system_inodes:
> + /* FIXME crash when no journal */
> + ocfs2_release_system_inodes(osb);
> +dlm_out:
> + ocfs2_put_dlm_debug(osb->osb_dlm_debug);
> +uuid_str:
> + kfree(osb->uuid_str);
> +orphan_wipes:
> + kfree(osb->osb_orphan_wipes);
> +slot_recovery_gen:
> + kfree(osb->slot_recovery_generations);
> +vol_label:
> + kfree(osb->vol_label);
> +recovery_map:
> + kfree(osb->recovery_map);
> bail:
> + kfree(osb);
> return status;
> }
Could we split ocfs2_initialize_super() changes as a single patch?
Also suggest we name the label as out_xxx.
>
> +static void ocfs2_uninitialize_super(struct ocfs2_super *osb)
> +{
> + kfree(osb->recovery_map);
> + ocfs2_delete_osb(osb);
> + kfree(osb);
> +}
Since ocfs2_initialize_super() does more than above, I don't think
the name is quite suitable here.
If we the above operations are enough, I suggest just open code in
error handling.
Thanks,
Joseph
> +
> /*
> * will return: -EAGAIN if it is ok to keep searching for superblocks
> * -EINVAL if there is a bad superblock
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: fix error handling during mounting stage
2022-04-06 9:27 ` Joseph Qi via Ocfs2-devel
@ 2022-04-06 15:14 ` heming.zhao--- via Ocfs2-devel
0 siblings, 0 replies; 4+ messages in thread
From: heming.zhao--- via Ocfs2-devel @ 2022-04-06 15:14 UTC (permalink / raw)
To: Joseph Qi, ocfs2-devel
On 4/6/22 17:27, Joseph Qi wrote:
>
> On 4/5/22 12:40 AM, Heming Zhao wrote:
>> This is draft version, It only passed compiling & mount/umount test.
>> I want to know if my idea is acceptable for maintainers.
>>
>> There left one issue: mounting crash when no dlm connection. I still
>> can't find out a better solution without directly free inode memory.
>> So I marked the related function (ocfs2_release_system_inodes) with
>> comment: "/* FIXME crash when no journal */"
>>
>> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
>> ---
>> fs/ocfs2/reservations.c | 4 +-
>> fs/ocfs2/reservations.h | 2 +-
>> fs/ocfs2/super.c | 156 +++++++++++++++++++++++++---------------
>> 3 files changed, 99 insertions(+), 63 deletions(-)
>>
>> diff --git a/fs/ocfs2/reservations.c b/fs/ocfs2/reservations.c
>> index 769e466887b0..a9d1296d736d 100644
>> --- a/fs/ocfs2/reservations.c
>> +++ b/fs/ocfs2/reservations.c
>> @@ -198,7 +198,7 @@ void ocfs2_resv_set_type(struct ocfs2_alloc_reservation *resv,
>> resv->r_flags |= flags;
>> }
>>
>> -int ocfs2_resmap_init(struct ocfs2_super *osb,
>> +void ocfs2_resmap_init(struct ocfs2_super *osb,
>> struct ocfs2_reservation_map *resmap)
>> {
>> memset(resmap, 0, sizeof(*resmap));
>> @@ -207,8 +207,6 @@ int ocfs2_resmap_init(struct ocfs2_super *osb,
>> resmap->m_reservations = RB_ROOT;
>> /* m_bitmap_len is initialized to zero by the above memset. */
>> INIT_LIST_HEAD(&resmap->m_lru);
>> -
>> - return 0;
>> }
>>
>> static void ocfs2_resv_mark_lru(struct ocfs2_reservation_map *resmap,
>> diff --git a/fs/ocfs2/reservations.h b/fs/ocfs2/reservations.h
>> index 677c50663595..6e5559d6940d 100644
>> --- a/fs/ocfs2/reservations.h
>> +++ b/fs/ocfs2/reservations.h
>> @@ -81,7 +81,7 @@ void ocfs2_resv_discard(struct ocfs2_reservation_map *resmap,
>> * Only possible return value other than '0' is -ENOMEM for failure to
>> * allocation mirror bitmap.
>> */
>> -int ocfs2_resmap_init(struct ocfs2_super *osb,
>> +void ocfs2_resmap_init(struct ocfs2_super *osb,
>> struct ocfs2_reservation_map *resmap);
>>
>> /**
>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>> index 477cdf94122e..04d7bc7e1110 100644
>> --- a/fs/ocfs2/super.c
>> +++ b/fs/ocfs2/super.c
>> @@ -112,6 +112,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> struct buffer_head *bh,
>> int sector_size,
>> struct ocfs2_blockcheck_stats *stats);
>> +static void ocfs2_uninitialize_super(struct ocfs2_super *osb);
>> static int ocfs2_get_sector(struct super_block *sb,
>> struct buffer_head **bh,
>> int block,
>> @@ -458,6 +459,7 @@ static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb)
>> continue;
>> new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
>> if (!new) {
>> + /* FIXME crash when no journal */
>
> I don't have better idea at moment, so we may factor out part of journal init
> and put bofore.
I will do the revert commit da5e7c87827e8 job in next patch verion (v1).
>
>> ocfs2_release_system_inodes(osb);
>> status = ocfs2_is_soft_readonly(osb) ? -EROFS : -EINVAL;
>> mlog_errno(status);
>> @@ -488,6 +490,7 @@ static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb)
>> continue;
>> new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
>> if (!new) {
>> + /* FIXME crash when no journal */
>> ocfs2_release_system_inodes(osb);
>> status = ocfs2_is_soft_readonly(osb) ? -EROFS : -EINVAL;
>> mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
>> @@ -989,28 +992,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 finally;
>> }
>>
>> /* 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 finally;
>> }
>>
>> 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 finally;
>> + }
>> + osb = OCFS2_SB(sb);
please note I move "osb = xx" & "if (xx)" after brelse(bh);
So bh don't need to free in goto label place.
>>
>> if (!ocfs2_check_set_options(sb, &parsed_options)) {
>> status = -EINVAL;
>> - goto read_super_error;
>> + goto super_out;
>> }
>> osb->s_mount_opt = parsed_options.mount_opt;
>> osb->s_atime_quantum = parsed_options.atime_quantum;
>> @@ -1027,7 +1029,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 super_out;
>>
>> sb->s_magic = OCFS2_SUPER_MAGIC;
>>
>> @@ -1041,7 +1043,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 super_out;
>> }
>>
>> /* You should not be able to start a local heartbeat
>> @@ -1050,7 +1052,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 super_out;
>> }
>>
>> status = ocfs2_check_journals_nolocks(osb);
>> @@ -1061,7 +1063,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent)
>> "unavailable.\n");
>> else
>> mlog_errno(status);
>> - goto read_super_error;
>> + goto super_out;
>> }
>>
>> ocfs2_set_ro_flag(osb, 1);
>> @@ -1079,7 +1081,7 @@ 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;
>> + goto super_out;
>> }
>>
>> osb->osb_debug_root = debugfs_create_dir(osb->uuid_str,
>> @@ -1094,15 +1096,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 debugfs_out;
>>
>> if (osb->root_inode)
>> inode = igrab(osb->root_inode);
>>
>> if (!inode) {
>> status = -EIO;
>> - mlog_errno(status);
>> - goto read_super_error;
>> + goto journal_out;
>> }
>>
>> osb->osb_dev_kset = kset_create_and_add(sb->s_id, NULL,
>> @@ -1110,7 +1111,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 journal_out;
>> }
>>
>> /* Create filecheck sysfs related directories/files at
>> @@ -1119,14 +1120,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 journal_out;
>> }
>>
>> root = d_make_root(inode);
>> if (!root) {
>> status = -ENOMEM;
>> - mlog_errno(status);
>> - goto read_super_error;
>> + goto journal_out;
>> }
>>
>> sb->s_root = root;
>> @@ -1178,17 +1178,22 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent)
>>
>> return status;
>>
>> -read_super_error:
>> - brelse(bh);
>> +journal_out:
>> + mlog_errno(status);
>> + atomic_set(&osb->vol_state, VOLUME_DISABLED);
>> + wake_up(&osb->osb_mount_event);
>> + ocfs2_dismount_volume(sb, 1);
>>
>> - if (status)
>> - mlog_errno(status);
>> + return status;
>>
>> - if (osb) {
>> - atomic_set(&osb->vol_state, VOLUME_DISABLED);
>> - wake_up(&osb->osb_mount_event);
>> - ocfs2_dismount_volume(sb, 1);
>> - }
>> +debugfs_out:
>> + debugfs_remove_recursive(osb->osb_debug_root);
>> +super_out:
>> + /* FIXME crash when no journal */
>> + ocfs2_release_system_inodes(osb);
>> + ocfs2_uninitialize_super(osb);
>> +finally:
>
> Missing brelse(bh) since it will goto here if ocfs2_sb_probe()
> fails.
No. please also check my previous comment. if ocfs2_sb_probe() fails, bh is
released before return. and in normal code flow, bh will be released after
ocfs2_initialize_super() return. So no need to call brelse(bh) in error
handling place.
> Suggest we name the label as out_xxx, and I have to take more
> time to review the above changes.
OK, will change in next patch version (v1).
>
>> + mlog_errno(status);
>>
>> return status;
>> }
>> @@ -1803,11 +1808,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 finally;
>>
>> mutex_init(&osb->obs_trim_fs_mutex);
>>
>> @@ -1817,44 +1821,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 finally;
>> }
>>
>> status = ocfs2_super_lock(osb, 1);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto leave;
>> + goto dlm_out;
>> }
>> - 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 super_lock;
>> }
>>
>> /* load all node-local system inodes */
>> status = ocfs2_init_local_system_inodes(osb);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto leave;
>> + goto super_lock;
>> }
>>
>> status = ocfs2_check_volume(osb);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto leave;
>> + goto system_inodes;
>> }
>>
>> status = ocfs2_truncate_log_init(osb);
>> - if (status < 0)
>> + if (status < 0) {
>> mlog_errno(status);
>> + goto journal_shutdown;
>> + }
>>
>> -leave:
>> - if (unlock_super)
>> - ocfs2_super_unlock(osb, 1);
>> + ocfs2_super_unlock(osb, 1);
>> + return status;
>>
>> +journal_shutdown:
>> + ocfs2_journal_shutdown(osb);
>> +system_inodes:
>> + /* FIXME crash when no journal */
>> + ocfs2_release_system_inodes(osb);
>> +super_lock:
>> + ocfs2_super_unlock(osb, 1);
>> +dlm_out:
>> + ocfs2_dlm_shutdown(osb, 0);
>> +finally:
>> return status;
>> }
>>
>> @@ -2110,17 +2124,13 @@ static int ocfs2_initialize_super(struct super_block *sb,
>>
>> init_waitqueue_head(&osb->osb_mount_event);
>>
>> - status = ocfs2_resmap_init(osb, &osb->osb_la_resmap);
>> - if (status) {
>> - mlog_errno(status);
>> - goto bail;
>> - }
>> + ocfs2_resmap_init(osb, &osb->osb_la_resmap);
>>
>> osb->vol_label = kmalloc(OCFS2_MAX_VOL_LABEL_LEN, GFP_KERNEL);
>> if (!osb->vol_label) {
>> mlog(ML_ERROR, "unable to alloc vol label\n");
>> status = -ENOMEM;
>> - goto bail;
>> + goto recovery_map;
>> }
>>
>> osb->slot_recovery_generations =
>> @@ -2129,7 +2139,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> if (!osb->slot_recovery_generations) {
>> status = -ENOMEM;
>> mlog_errno(status);
>> - goto bail;
>> + goto vol_label;
>> }
>>
>> init_waitqueue_head(&osb->osb_wipe_event);
>> @@ -2139,7 +2149,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> if (!osb->osb_orphan_wipes) {
>> status = -ENOMEM;
>> mlog_errno(status);
>> - goto bail;
>> + goto slot_recovery_gen;
>> }
>>
>> osb->osb_rf_lock_tree = RB_ROOT;
>> @@ -2155,13 +2165,13 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> mlog(ML_ERROR, "couldn't mount because of unsupported "
>> "optional features (%x).\n", i);
>> status = -EINVAL;
>> - goto bail;
>> + goto orphan_wipes;
>> }
>> if (!sb_rdonly(osb->sb) && (i = OCFS2_HAS_RO_COMPAT_FEATURE(osb->sb, ~OCFS2_FEATURE_RO_COMPAT_SUPP))) {
>> mlog(ML_ERROR, "couldn't mount RDWR because of "
>> "unsupported optional features (%x).\n", i);
>> status = -EINVAL;
>> - goto bail;
>> + goto orphan_wipes;
>> }
>>
>> if (ocfs2_clusterinfo_valid(osb)) {
>> @@ -2182,7 +2192,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> "cluster stack label (%s) \n",
>> osb->osb_cluster_stack);
>> status = -EINVAL;
>> - goto bail;
>> + goto orphan_wipes;
>> }
>> memcpy(osb->osb_cluster_name,
>> OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster,
>> @@ -2208,7 +2218,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> mlog(ML_ERROR, "Volume has invalid cluster size (%d)\n",
>> osb->s_clustersize);
>> status = -EINVAL;
>> - goto bail;
>> + goto orphan_wipes;
>> }
>>
>> total_blocks = ocfs2_clusters_to_blocks(osb->sb,
>> @@ -2220,14 +2230,14 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> mlog(ML_ERROR, "Volume too large "
>> "to mount safely on this system");
>> status = -EFBIG;
>> - goto bail;
>> + goto orphan_wipes;
>> }
>>
>> if (ocfs2_setup_osb_uuid(osb, di->id2.i_super.s_uuid,
>> sizeof(di->id2.i_super.s_uuid))) {
>> mlog(ML_ERROR, "Out of memory trying to setup our uuid.\n");
>> status = -ENOMEM;
>> - goto bail;
>> + goto orphan_wipes;
>> }
>>
>> strlcpy(osb->vol_label, di->id2.i_super.s_label,
>> @@ -2247,7 +2257,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> if (!osb->osb_dlm_debug) {
>> status = -ENOMEM;
>> mlog_errno(status);
>> - goto bail;
>> + goto uuid_str;
>> }
>>
>> atomic_set(&osb->vol_state, VOLUME_INIT);
>> @@ -2256,7 +2266,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> status = ocfs2_init_global_system_inodes(osb);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto bail;
>> + goto dlm_out;
>> }
>>
>> /*
>> @@ -2267,7 +2277,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> if (!inode) {
>> status = -EINVAL;
>> mlog_errno(status);
>> - goto bail;
>> + goto system_inodes;
>> }
>>
>> osb->bitmap_blkno = OCFS2_I(inode)->ip_blkno;
>> @@ -2280,19 +2290,47 @@ static int ocfs2_initialize_super(struct super_block *sb,
>> status = ocfs2_init_slot_info(osb);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto bail;
>> + goto system_inodes;
>> }
>>
>> osb->ocfs2_wq = alloc_ordered_workqueue("ocfs2_wq", WQ_MEM_RECLAIM);
>> if (!osb->ocfs2_wq) {
>> status = -ENOMEM;
>> mlog_errno(status);
>> + goto slot_info;
>> }
>>
>> + return status;
>> +
>> +slot_info:
>> + ocfs2_free_slot_info(osb);
>> +system_inodes:
>> + /* FIXME crash when no journal */
>> + ocfs2_release_system_inodes(osb);
>> +dlm_out:
>> + ocfs2_put_dlm_debug(osb->osb_dlm_debug);
>> +uuid_str:
>> + kfree(osb->uuid_str);
>> +orphan_wipes:
>> + kfree(osb->osb_orphan_wipes);
>> +slot_recovery_gen:
>> + kfree(osb->slot_recovery_generations);
>> +vol_label:
>> + kfree(osb->vol_label);
>> +recovery_map:
>> + kfree(osb->recovery_map);
>> bail:
>> + kfree(osb);
>> return status;
>> }
> Could we split ocfs2_initialize_super() changes as a single patch?
> Also suggest we name the label as out_xxx.
>
NO problem.
>>
>> +static void ocfs2_uninitialize_super(struct ocfs2_super *osb)
>> +{
>> + kfree(osb->recovery_map);
>> + ocfs2_delete_osb(osb);
>> + kfree(osb);
>> +}
>
> Since ocfs2_initialize_super() does more than above, I don't think
> the name is quite suitable here.
> If we the above operations are enough, I suggest just open code in
> error handling.
>
No problem, will change to directly call these three line codes.
I checked many times and thought they're enough.
Thank you for your kindly review. please wait for patch v1.
- Heming
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-06 15:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-04 16:40 [Ocfs2-devel] [PATCH] ocfs2: fix error handling during mounting stage Heming Zhao via Ocfs2-devel
2022-04-05 10:26 ` heming.zhao--- via Ocfs2-devel
2022-04-06 9:27 ` Joseph Qi via Ocfs2-devel
2022-04-06 15:14 ` heming.zhao--- via Ocfs2-devel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.