diff mbox series

ocfs2: fix error handling during mounting stage

Message ID 20220404164055.1434-1-heming.zhao@suse.com (mailing list archive)
State New
Headers show
Series ocfs2: fix error handling during mounting stage | expand

Commit Message

Heming Zhao April 4, 2022, 4:40 p.m. UTC
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(-)

Comments

heming.zhao--- via Ocfs2-devel April 5, 2022, 10:26 a.m. UTC | #1
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
Joseph Qi April 6, 2022, 9:27 a.m. UTC | #2
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, &sector_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
heming.zhao--- via Ocfs2-devel April 6, 2022, 3:14 p.m. UTC | #3
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, &sector_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
diff mbox series

Patch

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, &sector_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