[Ocfs2-devel] [patch 15/25] ocfs2: extend transaction for ocfs2_remove_rightmost_path() and ocfs2_update_edge_lengths() before to avoid inconsistency between inode and et

[Joseph Qi <joseph.qi@huawei.com>]

On 2016/3/24 4:12, akpm at linux-foundation.org wrote:
> From: Xue jiufei <xuejiufei@huawei.com>
> Subject: ocfs2: extend transaction for ocfs2_remove_rightmost_path() and ocfs2_update_edge_lengths() before to avoid inconsistency between inode and et
> 
> I found that jbd2_journal_restart() is called in some places without
> keeping things consistently before.  However, jbd2_journal_restart() may
> commit the handle's transaction and restart another one.  If the first
> transaction is committed successfully while another not, it may cause
> filesystem inconsistency or read only.  This is an effort to fix this kind
> of problems.
> 
> 
> This patch (of 3):
> 
> The following functions will be called while truncating an extent:
> ocfs2_remove_btree_range
>   -> ocfs2_start_trans
>   -> ocfs2_remove_extent
>      -> ocfs2_truncate_rec
>        -> ocfs2_extend_rotate_transaction
>          -> jbd2_journal_restart if jbd2_journal_extend fail
>        -> ocfs2_rotate_tree_left
>          -> ocfs2_remove_rightmost_path
>              -> ocfs2_extend_rotate_transaction
>                -> ocfs2_unlink_subtree
>                 -> ocfs2_update_edge_lengths
>                   -> ocfs2_extend_trans
>                     -> jbd2_journal_restart if jbd2_journal_extend fail
>   -> ocfs2_et_update_clusters
>   -> ocfs2_commit_trans
> 
> jbd2_journal_restart() may be called and it may happened that the buffers
> dirtied in ocfs2_truncate_rec() are committed while buffers dirtied in
> ocfs2_et_update_clusters() are not, the total clusters on extent tree and
> i_clusters in ocfs2_dinode is inconsistency.  So the clusters got from
> ocfs2_dinode is incorrect, and it also cause read-only problem when call
> ocfs2_commit_truncate() with the error message: "Inode %llu has empty
> extent block at %llu".
> 
> We should extend enough credits for function ocfs2_remove_rightmost_path
> and ocfs2_update_edge_lengths to avoid this inconsistency.
> 
> Signed-off-by: joyce.xue <xuejiufei@huawei.com>
Acked-by: Joseph Qi <joseph.qi@huawei.com>

