[Ocfs2-devel] [RFC] ocfs2: Make ocfs2_extend_trans really extending.

[Ocfs2-devel] [RFC] ocfs2: Make ocfs2_extend_trans really extending.

[Tao Ma]

Hi Joel/Mark/Sunil,
	This patch just want to make ocfs2_extend_trans(which used
to restart the journal with only 'nblocks' in some case) really
extending. I have met with many troubles when I used this function
in implementing xattr support. And I used to think that it is
my fault.

But I changed my mind when I saw that Joel(Sorry, Joel, ;) ) also use
it wrongly. See
http://git.kernel.org/?p=linux/kernel/git/jlbec/ocfs2.git;a=commitdiff;h=991f171d287781ae12d1ec06cb1917dae3334a9b;hp=f5abd5404e8d1551f1bf58b711543e3b5dcc1079

Acutally ocfs2_extend_trans in ocfs2_block_group_alloc_discontig
has to be
	status = ocfs2_extend_trans(handle,
				    ocfs2_calc_bg_discontig_credits(osb->sb) +
				    handle->h_buffer_credits);
because we will modify the bitmap file later which isn't included in
ocfs2_calc_bg_discontig_credits.

That makes me to think more. So why we don't reserve the old blocks? I just
skimmed all the callers of ocfs2_extend_trans, and to my surprise, 12 of 15
callers added h_buffer_credits by themself to avoid the problem. Then why
handle this in ocfs2_extend_trans? So here comes the patch. Please review.

btw, I am not sure whether this patch will have some conflict with Joel's
patch "make ocfs2_journal_dirty void". I suppose not. But if yes, I will
rebase it when I get some ack.

Regards,
Tao

From 68973800e76d5677a1d519d15fa8969ae348e248 Mon Sep 17 00:00:00 2001
From: Tao Ma <tao.ma@oracle.com>
Date: Mon, 22 Mar 2010 15:50:06 +0800
Subject: [PATCH] ocfs2: Make ocfs2_extend_trans really extending.

In ocfs2, we use ocfs2_extend_trans to extend a journal's
blocks. But it is designed that in case jbd2_journal_extend
fails, it will only restart with the the new block number.
This tends to be awkward since in most cases we want our
original reserved blocks there and make our codes hard to
mantain since the caller sometimes can't make sure all the
original blocks will not be accessed and dirtied again.
There are 15 callers of ocfs2_extend_trans in fs/ocfs2,
and 12 of them have to add h_buffer_credits before they
call ocfs2_extend_trans. So make it really behaves like
extending.

Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/ocfs2/alloc.c        |   30 +++++++++---------------------
 fs/ocfs2/journal.c      |   10 +++++-----
 fs/ocfs2/refcounttree.c |    2 +-
 fs/ocfs2/xattr.c        |   17 ++++++-----------
 4 files changed, 21 insertions(+), 38 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index 9f8bd91..0c55b53 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -1129,8 +1129,7 @@ static int ocfs2_adjust_rightmost_branch(handle_t *handle,
 		goto out;
 	}
 