> Cc: Mark Fasheh <mfasheh@suse.com>
> Cc: Joel Becker <jlbec@evilplan.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  fs/ocfs2/alloc.c |   82 +++++++++++++++++++++++++++++----------------
>  1 file changed, 54 insertions(+), 28 deletions(-)
> 
> diff -puN fs/ocfs2/alloc.c~ocfs2-extend-transaction-for-ocfs2_remove_rightmost_path-and-ocfs2_update_edge_lengths-before-to-avoid-inconsistency-between-inode-and-et fs/ocfs2/alloc.c
> --- a/fs/ocfs2/alloc.c~ocfs2-extend-transaction-for-ocfs2_remove_rightmost_path-and-ocfs2_update_edge_lengths-before-to-avoid-inconsistency-between-inode-and-et
> +++ a/fs/ocfs2/alloc.c
> @@ -2516,21 +2516,6 @@ static int ocfs2_update_edge_lengths(han
>  	struct ocfs2_extent_block *eb;
>  	u32 range;
>  
> -	/*
> -	 * In normal tree rotation process, we will never touch the
> -	 * tree branch above subtree_index and ocfs2_extend_rotate_transaction
> -	 * doesn't reserve the credits for them either.
> -	 *
> -	 * But we do have a special case here which will update the rightmost
> -	 * records for all the bh in the path.
> -	 * So we have to allocate extra credits and access them.
> -	 */
> -	ret = ocfs2_extend_trans(handle, subtree_index);
> -	if (ret) {
> -		mlog_errno(ret);
> -		goto out;
> -	}
> -
>  	ret = ocfs2_journal_access_path(et->et_ci, handle, path);
>  	if (ret) {
>  		mlog_errno(ret);
> @@ -2956,7 +2941,7 @@ static int __ocfs2_rotate_tree_left(hand
>  		     right_path->p_node[subtree_root].bh->b_blocknr,
>  		     right_path->p_tree_depth);
>  
> -		ret = ocfs2_extend_rotate_transaction(handle, subtree_root,
> +		ret = ocfs2_extend_rotate_transaction(handle, 0,
>  						      orig_credits, left_path);
>  		if (ret) {
>  			mlog_errno(ret);
> @@ -3029,21 +3014,9 @@ static int ocfs2_remove_rightmost_path(h
>  	struct ocfs2_extent_block *eb;
>  	struct ocfs2_extent_list *el;
>  
> -
>  	ret = ocfs2_et_sanity_check(et);
>  	if (ret)
>  		goto out;
> -	/*
> -	 * There's two ways we handle this depending on
> -	 * whether path is the only existing one.
> -	 */
> -	ret = ocfs2_extend_rotate_transaction(handle, 0,
> -					      handle->h_buffer_credits,
> -					      path);
> -	if (ret) {
> -		mlog_errno(ret);
> -		goto out;
> -	}
>  
>  	ret = ocfs2_journal_access_path(et->et_ci, handle, path);
>  	if (ret) {
> @@ -3641,6 +3614,14 @@ static int ocfs2_merge_rec_left(struct o
>  		 */
>  		if (le16_to_cpu(right_rec->e_leaf_clusters) == 0 &&
>  		    le16_to_cpu(el->l_next_free_rec) == 1) {
> +			/* extend credit for ocfs2_remove_rightmost_path */
> +			ret = ocfs2_extend_rotate_transaction(handle, 0,
> +					handle->h_buffer_credits,
> +					right_path);
> +			if (ret) {
> +				mlog_errno(ret);
> +				goto out;
> +			}
>  
>  			ret = ocfs2_remove_rightmost_path(handle, et,
>  							  right_path,
> @@ -3679,6 +3660,14 @@ static int ocfs2_try_to_merge_extent(han
>  	BUG_ON(ctxt->c_contig_type == CONTIG_NONE);
>  
>  	if (ctxt->c_split_covers_rec && ctxt->c_has_empty_extent) {
> +		/* extend credit for ocfs2_remove_rightmost_path */
> +		ret = ocfs2_extend_rotate_transaction(handle, 0,
> +				handle->h_buffer_credits,
> +				path);
> +		if (ret) {
> +			mlog_errno(ret);
> +			goto out;
> +		}
>  		/*
>  		 * The merge code will need to create an empty
>  		 * extent to take the place of the newly
> @@ -3727,6 +3716,15 @@ static int ocfs2_try_to_merge_extent(han
>  		 */
>  		BUG_ON(!ocfs2_is_empty_extent(&el->l_recs[0]));
>  
> +		/* extend credit for ocfs2_remove_rightmost_path */
> +		ret = ocfs2_extend_rotate_transaction(handle, 0,
> +					handle->h_buffer_credits,
> +					path);
> +		if (ret) {
> +			mlog_errno(ret);
> +			goto out;
> +		}
> +
>  		/* The merge left us with an empty extent, remove it. */
>  		ret = ocfs2_rotate_tree_left(handle, et, path, dealloc);
>  		if (ret) {
> @@ -3748,6 +3746,15 @@ static int ocfs2_try_to_merge_extent(han
>  			goto out;
>  		}
>  
> +		/* extend credit for ocfs2_remove_rightmost_path */
> +		ret = ocfs2_extend_rotate_transaction(handle, 0,
> +				handle->h_buffer_credits,
> +				path);
> +		if (ret) {
> +			mlog_errno(ret);
> +			goto out;
> +		}
> +
>  		ret = ocfs2_rotate_tree_left(handle, et, path, dealloc);
>  		/*
>  		 * Error from this last rotate is not critical, so
> @@ -3783,6 +3790,16 @@ static int ocfs2_try_to_merge_extent(han
>  		}
>  
>  		if (ctxt->c_split_covers_rec) {
> +			/* extend credit for ocfs2_remove_rightmost_path */
> +			ret = ocfs2_extend_rotate_transaction(handle, 0,
> +					handle->h_buffer_credits,
> +					path);
> +			if (ret) {
> +				mlog_errno(ret);
> +				ret = 0;
> +				goto out;
> +			}
> +
>  			/*
>  			 * The merge may have left an empty extent in
>  			 * our leaf. Try to rotate it away.
> @@ -5342,6 +5359,15 @@ static int ocfs2_truncate_rec(handle_t *
>  	struct ocfs2_extent_block *eb;
>  
>  	if (ocfs2_is_empty_extent(&el->l_recs[0]) && index > 0) {
> +		/* extend credit for ocfs2_remove_rightmost_path */
> +		ret = ocfs2_extend_rotate_transaction(handle, 0,
> +				handle->h_buffer_credits,
> +				path);
> +		if (ret) {
> +			mlog_errno(ret);
> +			goto out;
> +		}
> +
>  		ret = ocfs2_rotate_tree_left(handle, et, path, dealloc);
>  		if (ret) {
>  			mlog_errno(ret);
> _
> 
> .
>