-	status = ocfs2_extend_trans(handle, path_num_items(path) +
-				    handle->h_buffer_credits);
+	status = ocfs2_extend_trans(handle, path_num_items(path));
 	if (status < 0) {
 		mlog_errno(status);
 		goto out;
@@ -2327,20 +2326,14 @@ static int ocfs2_extend_rotate_transaction(handle_t *handle, int subtree_depth,
 					   int op_credits,
 					   struct ocfs2_path *path)
 {
-	int ret;
+	int ret = 0;
 	int credits = (path->p_tree_depth - subtree_depth) * 2 + 1 + op_credits;
 
-	if (handle->h_buffer_credits < credits) {
+	if (handle->h_buffer_credits < credits)
 		ret = ocfs2_extend_trans(handle,
 					 credits - handle->h_buffer_credits);
-		if (ret)
-			return ret;
-
-		if (unlikely(handle->h_buffer_credits < credits))
-			return ocfs2_extend_trans(handle, credits);
-	}
 
-	return 0;
+	return ret;
 }
 
 /*
@@ -2584,8 +2577,7 @@ static int ocfs2_update_edge_lengths(handle_t *handle,
 	 * records for all the bh in the path.
 	 * So we have to allocate extra credits and access them.
 	 */
-	ret = ocfs2_extend_trans(handle,
-				 handle->h_buffer_credits + subtree_index);
+	ret = ocfs2_extend_trans(handle, subtree_index);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
@@ -4203,17 +4195,13 @@ static int ocfs2_insert_path(handle_t *handle,
 	struct buffer_head *leaf_bh = path_leaf_bh(right_path);
 
 	if (left_path) {
-		int credits = handle->h_buffer_credits;
-
 		/*
 		 * There's a chance that left_path got passed back to
 		 * us without being accounted for in the
 		 * journal. Extend our transaction here to be sure we
 		 * can change those blocks.
 		 */
-		credits += left_path->p_tree_depth;
-
-		ret = ocfs2_extend_trans(handle, credits);
+		ret = ocfs2_extend_trans(handle, left_path->p_tree_depth);
 		if (ret < 0) {
 			mlog_errno(ret);
 			goto out;
@@ -5309,7 +5297,7 @@ static int ocfs2_split_tree(handle_t *handle, struct ocfs2_extent_tree *et,
 			    int index, u32 new_range,
 			    struct ocfs2_alloc_context *meta_ac)
 {
-	int ret, depth, credits = handle->h_buffer_credits;
+	int ret, depth, credits;
 	struct buffer_head *last_eb_bh = NULL;
 	struct ocfs2_extent_block *eb;
 	struct ocfs2_extent_list *rightmost_el, *el;
@@ -5340,8 +5328,8 @@ static int ocfs2_split_tree(handle_t *handle, struct ocfs2_extent_tree *et,
 	} else
 		rightmost_el = path_leaf_el(path);
 
-	credits += path->p_tree_depth +
-		   ocfs2_extend_meta_needed(et->et_root_el);
+	credits = path->p_tree_depth +
+		  ocfs2_extend_meta_needed(et->et_root_el);
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
 		mlog_errno(ret);
diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index 9336c60..4b03a21 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -402,9 +402,7 @@ int ocfs2_commit_trans(struct ocfs2_super *osb,
 }
 
 /*
- * 'nblocks' is what you want to add to the current
- * transaction. extend_trans will either extend the current handle by
- * nblocks, or commit it and start a new one with nblocks credits.
+ * 'nblocks' is what you want to add to the current transaction.
  *
  * This might call jbd2_journal_restart() which will commit dirty buffers
  * and then restart the transaction. Before calling
@@ -422,11 +420,12 @@ int ocfs2_commit_trans(struct ocfs2_super *osb,
  */
 int ocfs2_extend_trans(handle_t *handle, int nblocks)
 {
-	int status;
+	int status, old_nblocks;
 
 	BUG_ON(!handle);
 	BUG_ON(!nblocks);
 
+	old_nblocks = handle->h_buffer_credits;
 	mlog_entry_void();
 
 	mlog(0, "Trying to extend transaction by %d blocks\n", nblocks);
@@ -445,7 +444,8 @@ int ocfs2_extend_trans(handle_t *handle, int nblocks)
 		mlog(0,
 		     "jbd2_journal_extend failed, trying "
 		     "jbd2_journal_restart\n");
-		status = jbd2_journal_restart(handle, nblocks);
+		status = jbd2_journal_restart(handle,
+					      old_nblocks + nblocks);
 		if (status < 0) {
 			mlog_errno(status);
 			goto bail;
diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 29405f2..02d3f82 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -1695,7 +1695,7 @@ static int ocfs2_adjust_refcount_rec(handle_t *handle,
 	 * 2 more credits, one for the leaf refcount block, one for
 	 * the extent block contains the extent rec.
 	 */
-	ret = ocfs2_extend_trans(handle, handle->h_buffer_credits + 2);
+	ret = ocfs2_extend_trans(handle, 2);
 	if (ret < 0) {
 		mlog_errno(ret);
 		goto out;
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 3e77730..bbbe212 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -3312,8 +3312,7 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
 				goto out;
 			}
 
-			ret = ocfs2_extend_trans(ctxt->handle, credits +
-					ctxt->handle->h_buffer_credits);
+			ret = ocfs2_extend_trans(ctxt->handle, credits);
 			if (ret) {
 				mlog_errno(ret);
 				goto out;
@@ -3343,8 +3342,7 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
 					goto out;
 				}
 
-				ret = ocfs2_extend_trans(ctxt->handle, credits +
-					ctxt->handle->h_buffer_credits);
+				ret = ocfs2_extend_trans(ctxt->handle, credits);
 				if (ret) {
 					mlog_errno(ret);
 					goto out;
@@ -3378,8 +3376,7 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
 					goto out;
 				}
 
-				ret = ocfs2_extend_trans(ctxt->handle, credits +
-						ctxt->handle->h_buffer_credits);
+				ret = ocfs2_extend_trans(ctxt->handle, credits);
 				if (ret) {
 					mlog_errno(ret);
 					goto out;
@@ -4887,8 +4884,7 @@ static int ocfs2_mv_xattr_buckets(struct inode *inode, handle_t *handle,
 	 * We need to update the first bucket of the old extent and all
 	 * the buckets going to the new extent.
 	 */
-	credits = ((num_buckets + 1) * blks_per_bucket) +
-		handle->h_buffer_credits;
+	credits = ((num_buckets + 1) * blks_per_bucket);
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
 		mlog_errno(ret);
@@ -4958,7 +4954,7 @@ static int ocfs2_divide_xattr_cluster(struct inode *inode,
 				      u32 *first_hash)
 {
 	u16 blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
-	int ret, credits = 2 * blk_per_bucket + handle->h_buffer_credits;
+	int ret, credits = 2 * blk_per_bucket;
 
 	BUG_ON(OCFS2_XATTR_BUCKET_SIZE < OCFS2_SB(inode->i_sb)->s_clustersize);
 
@@ -5200,8 +5196,7 @@ static int ocfs2_extend_xattr_bucket(struct inode *inode,
 	 * existing bucket.  Then we add the last existing bucket, the
 	 * new bucket, and the first bucket (3 * blk_per_bucket).
 	 */
-	credits = (end_blk - target_blk) + (3 * blk_per_bucket) +
-		  handle->h_buffer_credits;
+	credits = (end_blk - target_blk) + (3 * blk_per_bucket);
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
 		mlog_errno(ret);
-- 
1.5.5

[Ocfs2-devel] [RFC] ocfs2: Make ocfs2_extend_trans really extending.

[Mark Fasheh]

On Mon, Mar 22, 2010 at 04:01:44PM +0800, Tao Ma wrote:
> Hi Joel/Mark/Sunil,
> 	This patch just want to make ocfs2_extend_trans(which used
> to restart the journal with only 'nblocks' in some case) really
> extending. I have met with many troubles when I used this function
> in implementing xattr support. And I used to think that it is
> my fault.
> 
> But I changed my mind when I saw that Joel(Sorry, Joel, ;) ) also use
> it wrongly. See
> http://git.kernel.org/?p=linux/kernel/git/jlbec/ocfs2.git;a=commitdiff;h=991f171d287781ae12d1ec06cb1917dae3334a9b;hp=f5abd5404e8d1551f1bf58b711543e3b5dcc1079
> 
> Acutally ocfs2_extend_trans in ocfs2_block_group_alloc_discontig
> has to be
> 	status = ocfs2_extend_trans(handle,
> 				    ocfs2_calc_bg_discontig_credits(osb->sb) +
> 				    handle->h_buffer_credits);
> because we will modify the bitmap file later which isn't included in
> ocfs2_calc_bg_discontig_credits.
> 
> That makes me to think more. So why we don't reserve the old blocks? I just
> skimmed all the callers of ocfs2_extend_trans, and to my surprise, 12 of 15
> callers added h_buffer_credits by themself to avoid the problem. Then why
> handle this in ocfs2_extend_trans? So here comes the patch. Please review.

Tao, this patch looks good to me. Thank you for taking on the time to fix up
this old interface. Have you tested the patch much? What are the results
like? Not only in stability, but does the new math inside
ocfs2_extend_trans() speed up any long-running operations?
	--Mark

--
Mark Fasheh

[Ocfs2-devel] [RFC] ocfs2: Make ocfs2_extend_trans really extending.

[Tao Ma]

Hi Mark,
Mark Fasheh wrote:
> On Mon, Mar 22, 2010 at 04:01:44PM +0800, Tao Ma wrote:
>   
>> Hi Joel/Mark/Sunil,
>> 	This patch just want to make ocfs2_extend_trans(which used
>> to restart the journal with only 'nblocks' in some case) really
>> extending. I have met with many troubles when I used this function
>> in implementing xattr support. And I used to think that it is
>> my fault.
>>
>> But I changed my mind when I saw that Joel(Sorry, Joel, ;) ) also use
>> it wrongly. See
>> http://git.kernel.org/?p=linux/kernel/git/jlbec/ocfs2.git;a=commitdiff;h=991f171d287781ae12d1ec06cb1917dae3334a9b;hp=f5abd5404e8d1551f1bf58b711543e3b5dcc1079
>>
>> Acutally ocfs2_extend_trans in ocfs2_block_group_alloc_discontig
>> has to be
>> 	status = ocfs2_extend_trans(handle,
>> 				    ocfs2_calc_bg_discontig_credits(osb->sb) +
>> 				    handle->h_buffer_credits);
>> because we will modify the bitmap file later which isn't included in
>> ocfs2_calc_bg_discontig_credits.
>>
>> That makes me to think more. So why we don't reserve the old blocks? I just
>> skimmed all the callers of ocfs2_extend_trans, and to my surprise, 12 of 15
>> callers added h_buffer_credits by themself to avoid the problem. Then why
>> handle this in ocfs2_extend_trans? So here comes the patch. Please review.
>>     
>
> Tao, this patch looks good to me. Thank you for taking on the time to fix up
> this old interface. Have you tested the patch much? What are the results
> like? Not only in stability, but does the new math inside
> ocfs2_extend_trans() speed up any long-running operations?
>   
I only did tristan's reflink test cases. And since it is only a RFC, I 
haven't put much time on it.
If you and Joel are both fine with it, I will give it more try, 
especially the stress test case on xattr, since it depends on
it heavily. And also I will run some fill_verify_hole test which will 
stress our b-tree codes.
I think these 2 places may be impacted most by this patch.

Regards,
Tao

[Ocfs2-devel] [RFC] ocfs2: Make ocfs2_extend_trans really extending.

[Joel Becker]

On Thu, Apr 08, 2010 at 07:43:20AM +0800, Tao Ma wrote:
> I only did tristan's reflink test cases. And since it is only a RFC,
> I haven't put much time on it.
> If you and Joel are both fine with it, I will give it more try,
> especially the stress test case on xattr, since it depends on
> it heavily. And also I will run some fill_verify_hole test which
> will stress our b-tree codes.
> I think these 2 places may be impacted most by this patch.

	Yes, please.  I agree it simplifies the call, and it may be a
performance help (fewer unnecessary commits).

Joel

-- 

"Senator let's be sincere,
 As much as you can."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [RFC] ocfs2: Make ocfs2_extend_trans really extending.

[Tao Ma]

Hi Joel,

On Thu, Apr 08, 2010 at 07:43:20AM +0800, Tao Ma wrote:
> > I only did tristan's reflink test cases. And since it is only a RFC,
> > I haven't put much time on it.
> > If you and Joel are both fine with it, I will give it more try,
> > especially the stress test case on xattr, since it depends on
> > it heavily. And also I will run some fill_verify_hole test which
> > will stress our b-tree codes.
> > I think these 2 places may be impacted most by this patch.

>	Yes, please.  I agree it simplifies the call, and it may be a
>performance help (fewer unnecessary commits).

I have finished the tests on xattr and reflink(I use reflink for b-tree
test because it also use b-tree heavily and also it has a stress test
case which create a b-tree with depth=2 easily). Both test cases works.
So please consider including it in the merge window. Thanks.

I also changed the patch a little in the beginning of ocfs2_extend_trans.
-	BUG_ON(!nblocks);
+	if (nblocks <= 0)
+		return 0;
+
The reason is that some b-tree codes now will extend transcation with
tree depth. So sometimes it may give us a '0' and we will just return back.

Regards,
Tao

From ad5e1239ffc85adb5ac98b29b369388e2849c772 Mon Sep 17 00:00:00 2001
From: Tao Ma <tao.ma@oracle.com>
Date: Wed, 14 Apr 2010 11:00:21 +0800
Subject: [PATCH] ocfs2: Make ocfs2_extend_trans really extending.

In ocfs2, we use ocfs2_extend_trans to extend a journal's
blocks. But it is designed that in case jbd2_journal_extend
fails, it will only restart with the the new block number.
This tends to be awkward since in most cases we want our
original reserved blocks there and make our codes hard to
mantain since the caller sometimes can't make sure all the
original blocks will not be accessed and dirtied again.
There are 15 callers of ocfs2_extend_trans in fs/ocfs2,
and 12 of them have to add h_buffer_credits before they
call ocfs2_extend_trans. So make it really behaves like
extending.

Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/ocfs2/alloc.c        |   30 +++++++++---------------------
 fs/ocfs2/journal.c      |   14 ++++++++------
 fs/ocfs2/refcounttree.c |    2 +-
 fs/ocfs2/xattr.c        |   17 ++++++-----------
 4 files changed, 24 insertions(+), 39 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index a74ea70..0cb2945 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -1125,8 +1125,7 @@ static int ocfs2_adjust_rightmost_branch(handle_t *handle,
 		goto out;
 	}
 
-	status = ocfs2_extend_trans(handle, path_num_items(path) +
-				    handle->h_buffer_credits);
+	status = ocfs2_extend_trans(handle, path_num_items(path));
 	if (status < 0) {
 		mlog_errno(status);
 		goto out;
@@ -2288,20 +2287,14 @@ static int ocfs2_extend_rotate_transaction(handle_t *handle, int subtree_depth,
 					   int op_credits,
 					   struct ocfs2_path *path)
 {
-	int ret;
+	int ret = 0;
 	int credits = (path->p_tree_depth - subtree_depth) * 2 + 1 + op_credits;
 
-	if (handle->h_buffer_credits < credits) {
+	if (handle->h_buffer_credits < credits)
 		ret = ocfs2_extend_trans(handle,
 					 credits - handle->h_buffer_credits);
-		if (ret)
-			return ret;
-
-		if (unlikely(handle->h_buffer_credits < credits))
-			return ocfs2_extend_trans(handle, credits);
-	}
 
-	return 0;
+	return ret;
 }
 
 /*
@@ -2545,8 +2538,7 @@ static int ocfs2_update_edge_lengths(handle_t *handle,
 	 * records for all the bh in the path.
 	 * So we have to allocate extra credits and access them.
 	 */
-	ret = ocfs2_extend_trans(handle,
-				 handle->h_buffer_credits + subtree_index);
+	ret = ocfs2_extend_trans(handle, subtree_index);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
@@ -4141,17 +4133,13 @@ static int ocfs2_insert_path(handle_t *handle,
 	struct buffer_head *leaf_bh = path_leaf_bh(right_path);
 
 	if (left_path) {
-		int credits = handle->h_buffer_credits;
-
 		/*
 		 * There's a chance that left_path got passed back to
 		 * us without being accounted for in the
 		 * journal. Extend our transaction here to be sure we
 		 * can change those blocks.
 		 */
-		credits += left_path->p_tree_depth;
-
-		ret = ocfs2_extend_trans(handle, credits);
+		ret = ocfs2_extend_trans(handle, left_path->p_tree_depth);
 		if (ret < 0) {
 			mlog_errno(ret);
 			goto out;
@@ -5237,7 +5225,7 @@ static int ocfs2_split_tree(handle_t *handle, struct ocfs2_extent_tree *et,
 			    int index, u32 new_range,
 			    struct ocfs2_alloc_context *meta_ac)
 {
-	int ret, depth, credits = handle->h_buffer_credits;
+	int ret, depth, credits;
 	struct buffer_head *last_eb_bh = NULL;
 	struct ocfs2_extent_block *eb;
 	struct ocfs2_extent_list *rightmost_el, *el;
@@ -5268,8 +5256,8 @@ static int ocfs2_split_tree(handle_t *handle, struct ocfs2_extent_tree *et,
 	} else
 		rightmost_el = path_leaf_el(path);
 
-	credits += path->p_tree_depth +
-		   ocfs2_extend_meta_needed(et->et_root_el);
+	credits = path->p_tree_depth +
+		  ocfs2_extend_meta_needed(et->et_root_el);
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
 		mlog_errno(ret);
diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index cfd271c..a81e981 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -402,9 +402,7 @@ int ocfs2_commit_trans(struct ocfs2_super *osb,
 }
 
 /*
- * 'nblocks' is what you want to add to the current
- * transaction. extend_trans will either extend the current handle by
- * nblocks, or commit it and start a new one with nblocks credits.
+ * 'nblocks' is what you want to add to the current transaction.
  *
  * This might call jbd2_journal_restart() which will commit dirty buffers
  * and then restart the transaction. Before calling
@@ -422,11 +420,14 @@ int ocfs2_commit_trans(struct ocfs2_super *osb,
  */
 int ocfs2_extend_trans(handle_t *handle, int nblocks)
 {
-	int status;
+	int status, old_nblocks;
 
 	BUG_ON(!handle);
-	BUG_ON(!nblocks);
 
+	if (nblocks <= 0)
+		return 0;
+
+	old_nblocks = handle->h_buffer_credits;
 	mlog_entry_void();
 
 	mlog(0, "Trying to extend transaction by %d blocks\n", nblocks);
@@ -445,7 +446,8 @@ int ocfs2_extend_trans(handle_t *handle, int nblocks)
 		mlog(0,
 		     "jbd2_journal_extend failed, trying "
 		     "jbd2_journal_restart\n");
-		status = jbd2_journal_restart(handle, nblocks);
+		status = jbd2_journal_restart(handle,
+					      old_nblocks + nblocks);
 		if (status < 0) {
 			mlog_errno(status);
 			goto bail;
diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 44d3e1c..55e4b9a 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -1693,7 +1693,7 @@ static int ocfs2_adjust_refcount_rec(handle_t *handle,
 	 * 2 more credits, one for the leaf refcount block, one for
 	 * the extent block contains the extent rec.
 	 */
-	ret = ocfs2_extend_trans(handle, handle->h_buffer_credits + 2);
+	ret = ocfs2_extend_trans(handle, 2);
 	if (ret < 0) {
 		mlog_errno(ret);
 		goto out;
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 4cf6fde..38a55ff 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -3295,8 +3295,7 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
 				goto out;
 			}
 
-			ret = ocfs2_extend_trans(ctxt->handle, credits +
-					ctxt->handle->h_buffer_credits);
+			ret = ocfs2_extend_trans(ctxt->handle, credits);
 			if (ret) {
 				mlog_errno(ret);
 				goto out;
@@ -3326,8 +3325,7 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
 					goto out;
 				}
 
-				ret = ocfs2_extend_trans(ctxt->handle, credits +
-					ctxt->handle->h_buffer_credits);
+				ret = ocfs2_extend_trans(ctxt->handle, credits);
 				if (ret) {
 					mlog_errno(ret);
 					goto out;
@@ -3361,8 +3359,7 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
 					goto out;
 				}
 
-				ret = ocfs2_extend_trans(ctxt->handle, credits +
-						ctxt->handle->h_buffer_credits);
+				ret = ocfs2_extend_trans(ctxt->handle, credits);
 				if (ret) {
 					mlog_errno(ret);
 					goto out;
@@ -4870,8 +4867,7 @@ static int ocfs2_mv_xattr_buckets(struct inode *inode, handle_t *handle,
 	 * We need to update the first bucket of the old extent and all
 	 * the buckets going to the new extent.
 	 */
-	credits = ((num_buckets + 1) * blks_per_bucket) +
-		handle->h_buffer_credits;
+	credits = ((num_buckets + 1) * blks_per_bucket);
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
 		mlog_errno(ret);
@@ -4941,7 +4937,7 @@ static int ocfs2_divide_xattr_cluster(struct inode *inode,
 				      u32 *first_hash)
 {
 	u16 blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
-	int ret, credits = 2 * blk_per_bucket + handle->h_buffer_credits;
+	int ret, credits = 2 * blk_per_bucket;
 
 	BUG_ON(OCFS2_XATTR_BUCKET_SIZE < OCFS2_SB(inode->i_sb)->s_clustersize);
 
@@ -5181,8 +5177,7 @@ static int ocfs2_extend_xattr_bucket(struct inode *inode,
 	 * existing bucket.  Then we add the last existing bucket, the
 	 * new bucket, and the first bucket (3 * blk_per_bucket).
 	 */
-	credits = (end_blk - target_blk) + (3 * blk_per_bucket) +
-		  handle->h_buffer_credits;
+	credits = (end_blk - target_blk) + (3 * blk_per_bucket);
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
 		mlog_errno(ret);
-- 
1.6.3.3.334.g916e1.dirty

[Ocfs2-devel] [RFC] ocfs2: Make ocfs2_extend_trans really extending.

[Joel Becker]

On Wed, Apr 21, 2010 at 11:29:39AM +0800, Tao Ma wrote:
> I have finished the tests on xattr and reflink(I use reflink for b-tree
> test because it also use b-tree heavily and also it has a stress test
> case which create a b-tree with depth=2 easily). Both test cases works.
> So please consider including it in the merge window. Thanks.

	I think I'm happy to take this patch, but a couple things first.

> I also changed the patch a little in the beginning of ocfs2_extend_trans.
> -	BUG_ON(!nblocks);
> +	if (nblocks <= 0)
> +		return 0;
> +
> The reason is that some b-tree codes now will extend transcation with
> tree depth. So sometimes it may give us a '0' and we will just return back.

	Well, negative nblocks is still a bug, right?  shouldn't we at
least warn or make it an unsigned int?

Joel

-- 

"In a crisis, don't hide behind anything or anybody. They're going
 to find you anyway."
	- Paul "Bear" Bryant

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH] ocfs2: Make ocfs2_extend_trans really extending.

[Tao Ma]

On Wed, Apr 21, 2010 at 11:29:39AM +0800, Tao Ma wrote:
> > I have finished the tests on xattr and reflink(I use reflink for b-tree
> > test because it also use b-tree heavily and also it has a stress test
> > case which create a b-tree with depth=2 easily). Both test cases works.
> > So please consider including it in the merge window. Thanks.

>	I think I'm happy to take this patch, but a couple things first.

> > I also changed the patch a little in the beginning of ocfs2_extend_trans.
> > -	BUG_ON(!nblocks);
> > +	if (nblocks <= 0)
> > +		return 0;
> > +
> > The reason is that some b-tree codes now will extend transcation with
> > tree depth. So sometimes it may give us a '0' and we will just return back.

>	Well, negative nblocks is still a bug, right?  shouldn't we at
>least warn or make it an unsigned int?
yeah, nblocks < 0 is a bug I think. I have added the BUG_ON for this case.
Here is the updated one.

Regards,
Tao

From c9dc26549b429eede9e47094a98b067bd5cd23b1 Mon Sep 17 00:00:00 2001
From: Tao Ma <tao.ma@oracle.com>
Date: Mon, 26 Apr 2010 13:49:11 +0800
Subject: [PATCH] ocfs2: Make ocfs2_extend_trans really extending.

In ocfs2, we use ocfs2_extend_trans to extend a journal's
blocks. But it is designed that in case jbd2_journal_extend
fails, it will only restart with the the new block number.
This tends to be awkward since in most cases we want our
original reserved blocks there and make our codes hard to
mantain since the caller sometimes can't make sure all the
original blocks will not be accessed and dirtied again.
There are 15 callers of ocfs2_extend_trans in fs/ocfs2,
and 12 of them have to add h_buffer_credits before they
call ocfs2_extend_trans. So make it really behaves like
extending.

Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/ocfs2/alloc.c        |   30 +++++++++---------------------
 fs/ocfs2/journal.c      |   15 +++++++++------
 fs/ocfs2/refcounttree.c |    2 +-
 fs/ocfs2/xattr.c        |   17 ++++++-----------
 4 files changed, 25 insertions(+), 39 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index a74ea70..0cb2945 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -1125,8 +1125,7 @@ static int ocfs2_adjust_rightmost_branch(handle_t *handle,
 		goto out;
 	}
 
-	status = ocfs2_extend_trans(handle, path_num_items(path) +
-				    handle->h_buffer_credits);
+	status = ocfs2_extend_trans(handle, path_num_items(path));
 	if (status < 0) {
 		mlog_errno(status);
 		goto out;
@@ -2288,20 +2287,14 @@ static int ocfs2_extend_rotate_transaction(handle_t *handle, int subtree_depth,
 					   int op_credits,
 					   struct ocfs2_path *path)
 {
-	int ret;
+	int ret = 0;
 	int credits = (path->p_tree_depth - subtree_depth) * 2 + 1 + op_credits;
 
-	if (handle->h_buffer_credits < credits) {
+	if (handle->h_buffer_credits < credits)
 		ret = ocfs2_extend_trans(handle,
 					 credits - handle->h_buffer_credits);
-		if (ret)
-			return ret;
-
-		if (unlikely(handle->h_buffer_credits < credits))
-			return ocfs2_extend_trans(handle, credits);
-	}
 
-	return 0;
+	return ret;
 }
 
 /*
@@ -2545,8 +2538,7 @@ static int ocfs2_update_edge_lengths(handle_t *handle,
 	 * records for all the bh in the path.
 	 * So we have to allocate extra credits and access them.
 	 */
-	ret = ocfs2_extend_trans(handle,
-				 handle->h_buffer_credits + subtree_index);
+	ret = ocfs2_extend_trans(handle, subtree_index);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
@@ -4141,17 +4133,13 @@ static int ocfs2_insert_path(handle_t *handle,
 	struct buffer_head *leaf_bh = path_leaf_bh(right_path);
 
 	if (left_path) {
-		int credits = handle->h_buffer_credits;
-
 		/*
 		 * There's a chance that left_path got passed back to
 		 * us without being accounted for in the
 		 * journal. Extend our transaction here to be sure we
 		 * can change those blocks.
 		 */
-		credits += left_path->p_tree_depth;
-
-		ret = ocfs2_extend_trans(handle, credits);
+		ret = ocfs2_extend_trans(handle, left_path->p_tree_depth);
 		if (ret < 0) {
 			mlog_errno(ret);
 			goto out;
@@ -5237,7 +5225,7 @@ static int ocfs2_split_tree(handle_t *handle, struct ocfs2_extent_tree *et,
 			    int index, u32 new_range,
 			    struct ocfs2_alloc_context *meta_ac)
 {
-	int ret, depth, credits = handle->h_buffer_credits;
+	int ret, depth, credits;
 	struct buffer_head *last_eb_bh = NULL;
 	struct ocfs2_extent_block *eb;
 	struct ocfs2_extent_list *rightmost_el, *el;
@@ -5268,8 +5256,8 @@ static int ocfs2_split_tree(handle_t *handle, struct ocfs2_extent_tree *et,
 	} else
 		rightmost_el = path_leaf_el(path);
 
-	credits += path->p_tree_depth +
-		   ocfs2_extend_meta_needed(et->et_root_el);
+	credits = path->p_tree_depth +
+		  ocfs2_extend_meta_needed(et->et_root_el);
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
 		mlog_errno(ret);
diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index cfd271c..47878cf 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -402,9 +402,7 @@ int ocfs2_commit_trans(struct ocfs2_super *osb,
 }
 
 /*
- * 'nblocks' is what you want to add to the current
- * transaction. extend_trans will either extend the current handle by
- * nblocks, or commit it and start a new one with nblocks credits.
+ * 'nblocks' is what you want to add to the current transaction.
  *
  * This might call jbd2_journal_restart() which will commit dirty buffers
  * and then restart the transaction. Before calling
@@ -422,11 +420,15 @@ int ocfs2_commit_trans(struct ocfs2_super *osb,
  */
 int ocfs2_extend_trans(handle_t *handle, int nblocks)
 {
-	int status;
+	int status, old_nblocks;
 
 	BUG_ON(!handle);
-	BUG_ON(!nblocks);
+	BUG_ON(nblocks < 0);
+
+	if (!nblocks)
+		return 0;
 
+	old_nblocks = handle->h_buffer_credits;
 	mlog_entry_void();
 
 	mlog(0, "Trying to extend transaction by %d blocks\n", nblocks);
@@ -445,7 +447,8 @@ int ocfs2_extend_trans(handle_t *handle, int nblocks)
 		mlog(0,
 		     "jbd2_journal_extend failed, trying "
 		     "jbd2_journal_restart\n");
-		status = jbd2_journal_restart(handle, nblocks);
+		status = jbd2_journal_restart(handle,
+					      old_nblocks + nblocks);
 		if (status < 0) {
 			mlog_errno(status);
 			goto bail;
diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 44d3e1c..55e4b9a 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -1693,7 +1693,7 @@ static int ocfs2_adjust_refcount_rec(handle_t *handle,
 	 * 2 more credits, one for the leaf refcount block, one for
 	 * the extent block contains the extent rec.
 	 */
-	ret = ocfs2_extend_trans(handle, handle->h_buffer_credits + 2);
+	ret = ocfs2_extend_trans(handle, 2);
 	if (ret < 0) {
 		mlog_errno(ret);
 		goto out;
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 26f704c..56f5c71 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -3295,8 +3295,7 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
 				goto out;
 			}
 
-			ret = ocfs2_extend_trans(ctxt->handle, credits +
-					ctxt->handle->h_buffer_credits);
+			ret = ocfs2_extend_trans(ctxt->handle, credits);
 			if (ret) {
 				mlog_errno(ret);
 				goto out;
@@ -3326,8 +3325,7 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
 					goto out;
 				}
 
-				ret = ocfs2_extend_trans(ctxt->handle, credits +
-					ctxt->handle->h_buffer_credits);
+				ret = ocfs2_extend_trans(ctxt->handle, credits);
 				if (ret) {
 					mlog_errno(ret);
 					goto out;
@@ -3361,8 +3359,7 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
 					goto out;
 				}
 
-				ret = ocfs2_extend_trans(ctxt->handle, credits +
-						ctxt->handle->h_buffer_credits);
+				ret = ocfs2_extend_trans(ctxt->handle, credits);
 				if (ret) {
 					mlog_errno(ret);
 					goto out;
@@ -4870,8 +4867,7 @@ static int ocfs2_mv_xattr_buckets(struct inode *inode, handle_t *handle,
 	 * We need to update the first bucket of the old extent and all
 	 * the buckets going to the new extent.
 	 */
-	credits = ((num_buckets + 1) * blks_per_bucket) +
-		handle->h_buffer_credits;
+	credits = ((num_buckets + 1) * blks_per_bucket);
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
 		mlog_errno(ret);
@@ -4941,7 +4937,7 @@ static int ocfs2_divide_xattr_cluster(struct inode *inode,
 				      u32 *first_hash)
 {
 	u16 blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
-	int ret, credits = 2 * blk_per_bucket + handle->h_buffer_credits;
+	int ret, credits = 2 * blk_per_bucket;
 
 	BUG_ON(OCFS2_XATTR_BUCKET_SIZE < OCFS2_SB(inode->i_sb)->s_clustersize);
 
@@ -5181,8 +5177,7 @@ static int ocfs2_extend_xattr_bucket(struct inode *inode,
 	 * existing bucket.  Then we add the last existing bucket, the
 	 * new bucket, and the first bucket (3 * blk_per_bucket).
 	 */
-	credits = (end_blk - target_blk) + (3 * blk_per_bucket) +
-		  handle->h_buffer_credits;
+	credits = (end_blk - target_blk) + (3 * blk_per_bucket);
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
 		mlog_errno(ret);
-- 
1.5.5

[Ocfs2-devel] [PATCH] ocfs2: Make ocfs2_extend_trans really extending.

[Joel Becker]

On Mon, Apr 26, 2010 at 02:34:57PM +0800, Tao Ma wrote:
> >From c9dc26549b429eede9e47094a98b067bd5cd23b1 Mon Sep 17 00:00:00 2001
> From: Tao Ma <tao.ma@oracle.com>
> Date: Mon, 26 Apr 2010 13:49:11 +0800
> Subject: [PATCH] ocfs2: Make ocfs2_extend_trans really extending.
> 
> In ocfs2, we use ocfs2_extend_trans to extend a journal's
> blocks. But it is designed that in case jbd2_journal_extend
> fails, it will only restart with the the new block number.
> This tends to be awkward since in most cases we want our
> original reserved blocks there and make our codes hard to
> mantain since the caller sometimes can't make sure all the
> original blocks will not be accessed and dirtied again.
> There are 15 callers of ocfs2_extend_trans in fs/ocfs2,
> and 12 of them have to add h_buffer_credits before they
> call ocfs2_extend_trans. So make it really behaves like
> extending.
> 
> Signed-off-by: Tao Ma <tao.ma@oracle.com>

	This is now in the 'merge-window' branch of ocfs2.git.

Joel

-- 

"Hey mister if you're gonna walk on water,
 Could you drop a line my way?"

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127