[Ocfs2-devel] [PATCH 0/3] ocfs2/xattr: xattr improvement

[Ocfs2-devel] [PATCH 0/3] ocfs2/xattr: xattr improvement

[Tao Ma]

Hi all,
	these 3 patches are already sent out yesterday, but some minor 
modifications are added. And the 3rd patch are rebased on the first 2 
ones. So resend them out.

modifications:
patch 2: Rename ocfs2_xattr_set_handle to __ocfs2_xattr_set_handle since 
tiger need this in acl.
patch 3: rebase it atop of the first 2.

Regards,
Tao

[Ocfs2-devel] [PATCH 1/3] ocfs2/xattr: Remove unused restore_extent_block.

[Tao Ma]

Since now ocfs2 supports empty bucket, we will never remove
xattr index tree even if all the xattrs are removed, so this
function will never be called. So remove it.

Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/ocfs2/xattr.c |   48 ------------------------------------------------
 1 files changed, 0 insertions(+), 48 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 8c0041e..f158280 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -1809,52 +1809,6 @@ cleanup:
 }
 
 /*
- * When all the xattrs are deleted from index btree, the ocfs2_xattr_tree
- * will be erased and ocfs2_xattr_block will have its ocfs2_xattr_header
- * re-initialized.
- */
-static int ocfs2_restore_xattr_block(struct inode *inode,
-				     struct ocfs2_xattr_search *xs)
-{
-	int ret;
-	handle_t *handle;
-	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
-	struct ocfs2_xattr_block *xb =
-		(struct ocfs2_xattr_block *)xs->xattr_bh->b_data;
-	struct ocfs2_extent_list *el = &xb->xb_attrs.xb_root.xt_list;
-	u16 xb_flags = le16_to_cpu(xb->xb_flags);
-
-	BUG_ON(!(xb_flags & OCFS2_XATTR_INDEXED) ||
-		le16_to_cpu(el->l_next_free_rec) != 0);
-
-	handle = ocfs2_start_trans(osb, OCFS2_XATTR_BLOCK_UPDATE_CREDITS);
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		handle = NULL;
-		goto out;
-	}
-
-	ret = ocfs2_journal_access(handle, inode, xs->xattr_bh,
-				   OCFS2_JOURNAL_ACCESS_WRITE);
-	if (ret < 0) {
-		mlog_errno(ret);
-		goto out_commit;
-	}
-
-	memset(&xb->xb_attrs, 0, inode->i_sb->s_blocksize -
-	       offsetof(struct ocfs2_xattr_block, xb_attrs));
-
-	xb->xb_flags = cpu_to_le16(xb_flags & ~OCFS2_XATTR_INDEXED);
-
-	ocfs2_journal_dirty(handle, xs->xattr_bh);
-
-out_commit:
-	ocfs2_commit_trans(osb, handle);
-out:
-	return ret;
-}
-
-/*
  * ocfs2_xattr_block_set()
  *
  * Set, replace or remove an extended attribute into external block.
@@ -1964,8 +1918,6 @@ out:
 	}
 
 	ret = ocfs2_xattr_set_entry_index_block(inode, xi, xs);
-	if (!ret && xblk->xb_attrs.xb_root.xt_list.l_next_free_rec == 0)
-		ret = ocfs2_restore_xattr_block(inode, xs);
 
 end:
 
-- 
1.5.4.GIT

[Ocfs2-devel] [PATCH 2/3] ocfs2/xattr: Merge xattr set transaction.

[Tao Ma]

In current ocfs2/xattr, the whole xattr set is divided into
many steps are many transaction are used, this make the
xattr set process isn't like a real transaction, so this
patch try to merge all the transaction into one. Another
benefit is that acl can use it easily now.

I don't merge the transaction of deleting xattr when we
remove an inode. The reason is that if we have a large number
of xattrs and every xattrs has large values(large enough
for outside storage), the whole transaction will be very
huge and it looks like jbd can't handle it(I meet with a
jbd complain once). And the old inode removal is also divided
into many steps, so I'd like to leave as it is.

Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/ocfs2/xattr.c |  889 +++++++++++++++++++++++++++++++-----------------------
 1 files changed, 514 insertions(+), 375 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index f158280..1bf72da 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -70,6 +70,12 @@ struct ocfs2_xattr_bucket {
 	struct ocfs2_xattr_header *xh;
 };
 
+struct ocfs2_xattr_set_ctxt {
+	struct ocfs2_alloc_context *meta_ac;
+	struct ocfs2_alloc_context *data_ac;
+	struct ocfs2_cached_dealloc_ctxt dealloc;
+};
+
 #define OCFS2_XATTR_ROOT_SIZE	(sizeof(struct ocfs2_xattr_def_value_root))
 #define OCFS2_XATTR_INLINE_SIZE	80
 
@@ -128,11 +134,15 @@ static int ocfs2_xattr_tree_list_index_block(struct inode *inode,
 					size_t buffer_size);
 
 static int ocfs2_xattr_create_index_block(struct inode *inode,
-					  struct ocfs2_xattr_search *xs);
+					  handle_t *handle,
+					  struct ocfs2_xattr_search *xs,
+					  struct ocfs2_xattr_set_ctxt *ctxt);
 
 static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
+					     handle_t *handle,
 					     struct ocfs2_xattr_info *xi,
-					     struct ocfs2_xattr_search *xs);
+					     struct ocfs2_xattr_search *xs,
+					     struct ocfs2_xattr_set_ctxt *ctxt);
 
 static int ocfs2_delete_xattr_index_block(struct inode *inode,
 					  struct buffer_head *xb_bh);
@@ -184,16 +194,14 @@ static void ocfs2_xattr_hash_entry(struct inode *inode,
 }
 
 static int ocfs2_xattr_extend_allocation(struct inode *inode,
+					 handle_t *handle,
 					 u32 clusters_to_add,
 					 struct buffer_head *xattr_bh,
-					 struct ocfs2_xattr_value_root *xv)
+					 struct ocfs2_xattr_value_root *xv,
+					 struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	int status = 0;
-	int restart_func = 0;
 	int credits = 0;
-	handle_t *handle = NULL;
-	struct ocfs2_alloc_context *data_ac = NULL;
-	struct ocfs2_alloc_context *meta_ac = NULL;
 	enum ocfs2_alloc_restarted why;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	u32 prev_clusters, logical_start = le32_to_cpu(xv->xr_clusters);
@@ -203,21 +211,10 @@ static int ocfs2_xattr_extend_allocation(struct inode *inode,
 
 	ocfs2_init_xattr_value_extent_tree(&et, inode, xattr_bh, xv);
 
-restart_all:
-
-	status = ocfs2_lock_allocators(inode, &et, clusters_to_add, 0,
-				       &data_ac, &meta_ac);
-	if (status) {
-		mlog_errno(status);
-		goto leave;
-	}
-
 	credits = ocfs2_calc_extend_credits(osb->sb, et.et_root_el,
-					    clusters_to_add);
-	handle = ocfs2_start_trans(osb, credits);
-	if (IS_ERR(handle)) {
-		status = PTR_ERR(handle);
-		handle = NULL;
+			clusters_to_add) + handle->h_buffer_credits;
+	status = ocfs2_extend_trans(handle, credits);
+	if (status) {
 		mlog_errno(status);
 		goto leave;
 	}
@@ -238,8 +235,8 @@ restarted_transaction:
 					     0,
 					     &et,
 					     handle,
-					     data_ac,
-					     meta_ac,
+					     ctxt->data_ac,
+					     ctxt->meta_ac,
 					     &why);
 	if ((status < 0) && (status != -EAGAIN)) {
 		if (status != -ENOSPC)
@@ -257,8 +254,9 @@ restarted_transaction:
 
 	if (why != RESTART_NONE && clusters_to_add) {
 		if (why == RESTART_META) {
-			mlog(0, "restarting function.\n");
-			restart_func = 1;
+			mlog(0, "haven't reserved meta for xattr set.\n");
+			status = -ENOSPC;
+			goto leave;
 		} else {
 			BUG_ON(why != RESTART_TRANS);
 
@@ -280,62 +278,30 @@ restarted_transaction:
 	}
 
 leave:
-	if (handle) {
-		ocfs2_commit_trans(osb, handle);
-		handle = NULL;
-	}
-	if (data_ac) {
-		ocfs2_free_alloc_context(data_ac);
-		data_ac = NULL;
-	}
-	if (meta_ac) {
-		ocfs2_free_alloc_context(meta_ac);
-		meta_ac = NULL;
-	}
-	if ((!status) && restart_func) {
-		restart_func = 0;
-		goto restart_all;
-	}
 
 	return status;
 }
 
 static int __ocfs2_remove_xattr_range(struct inode *inode,
+				      handle_t *handle,
 				      struct buffer_head *root_bh,
 				      struct ocfs2_xattr_value_root *xv,
 				      u32 cpos, u32 phys_cpos, u32 len,
-				      struct ocfs2_cached_dealloc_ctxt *dealloc)
+				      struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	int ret;
 	u64 phys_blkno = ocfs2_clusters_to_blocks(inode->i_sb, phys_cpos);
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct inode *tl_inode = osb->osb_tl_inode;
-	handle_t *handle;
-	struct ocfs2_alloc_context *meta_ac = NULL;
 	struct ocfs2_extent_tree et;
 
 	ocfs2_init_xattr_value_extent_tree(&et, inode, root_bh, xv);
 
-	ret = ocfs2_lock_allocators(inode, &et, 0, 1, NULL, &meta_ac);
+	ret = ocfs2_extend_trans(handle,
+				 OCFS2_REMOVE_EXTENT_CREDITS +
+				 handle->h_buffer_credits);
 	if (ret) {
 		mlog_errno(ret);
-		return ret;
-	}
-
-	mutex_lock(&tl_inode->i_mutex);
-
-	if (ocfs2_truncate_log_needs_flush(osb)) {
-		ret = __ocfs2_flush_truncate_log(osb);
-		if (ret < 0) {
-			mlog_errno(ret);
-			goto out;
-		}
-	}
-
-	handle = ocfs2_start_trans(osb, OCFS2_REMOVE_EXTENT_CREDITS);
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		mlog_errno(ret);
 		goto out;
 	}
 
@@ -343,14 +309,14 @@ static int __ocfs2_remove_xattr_range(struct inode *inode,
 				   OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
-		goto out_commit;
+		goto out;
 	}
 
-	ret = ocfs2_remove_extent(inode, &et, cpos, len, handle, meta_ac,
-				  dealloc);
+	ret = ocfs2_remove_extent(inode, &et, cpos, len, handle, ctxt->meta_ac,
+				  &ctxt->dealloc);
 	if (ret) {
 		mlog_errno(ret);
-		goto out_commit;
+		goto out;
 	}
 
 	le32_add_cpu(&xv->xr_clusters, -len);
@@ -358,37 +324,30 @@ static int __ocfs2_remove_xattr_range(struct inode *inode,
 	ret = ocfs2_journal_dirty(handle, root_bh);
 	if (ret) {
 		mlog_errno(ret);
-		goto out_commit;
+		goto out;
 	}
 
+	mutex_lock(&tl_inode->i_mutex);
 	ret = ocfs2_truncate_log_append(osb, handle, phys_blkno, len);
 	if (ret)
 		mlog_errno(ret);
+	 mutex_unlock(&tl_inode->i_mutex);
 
-out_commit:
-	ocfs2_commit_trans(osb, handle);
 out:
-	mutex_unlock(&tl_inode->i_mutex);
-
-	if (meta_ac)
-		ocfs2_free_alloc_context(meta_ac);
-
 	return ret;
 }
 
 static int ocfs2_xattr_shrink_size(struct inode *inode,
+				   handle_t *handle,
 				   u32 old_clusters,
 				   u32 new_clusters,
 				   struct buffer_head *root_bh,
-				   struct ocfs2_xattr_value_root *xv)
+				   struct ocfs2_xattr_value_root *xv,
+				   struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	int ret = 0;
 	u32 trunc_len, cpos, phys_cpos, alloc_size;
 	u64 block;
-	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
-	struct ocfs2_cached_dealloc_ctxt dealloc;
-
-	ocfs2_init_dealloc_ctxt(&dealloc);
 
 	if (old_clusters <= new_clusters)
 		return 0;
@@ -406,9 +365,10 @@ static int ocfs2_xattr_shrink_size(struct inode *inode,
 		if (alloc_size > trunc_len)
 			alloc_size = trunc_len;
 
-		ret = __ocfs2_remove_xattr_range(inode, root_bh, xv, cpos,
+		ret = __ocfs2_remove_xattr_range(inode, handle, root_bh,
+						 xv, cpos,
 						 phys_cpos, alloc_size,
-						 &dealloc);
+						 ctxt);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
@@ -422,16 +382,15 @@ static int ocfs2_xattr_shrink_size(struct inode *inode,
 	}
 
 out:
-	ocfs2_schedule_truncate_log_flush(osb, 1);
-	ocfs2_run_deallocs(osb, &dealloc);
-
 	return ret;
 }
 
 static int ocfs2_xattr_value_truncate(struct inode *inode,
+				      handle_t *handle,
 				      struct buffer_head *root_bh,
 				      struct ocfs2_xattr_value_root *xv,
-				      int len)
+				      int len,
+				      struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	int ret;
 	u32 new_clusters = ocfs2_clusters_for_bytes(inode->i_sb, len);
@@ -442,12 +401,14 @@ static int ocfs2_xattr_value_truncate(struct inode *inode,
 
 	if (new_clusters > old_clusters)
 		ret = ocfs2_xattr_extend_allocation(inode,
+						    handle,
 						    new_clusters - old_clusters,
-						    root_bh, xv);
+						    root_bh, xv, ctxt);
 	else
 		ret = ocfs2_xattr_shrink_size(inode,
+					      handle,
 					      old_clusters, new_clusters,
-					      root_bh, xv);
+					      root_bh, xv, ctxt);
 
 	return ret;
 }
@@ -885,6 +846,7 @@ int ocfs2_xattr_get(struct inode *inode,
 }
 
 static int __ocfs2_xattr_set_value_outside(struct inode *inode,
+					   handle_t *handle,
 					   struct ocfs2_xattr_value_root *xv,
 					   const void *value,
 					   int value_len)
@@ -896,14 +858,12 @@ static int __ocfs2_xattr_set_value_outside(struct inode *inode,
 	u32 clusters = ocfs2_clusters_for_bytes(inode->i_sb, value_len);
 	u64 blkno;
 	struct buffer_head *bh = NULL;
-	handle_t *handle;
 
 	BUG_ON(clusters > le32_to_cpu(xv->xr_clusters));
 
 	credits = clusters * bpc;
-	handle = ocfs2_start_trans(OCFS2_SB(inode->i_sb), credits);
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
+	ret = ocfs2_extend_trans(handle, credits);
+	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
@@ -913,7 +873,7 @@ static int __ocfs2_xattr_set_value_outside(struct inode *inode,
 					       &num_clusters, &xv->xr_list);
 		if (ret) {
 			mlog_errno(ret);
-			goto out_commit;
+			goto out;
 		}
 
 		blkno = ocfs2_clusters_to_blocks(inode->i_sb, p_cluster);
@@ -922,7 +882,7 @@ static int __ocfs2_xattr_set_value_outside(struct inode *inode,
 			ret = ocfs2_read_block(inode, blkno, &bh);
 			if (ret) {
 				mlog_errno(ret);
-				goto out_commit;
+				goto out;
 			}
 
 			ret = ocfs2_journal_access(handle,
@@ -931,7 +891,7 @@ static int __ocfs2_xattr_set_value_outside(struct inode *inode,
 						   OCFS2_JOURNAL_ACCESS_WRITE);
 			if (ret < 0) {
 				mlog_errno(ret);
-				goto out_commit;
+				goto out;
 			}
 
 			cp_len = value_len > blocksize ? blocksize : value_len;
@@ -945,7 +905,7 @@ static int __ocfs2_xattr_set_value_outside(struct inode *inode,
 			ret = ocfs2_journal_dirty(handle, bh);
 			if (ret < 0) {
 				mlog_errno(ret);
-				goto out_commit;
+				goto out;
 			}
 			brelse(bh);
 			bh = NULL;
@@ -959,8 +919,6 @@ static int __ocfs2_xattr_set_value_outside(struct inode *inode,
 		}
 		cpos += num_clusters;
 	}
-out_commit:
-	ocfs2_commit_trans(OCFS2_SB(inode->i_sb), handle);
 out:
 	brelse(bh);
 
@@ -968,28 +926,21 @@ out:
 }
 
 static int ocfs2_xattr_cleanup(struct inode *inode,
+			       handle_t *handle,
 			       struct ocfs2_xattr_info *xi,
 			       struct ocfs2_xattr_search *xs,
 			       size_t offs)
 {
-	handle_t *handle = NULL;
 	int ret = 0;
 	size_t name_len = strlen(xi->name);
 	void *val = xs->base + offs;
 	size_t size = OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_ROOT_SIZE;
 
-	handle = ocfs2_start_trans((OCFS2_SB(inode->i_sb)),
-				   OCFS2_XATTR_BLOCK_UPDATE_CREDITS);
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		mlog_errno(ret);
-		goto out;
-	}
 	ret = ocfs2_journal_access(handle, inode, xs->xattr_bh,
 				   OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
-		goto out_commit;
+		goto out;
 	}
 	/* Decrease xattr count */
 	le16_add_cpu(&xs->header->xh_count, -1);
@@ -1000,32 +951,29 @@ static int ocfs2_xattr_cleanup(struct inode *inode,
 	ret = ocfs2_journal_dirty(handle, xs->xattr_bh);
 	if (ret < 0)
 		mlog_errno(ret);
-out_commit:
-	ocfs2_commit_trans(OCFS2_SB(inode->i_sb), handle);
 out:
 	return ret;
 }
 
 static int ocfs2_xattr_update_entry(struct inode *inode,
+				    handle_t *handle,
 				    struct ocfs2_xattr_info *xi,
 				    struct ocfs2_xattr_search *xs,
 				    size_t offs)
 {
-	handle_t *handle = NULL;
-	int ret = 0;
+	int ret;
 
-	handle = ocfs2_start_trans((OCFS2_SB(inode->i_sb)),
-				   OCFS2_XATTR_BLOCK_UPDATE_CREDITS);
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
+	ret = ocfs2_extend_trans(handle, OCFS2_XATTR_BLOCK_UPDATE_CREDITS);
+	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
+
 	ret = ocfs2_journal_access(handle, inode, xs->xattr_bh,
 				   OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
-		goto out_commit;
+		goto out;
 	}
 
 	xs->here->xe_name_offset = cpu_to_le16(offs);
@@ -1039,8 +987,6 @@ static int ocfs2_xattr_update_entry(struct inode *inode,
 	ret = ocfs2_journal_dirty(handle, xs->xattr_bh);
 	if (ret < 0)
 		mlog_errno(ret);
-out_commit:
-	ocfs2_commit_trans(OCFS2_SB(inode->i_sb), handle);
 out:
 	return ret;
 }
@@ -1051,8 +997,10 @@ out:
  * Set large size value in B tree.
  */
 static int ocfs2_xattr_set_value_outside(struct inode *inode,
+					 handle_t *handle,
 					 struct ocfs2_xattr_info *xi,
 					 struct ocfs2_xattr_search *xs,
+					 struct ocfs2_xattr_set_ctxt *ctxt,
 					 size_t offs)
 {
 	size_t name_len = strlen(xi->name);
@@ -1071,19 +1019,20 @@ static int ocfs2_xattr_set_value_outside(struct inode *inode,
 	xv->xr_list.l_count = cpu_to_le16(1);
 	xv->xr_list.l_next_free_rec = 0;
 
-	ret = ocfs2_xattr_value_truncate(inode, xs->xattr_bh, xv,
-					 xi->value_len);
+	ret = ocfs2_xattr_value_truncate(inode, handle, xs->xattr_bh, xv,
+					 xi->value_len, ctxt);
+
 	if (ret < 0) {
 		mlog_errno(ret);
 		return ret;
 	}
-	ret = __ocfs2_xattr_set_value_outside(inode, xv, xi->value,
-					      xi->value_len);
+	ret = __ocfs2_xattr_set_value_outside(inode, handle, xv,
+					      xi->value, xi->value_len);
 	if (ret < 0) {
 		mlog_errno(ret);
 		return ret;
 	}
-	ret = ocfs2_xattr_update_entry(inode, xi, xs, offs);
+	ret = ocfs2_xattr_update_entry(inode, handle, xi, xs, offs);
 	if (ret < 0)
 		mlog_errno(ret);
 
@@ -1201,8 +1150,10 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
  * then set value in B tree with set_value_outside().
  */
 static int ocfs2_xattr_set_entry(struct inode *inode,
+				 handle_t *handle,
 				 struct ocfs2_xattr_info *xi,
 				 struct ocfs2_xattr_search *xs,
+				 struct ocfs2_xattr_set_ctxt *ctxt,
 				 int flag)
 {
 	struct ocfs2_xattr_entry *last;
@@ -1210,7 +1161,6 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
 	struct ocfs2_dinode *di = (struct ocfs2_dinode *)xs->inode_bh->b_data;
 	size_t min_offs = xs->end - xs->base, name_len = strlen(xi->name);
 	size_t size_l = 0;
-	handle_t *handle = NULL;
 	int free, i, ret;
 	struct ocfs2_xattr_info xi_l = {
 		.name_index = xi->name_index,
@@ -1272,8 +1222,9 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
 
 		if (ocfs2_xattr_is_local(xs->here) && size == size_l) {
 			/* Replace existing local xattr with tree root */
-			ret = ocfs2_xattr_set_value_outside(inode, xi, xs,
-							    offs);
+			ret = ocfs2_xattr_set_value_outside(inode, handle,
+							    xi, xs,
+							    ctxt, offs);
 			if (ret < 0)
 				mlog_errno(ret);
 			goto out;
@@ -1290,15 +1241,18 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
 				 * then set new value with set_value_outside().
 				 */
 				ret = ocfs2_xattr_value_truncate(inode,
+								 handle,
 								 xs->xattr_bh,
 								 xv,
-								 xi->value_len);
+								 xi->value_len,
+								 ctxt);
 				if (ret < 0) {
 					mlog_errno(ret);
 					goto out;
 				}
 
 				ret = __ocfs2_xattr_set_value_outside(inode,
+								handle,
 								xv,
 								xi->value,
 								xi->value_len);
@@ -1308,6 +1262,7 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
 				}
 
 				ret = ocfs2_xattr_update_entry(inode,
+							       handle,
 							       xi,
 							       xs,
 							       offs);
@@ -1320,44 +1275,30 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
 				 * just trucate old value to zero.
 				 */
 				 ret = ocfs2_xattr_value_truncate(inode,
-								 xs->xattr_bh,
-								 xv,
-								 0);
+								  handle,
+								  xs->xattr_bh,
+								  xv,
+								  0,
+								  ctxt);
 				if (ret < 0)
 					mlog_errno(ret);
 			}
 		}
 	}
 
-	handle = ocfs2_start_trans((OCFS2_SB(inode->i_sb)),
-				   OCFS2_INODE_UPDATE_CREDITS);
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		mlog_errno(ret);
-		goto out;
-	}
-
 	ret = ocfs2_journal_access(handle, inode, xs->inode_bh,
 				   OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
-		goto out_commit;
+		goto out;
 	}
 
 	if (!(flag & OCFS2_INLINE_XATTR_FL)) {
-		/* set extended attribute in external block. */
-		ret = ocfs2_extend_trans(handle,
-					 OCFS2_INODE_UPDATE_CREDITS +
-					 OCFS2_XATTR_BLOCK_UPDATE_CREDITS);
-		if (ret) {
-			mlog_errno(ret);
-			goto out_commit;
-		}
 		ret = ocfs2_journal_access(handle, inode, xs->xattr_bh,
 					   OCFS2_JOURNAL_ACCESS_WRITE);
 		if (ret) {
 			mlog_errno(ret);
-			goto out_commit;
+			goto out;
 		}
 	}
 
@@ -1371,7 +1312,7 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
 		ret = ocfs2_journal_dirty(handle, xs->xattr_bh);
 		if (ret < 0) {
 			mlog_errno(ret);
-			goto out_commit;
+			goto out;
 		}
 	}
 
@@ -1408,16 +1349,14 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
 	if (ret < 0)
 		mlog_errno(ret);
 
-out_commit:
-	ocfs2_commit_trans(OCFS2_SB(inode->i_sb), handle);
-
 	if (!ret && xi->value_len > OCFS2_XATTR_INLINE_SIZE) {
 		/*
 		 * Set value outside in B tree.
 		 * This is the second step for value size > INLINE_SIZE.
 		 */
 		size_t offs = le16_to_cpu(xs->here->xe_name_offset);
-		ret = ocfs2_xattr_set_value_outside(inode, xi, xs, offs);
+		ret = ocfs2_xattr_set_value_outside(inode, handle, xi, xs,
+						    ctxt, offs);
 		if (ret < 0) {
 			int ret2;
 
@@ -1426,14 +1365,13 @@ out_commit:
 			 * If set value outside failed, we have to clean
 			 * the junk tree root we have already set in local.
 			 */
-			ret2 = ocfs2_xattr_cleanup(inode, xi, xs, offs);
+			ret2 = ocfs2_xattr_cleanup(inode, handle, xi, xs, offs);
 			if (ret2 < 0)
 				mlog_errno(ret2);
 		}
 	}
 out:
 	return ret;
-
 }
 
 static int ocfs2_remove_value_outside(struct inode*inode,
@@ -1441,6 +1379,23 @@ static int ocfs2_remove_value_outside(struct inode*inode,
 				      struct ocfs2_xattr_header *header)
 {
 	int ret = 0, i;
+	handle_t *handle = NULL;
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	struct ocfs2_xattr_set_ctxt ctxt;
+
+	memset(&ctxt, 0, sizeof(ctxt));
+	ocfs2_init_dealloc_ctxt(&ctxt.dealloc);
+
+	/*
+	 * actually ofs2_xattr_value_truncate will extend trans
+	 * if needed, so just 1 is enough.
+	 */
+	handle = ocfs2_start_trans(osb, 1);
+	if (IS_ERR(handle)) {
+		ret = PTR_ERR(handle);
+		mlog_errno(ret);
+		goto out;
+	}
 
 	for (i = 0; i < le16_to_cpu(header->xh_count); i++) {
 		struct ocfs2_xattr_entry *entry = &header->xh_entries[i];
@@ -1453,14 +1408,19 @@ static int ocfs2_remove_value_outside(struct inode*inode,
 				le16_to_cpu(entry->xe_name_offset);
 			xv = (struct ocfs2_xattr_value_root *)
 				(val + OCFS2_XATTR_SIZE(entry->xe_name_len));
-			ret = ocfs2_xattr_value_truncate(inode, bh, xv, 0);
+			ret = ocfs2_xattr_value_truncate(inode, handle,
+							 bh, xv, 0, &ctxt);
 			if (ret < 0) {
 				mlog_errno(ret);
-				return ret;
+				break;
 			}
 		}
 	}
 
+	ret = ocfs2_commit_trans(osb, handle);
+	ocfs2_schedule_truncate_log_flush(osb, 1);
+	ocfs2_run_deallocs(osb, &ctxt.dealloc);
+out:
 	return ret;
 }
 
@@ -1723,8 +1683,10 @@ static int ocfs2_xattr_ibody_find(struct inode *inode,
  *
  */
 static int ocfs2_xattr_ibody_set(struct inode *inode,
+				 handle_t *handle,
 				 struct ocfs2_xattr_info *xi,
-				 struct ocfs2_xattr_search *xs)
+				 struct ocfs2_xattr_search *xs,
+				 struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
 	struct ocfs2_dinode *di = (struct ocfs2_dinode *)xs->inode_bh->b_data;
@@ -1741,7 +1703,7 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
 		}
 	}
 
-	ret = ocfs2_xattr_set_entry(inode, xi, xs,
+	ret = ocfs2_xattr_set_entry(inode, handle, xi, xs, ctxt,
 				(OCFS2_INLINE_XATTR_FL | OCFS2_HAS_XATTR_FL));
 out:
 	up_write(&oi->ip_alloc_sem);
@@ -1815,14 +1777,14 @@ cleanup:
  *
  */
 static int ocfs2_xattr_block_set(struct inode *inode,
+				 handle_t *handle,
 				 struct ocfs2_xattr_info *xi,
-				 struct ocfs2_xattr_search *xs)
+				 struct ocfs2_xattr_search *xs,
+				 struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	struct buffer_head *new_bh = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct ocfs2_dinode *di =  (struct ocfs2_dinode *)xs->inode_bh->b_data;
-	struct ocfs2_alloc_context *meta_ac = NULL;
-	handle_t *handle = NULL;
 	struct ocfs2_xattr_block *xblk = NULL;
 	u16 suballoc_bit_start;
 	u32 num_got;
@@ -1830,35 +1792,26 @@ static int ocfs2_xattr_block_set(struct inode *inode,
 	int ret;
 
 	if (!xs->xattr_bh) {
-		/*
-		 * Alloc one external block for extended attribute
-		 * outside of inode.
-		 */
-		ret = ocfs2_reserve_new_metadata_blocks(osb, 1, &meta_ac);
-		if (ret < 0) {
-			mlog_errno(ret);
-			goto out;
-		}
-		handle = ocfs2_start_trans(osb,
-					   OCFS2_XATTR_BLOCK_CREATE_CREDITS);
-		if (IS_ERR(handle)) {
-			ret = PTR_ERR(handle);
+		ret = ocfs2_extend_trans(handle,
+					 OCFS2_XATTR_BLOCK_CREATE_CREDITS);
+		if (ret) {
 			mlog_errno(ret);
 			goto out;
 		}
+
 		ret = ocfs2_journal_access(handle, inode, xs->inode_bh,
 					   OCFS2_JOURNAL_ACCESS_CREATE);
 		if (ret < 0) {
 			mlog_errno(ret);
-			goto out_commit;
+			goto out;
 		}
 
-		ret = ocfs2_claim_metadata(osb, handle, meta_ac, 1,
+		ret = ocfs2_claim_metadata(osb, handle, ctxt->meta_ac, 1,
 					   &suballoc_bit_start, &num_got,
 					   &first_blkno);
 		if (ret < 0) {
 			mlog_errno(ret);
-			goto out_commit;
+			goto out;
 		}
 
 		new_bh = sb_getblk(inode->i_sb, first_blkno);
@@ -1868,7 +1821,7 @@ static int ocfs2_xattr_block_set(struct inode *inode,
 					   OCFS2_JOURNAL_ACCESS_CREATE);
 		if (ret < 0) {
 			mlog_errno(ret);
-			goto out_commit;
+			goto out;
 		}
 
 		/* Initialize ocfs2_xattr_block */
@@ -1886,44 +1839,249 @@ static int ocfs2_xattr_block_set(struct inode *inode,
 		xs->end = (void *)xblk + inode->i_sb->s_blocksize;
 		xs->here = xs->header->xh_entries;
 
-
 		ret = ocfs2_journal_dirty(handle, new_bh);
 		if (ret < 0) {
 			mlog_errno(ret);
-			goto out_commit;
+			goto out;
 		}
 		di->i_xattr_loc = cpu_to_le64(first_blkno);
-		ret = ocfs2_journal_dirty(handle, xs->inode_bh);
-		if (ret < 0)
-			mlog_errno(ret);
-out_commit:
-		ocfs2_commit_trans(osb, handle);
-out:
-		if (meta_ac)
-			ocfs2_free_alloc_context(meta_ac);
-		if (ret < 0)
-			return ret;
+		ocfs2_journal_dirty(handle, xs->inode_bh);
 	} else
 		xblk = (struct ocfs2_xattr_block *)xs->xattr_bh->b_data;
 
 	if (!(le16_to_cpu(xblk->xb_flags) & OCFS2_XATTR_INDEXED)) {
 		/* Set extended attribute into external block */
-		ret = ocfs2_xattr_set_entry(inode, xi, xs, OCFS2_HAS_XATTR_FL);
+		ret = ocfs2_xattr_set_entry(inode, handle, xi, xs, ctxt,
+					    OCFS2_HAS_XATTR_FL);
 		if (!ret || ret != -ENOSPC)
-			goto end;
+			goto out;
 
-		ret = ocfs2_xattr_create_index_block(inode, xs);
+		ret = ocfs2_xattr_create_index_block(inode, handle, xs, ctxt);
 		if (ret)
-			goto end;
+			goto out;
 	}
 
-	ret = ocfs2_xattr_set_entry_index_block(inode, xi, xs);
+	ret = ocfs2_xattr_set_entry_index_block(inode, handle, xi, xs, ctxt);
 
-end:
+out:
 
 	return ret;
 }
 
+static int ocfs2_calc_xattr_set_credits(struct inode *inode,
+					struct ocfs2_xattr_info *xi,
+					struct ocfs2_xattr_search *xis,
+					struct ocfs2_xattr_search *xbs)
+{
+	int credits = OCFS2_INODE_UPDATE_CREDITS;
+
+	/* calculate xattr metadata update credits. */
+	if (xbs->xattr_bh) {
+		struct ocfs2_xattr_block *xb =
+			(struct ocfs2_xattr_block *)xbs->xattr_bh->b_data;
+
+		if (!(le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED))
+			credits += OCFS2_XATTR_BLOCK_UPDATE_CREDITS;
+		else
+			credits += ocfs2_blocks_per_xattr_bucket(inode->i_sb);
+	}
+
+	return credits;
+}
+
+static int ocfs2_init_xattr_set_ctxt(struct inode *inode,
+				     struct ocfs2_dinode *di,
+				     struct ocfs2_xattr_info *xi,
+				     struct ocfs2_xattr_search *xis,
+				     struct ocfs2_xattr_search *xbs,
+				     struct ocfs2_xattr_set_ctxt *ctxt)
+{
+	int ret = 0, clusters_add = 0, meta_add = 0;
+	struct buffer_head *bh = NULL;
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	struct ocfs2_xattr_block *xb = NULL;
+	struct ocfs2_xattr_entry *xe;
+
+	memset(ctxt, 0, sizeof(struct ocfs2_xattr_set_ctxt));
+
+	ocfs2_init_dealloc_ctxt(&ctxt->dealloc);
+
+	if (di->i_xattr_loc) {
+		if (!xbs->xattr_bh) {
+			ret = ocfs2_read_block(inode,
+					       le64_to_cpu(di->i_xattr_loc),
+					       &bh);
+			if (ret) {
+				mlog_errno(ret);
+				goto out;
+			}
+
+			xb = (struct ocfs2_xattr_block *)bh->b_data;
+		} else
+			xb = (struct ocfs2_xattr_block *)xbs->xattr_bh->b_data;
+
+		if (le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED) {
+			struct ocfs2_extent_list *el =
+				 &xb->xb_attrs.xb_root.xt_list;
+			meta_add += ocfs2_extend_meta_needed(el);
+		}
+		/*
+		 * This cluster will be used either for new bucket or for
+		 * new xattr block.
+		 * If the cluster size is the same as the bucket size, one
+		 * more is needed since we may need to extend the bucket
+		 * also.
+		 */
+		clusters_add += 1;
+		if (OCFS2_XATTR_BUCKET_SIZE == osb->s_clustersize)
+			clusters_add += 1;
+	} else
+		meta_add += 1;
+
+	/* calculate xattr value update. */
+	if (xi->value && xi->value_len > OCFS2_XATTR_INLINE_SIZE) {
+		char *base;
+		int name_offset, name_len, block_off, i;
+		u32 new_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
+							    xi->value_len);
+
+		xe = NULL;
+		if (!xis->not_found) {
+			xe = xis->here;
+			name_offset = le16_to_cpu(xe->xe_name_offset);
+			name_len = OCFS2_XATTR_SIZE(xe->xe_name_len);
+			base = xis->base;
+		} else if (!xbs->not_found) {
+			xe = xbs->here;
+			name_offset = le16_to_cpu(xe->xe_name_offset);
+			name_len = OCFS2_XATTR_SIZE(xe->xe_name_len);
+			i = xbs->here - xbs->header->xh_entries;
+
+			if (le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED) {
+				ret = ocfs2_xattr_bucket_get_name_value(inode,
+								xbs->bucket.xh,
+								i,
+								&block_off,
+								&name_offset);
+				base = xbs->bucket.bhs[block_off]->b_data;
+			} else
+				base = xbs->base;
+		}
+
+		/* calc value clusters and value tree metadata change. */
+		if (!xe ||
+		    (le64_to_cpu(xe->xe_value_size) <= OCFS2_XATTR_INLINE_SIZE))
+			clusters_add += new_clusters;
+		else {
+			u32 old_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
+						le64_to_cpu(xe->xe_value_size));
+
+			if (old_clusters < new_clusters) {
+				struct ocfs2_xattr_value_root *xv =
+					(struct ocfs2_xattr_value_root *)
+					(base + name_offset + name_len);
+				meta_add +=
+					ocfs2_extend_meta_needed(&xv->xr_list);
+				clusters_add += new_clusters - old_clusters;
+
+			}
+		}
+	}
+
+	mlog(0, "Set xattr %s, reserve meta blocks = %d, clusters = %d\n",
+	     xi->name, meta_add, clusters_add);
+	if (meta_add) {
+		ret = ocfs2_reserve_new_metadata_blocks(osb, meta_add,
+							&ctxt->meta_ac);
+		if (ret)
+			mlog_errno(ret);
+	}
+
+	if (clusters_add) {
+		ret = ocfs2_reserve_clusters(osb, clusters_add, &ctxt->data_ac);
+		if (ret) {
+			mlog_errno(ret);
+			goto out;
+		}
+	}
+
+out:
+	return ret;
+}
+
+static int __ocfs2_xattr_set_handle(struct inode *inode,
+				    handle_t *handle,
+				    struct ocfs2_dinode *di,
+				    struct ocfs2_xattr_info *xi,
+				    struct ocfs2_xattr_search *xis,
+				    struct ocfs2_xattr_search *xbs,
+				    struct ocfs2_xattr_set_ctxt *ctxt)
+{
+	int ret = 0, credits;
+
+	if (!xi->value) {
+		/* Remove existing extended attribute */
+		if (!xis->not_found)
+			ret = ocfs2_xattr_ibody_set(inode, handle,
+						    xi, xis, ctxt);
+		else if (!xbs->not_found)
+			ret = ocfs2_xattr_block_set(inode, handle,
+						    xi, xbs, ctxt);
+	} else {
+		/* We always try to set extended attribute into inode first*/
+		ret = ocfs2_xattr_ibody_set(inode, handle, xi, xis, ctxt);
+		if (!ret && !xbs->not_found) {
+			/*
+			 * If succeed and that extended attribute existing in
+			 * external block, then we will remove it.
+			 */
+			xi->value = NULL;
+			xi->value_len = 0;
+			ret = ocfs2_xattr_block_set(inode, handle,
+						    xi, xbs, ctxt);
+		} else if (ret == -ENOSPC) {
+			if (di->i_xattr_loc && !xbs->xattr_bh) {
+				ret = ocfs2_xattr_block_find(inode,
+							     xi->name_index,
+							     xi->name, xbs);
+				if (ret)
+					goto out;
+
+				credits = ocfs2_calc_xattr_set_credits(inode,
+								       xi,
+								       xis,
+								       xbs);
+				ret = ocfs2_extend_trans(handle, credits);
+				if (ret) {
+					mlog_errno(ret);
+					goto out;
+				}
+			}
+			/*
+			 * If no space in inode, we will set extended attribute
+			 * into external block.
+			 */
+			ret = ocfs2_xattr_block_set(inode, handle,
+						    xi, xbs, ctxt);
+			if (ret)
+				goto out;
+			if (!xis->not_found) {
+				/*
+				 * If succeed and that extended attribute
+				 * existing in inode, we will remove it.
+				 */
+				xi->value = NULL;
+				xi->value_len = 0;
+				ret = ocfs2_xattr_ibody_set(inode, handle,
+							    xi, xis, ctxt);
+			}
+		}
+	}
+
+out:
+	return ret;
+}
+
 /*
  * ocfs2_xattr_set()
  *
@@ -1940,8 +2098,12 @@ int ocfs2_xattr_set(struct inode *inode,
 {
 	struct buffer_head *di_bh = NULL;
 	struct ocfs2_dinode *di;
-	int ret;
+	int ret, credits;
 	u16 i, blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
+	struct ocfs2_xattr_set_ctxt ctxt;
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	struct inode *tl_inode = osb->osb_tl_inode;
+	handle_t *handle = NULL;
 
 	struct ocfs2_xattr_info xi = {
 		.name_index = name_index,
@@ -1996,48 +2158,45 @@ int ocfs2_xattr_set(struct inode *inode,
 			goto cleanup;
 	}
 
-	if (!value) {
-		/* Remove existing extended attribute */
-		if (!xis.not_found)
-			ret = ocfs2_xattr_ibody_set(inode, &xi, &xis);
-		else if (!xbs.not_found)
-			ret = ocfs2_xattr_block_set(inode, &xi, &xbs);
-	} else {
-		/* We always try to set extended attribute into inode first*/
-		ret = ocfs2_xattr_ibody_set(inode, &xi, &xis);
-		if (!ret && !xbs.not_found) {
-			/*
-			 * If succeed and that extended attribute existing in
-			 * external block, then we will remove it.
-			 */
-			xi.value = NULL;
-			xi.value_len = 0;
-			ret = ocfs2_xattr_block_set(inode, &xi, &xbs);
-		} else if (ret == -ENOSPC) {
-			if (di->i_xattr_loc && !xbs.xattr_bh) {
-				ret = ocfs2_xattr_block_find(inode, name_index,
-							     name, &xbs);
-				if (ret)
-					goto cleanup;
-			}
-			/*
-			 * If no space in inode, we will set extended attribute
-			 * into external block.
-			 */
-			ret = ocfs2_xattr_block_set(inode, &xi, &xbs);
-			if (ret)
-				goto cleanup;
-			if (!xis.not_found) {
-				/*
-				 * If succeed and that extended attribute
-				 * existing in inode, we will remove it.
-				 */
-				xi.value = NULL;
-				xi.value_len = 0;
-				ret = ocfs2_xattr_ibody_set(inode, &xi, &xis);
-			}
+
+	mutex_lock(&tl_inode->i_mutex);
+
+	if (ocfs2_truncate_log_needs_flush(osb)) {
+		ret = __ocfs2_flush_truncate_log(osb);
+		if (ret < 0) {
+			mutex_unlock(&tl_inode->i_mutex);
+			mlog_errno(ret);
+			goto cleanup;
 		}
 	}
+	mutex_unlock(&tl_inode->i_mutex);
+
+	ret = ocfs2_init_xattr_set_ctxt(inode, di, &xi, &xis, &xbs, &ctxt);
+	if (ret) {
+		mlog_errno(ret);
+		goto cleanup;
+	}
+
+	credits = ocfs2_calc_xattr_set_credits(inode, &xi, &xis, &xbs);
+	handle = ocfs2_start_trans(osb, credits);
+	if (IS_ERR(handle)) {
+		ret = PTR_ERR(handle);
+		mlog_errno(ret);
+		goto cleanup;
+	}
+
+	ret = __ocfs2_xattr_set_handle(inode, handle, di,
+				       &xi, &xis, &xbs, &ctxt);
+
+	ocfs2_commit_trans(osb, handle);
+
+	if (ctxt.data_ac)
+		ocfs2_free_alloc_context(ctxt.data_ac);
+	if (ctxt.meta_ac)
+		ocfs2_free_alloc_context(ctxt.meta_ac);
+	ocfs2_schedule_truncate_log_flush(osb, 1);
+	ocfs2_run_deallocs(osb, &ctxt.dealloc);
+
 cleanup:
 	up_write(&OCFS2_I(inode)->ip_xattr_sem);
 	ocfs2_inode_unlock(inode, 1);
@@ -2667,15 +2826,15 @@ static int ocfs2_xattr_update_xattr_search(struct inode *inode,
 }
 
 static int ocfs2_xattr_create_index_block(struct inode *inode,
-					  struct ocfs2_xattr_search *xs)
+					  handle_t *handle,
+					  struct ocfs2_xattr_search *xs,
+					  struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	int ret, credits = OCFS2_SUBALLOC_ALLOC;
 	u32 bit_off, len;
 	u64 blkno;
-	handle_t *handle;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
-	struct ocfs2_alloc_context *data_ac;
 	struct buffer_head *xh_bh = NULL, *data_bh = NULL;
 	struct buffer_head *xb_bh = xs->xattr_bh;
 	struct ocfs2_xattr_block *xb =
@@ -2689,12 +2848,6 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
 
 	BUG_ON(xb_flags & OCFS2_XATTR_INDEXED);
 
-	ret = ocfs2_reserve_clusters(osb, 1, &data_ac);
-	if (ret) {
-		mlog_errno(ret);
-		goto out;
-	}
-
 	/*
 	 * XXX:
 	 * We can use this lock for now, and maybe move to a dedicated mutex
@@ -2707,24 +2860,24 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
 	 * of the new xattr bucket and one for the value/data.
 	 */
 	credits += 3;
-	handle = ocfs2_start_trans(osb, credits);
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
+	ret = ocfs2_extend_trans(handle, credits + handle->h_buffer_credits);
+	if (ret) {
 		mlog_errno(ret);
-		goto out_sem;
+		goto out;
 	}
 
 	ret = ocfs2_journal_access(handle, inode, xb_bh,
 				   OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
-		goto out_commit;
+		goto out;
 	}
 
-	ret = ocfs2_claim_clusters(osb, handle, data_ac, 1, &bit_off, &len);
+	ret = __ocfs2_claim_clusters(osb, handle, ctxt->data_ac,
+				     1, 1, &bit_off, &len);
 	if (ret) {
 		mlog_errno(ret);
-		goto out_commit;
+		goto out;
 	}
 
 	/*
@@ -2740,7 +2893,7 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
 	if (!xh_bh) {
 		ret = -EIO;
 		mlog_errno(ret);
-		goto out_commit;
+		goto out;
 	}
 
 	ocfs2_set_new_buffer_uptodate(inode, xh_bh);
@@ -2749,7 +2902,7 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
 				   OCFS2_JOURNAL_ACCESS_CREATE);
 	if (ret) {
 		mlog_errno(ret);
-		goto out_commit;
+		goto out;
 	}
 
 	if (bpb > 1) {
@@ -2757,7 +2910,7 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
 		if (!data_bh) {
 			ret = -EIO;
 			mlog_errno(ret);
-			goto out_commit;
+			goto out;
 		}
 
 		ocfs2_set_new_buffer_uptodate(inode, data_bh);
@@ -2766,7 +2919,7 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
 					   OCFS2_JOURNAL_ACCESS_CREATE);
 		if (ret) {
 			mlog_errno(ret);
-			goto out_commit;
+			goto out;
 		}
 	}
 
@@ -2795,21 +2948,10 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
 
 	xb->xb_flags = cpu_to_le16(xb_flags | OCFS2_XATTR_INDEXED);
 
-	ret = ocfs2_journal_dirty(handle, xb_bh);
-	if (ret) {
-		mlog_errno(ret);
-		goto out_commit;
-	}
-
-out_commit:
-	ocfs2_commit_trans(osb, handle);
-
-out_sem:
-	up_write(&oi->ip_alloc_sem);
+	ocfs2_journal_dirty(handle, xb_bh);
 
 out:
-	if (data_ac)
-		ocfs2_free_alloc_context(data_ac);
+	up_write(&oi->ip_alloc_sem);
 
 	brelse(xh_bh);
 	brelse(data_bh);
@@ -2837,6 +2979,7 @@ static int cmp_xe_offset(const void *a, const void *b)
  * so that we can spare some space for insertion.
  */
 static int ocfs2_defrag_xattr_bucket(struct inode *inode,
+				     handle_t *handle,
 				     struct ocfs2_xattr_bucket *bucket)
 {
 	int ret, i;
@@ -2847,9 +2990,9 @@ static int ocfs2_defrag_xattr_bucket(struct inode *inode,
 	u16 blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
 	u16 xh_free_start;
 	size_t blocksize = inode->i_sb->s_blocksize;
-	handle_t *handle;
 	struct buffer_head **bhs;
 	struct ocfs2_xattr_entry *xe;
+	int credits;
 
 	bhs = kzalloc(sizeof(struct buffer_head *) * blk_per_bucket,
 			GFP_NOFS);
@@ -2876,10 +3019,10 @@ static int ocfs2_defrag_xattr_bucket(struct inode *inode,
 	for (i = 0; i < blk_per_bucket; i++, buf += blocksize)
 		memcpy(buf, bhs[i]->b_data, blocksize);
 
-	handle = ocfs2_start_trans((OCFS2_SB(inode->i_sb)), blk_per_bucket);
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		handle = NULL;
+
+	credits = blk_per_bucket + handle->h_buffer_credits;
+	ret = ocfs2_extend_trans(handle, credits);
+	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
@@ -2889,7 +3032,7 @@ static int ocfs2_defrag_xattr_bucket(struct inode *inode,
 					   OCFS2_JOURNAL_ACCESS_WRITE);
 		if (ret < 0) {
 			mlog_errno(ret);
-			goto commit;
+			goto out;
 		}
 	}
 
@@ -2948,7 +3091,7 @@ static int ocfs2_defrag_xattr_bucket(struct inode *inode,
 			"bucket %llu\n", (unsigned long long)blkno);
 
 	if (xh_free_start == end)
-		goto commit;
+		goto out;
 
 	memset(bucket_buf + xh_free_start, 0, end - xh_free_start);
 	xh->xh_free_start = cpu_to_le16(end);
@@ -2964,8 +3107,6 @@ static int ocfs2_defrag_xattr_bucket(struct inode *inode,
 		ocfs2_journal_dirty(handle, bhs[i]);
 	}
 
-commit:
-	ocfs2_commit_trans(OCFS2_SB(inode->i_sb), handle);
 out:
 
 	if (bhs) {
@@ -3565,22 +3706,21 @@ static int ocfs2_adjust_xattr_cross_cluster(struct inode *inode,
  * when the header_bh is moved into the new cluster.
  */
 static int ocfs2_add_new_xattr_cluster(struct inode *inode,
+				       handle_t *handle,
 				       struct buffer_head *root_bh,
 				       struct buffer_head **first_bh,
 				       struct buffer_head **header_bh,
 				       u32 *num_clusters,
 				       u32 prev_cpos,
 				       u64 prev_blkno,
-				       int *extend)
+				       int *extend,
+				       struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	int ret, credits;
 	u16 bpc = ocfs2_clusters_to_blocks(inode->i_sb, 1);
 	u32 prev_clusters = *num_clusters;
 	u32 clusters_to_add = 1, bit_off, num_bits, v_start = 0;
 	u64 block;
-	handle_t *handle = NULL;
-	struct ocfs2_alloc_context *data_ac = NULL;
-	struct ocfs2_alloc_context *meta_ac = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct ocfs2_extent_tree et;
 
@@ -3591,19 +3731,11 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 
 	ocfs2_init_xattr_tree_extent_tree(&et, inode, root_bh);
 
-	ret = ocfs2_lock_allocators(inode, &et, clusters_to_add, 0,
-				    &data_ac, &meta_ac);
-	if (ret) {
-		mlog_errno(ret);
-		goto leave;
-	}
-
 	credits = ocfs2_calc_extend_credits(osb->sb, et.et_root_el,
-					    clusters_to_add);
-	handle = ocfs2_start_trans(osb, credits);
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		handle = NULL;
+				clusters_to_add) + handle->h_buffer_credits;
+
+	ret = ocfs2_extend_trans(handle, credits);
+	if (ret) {
 		mlog_errno(ret);
 		goto leave;
 	}
@@ -3615,7 +3747,7 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 		goto leave;
 	}
 
-	ret = __ocfs2_claim_clusters(osb, handle, data_ac, 1,
+	ret = __ocfs2_claim_clusters(osb, handle, ctxt->data_ac, 1,
 				     clusters_to_add, &bit_off, &num_bits);
 	if (ret < 0) {
 		if (ret != -ENOSPC)
@@ -3676,26 +3808,17 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 	mlog(0, "Insert %u clusters at block %llu for xattr@%u\n",
 	     num_bits, block, v_start);
 	ret = ocfs2_insert_extent(osb, handle, inode, &et, v_start, block,
-				  num_bits, 0, meta_ac);
+				  num_bits, 0, ctxt->meta_ac);
 	if (ret < 0) {
 		mlog_errno(ret);
 		goto leave;
 	}
 
 	ret = ocfs2_journal_dirty(handle, root_bh);
-	if (ret < 0) {
+	if (ret < 0)
 		mlog_errno(ret);
-		goto leave;
-	}
 
 leave:
-	if (handle)
-		ocfs2_commit_trans(osb, handle);
-	if (data_ac)
-		ocfs2_free_alloc_context(data_ac);
-	if (meta_ac)
-		ocfs2_free_alloc_context(meta_ac);
-
 	return ret;
 }
 
@@ -3704,6 +3827,7 @@ leave:
  * We meet with start_bh. Only move half of the xattrs to the bucket after it.
  */
 static int ocfs2_extend_xattr_bucket(struct inode *inode,
+				     handle_t *handle,
 				     struct buffer_head *first_bh,
 				     struct buffer_head *start_bh,
 				     u32 num_clusters)
@@ -3713,7 +3837,6 @@ static int ocfs2_extend_xattr_bucket(struct inode *inode,
 	u16 blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
 	u64 start_blk = start_bh->b_blocknr, end_blk;
 	u32 num_buckets = num_clusters * ocfs2_xattr_buckets_per_cluster(osb);
-	handle_t *handle;
 	struct ocfs2_xattr_header *first_xh =
 				(struct ocfs2_xattr_header *)first_bh->b_data;
 	u16 bucket = le16_to_cpu(first_xh->xh_num_buckets);
@@ -3730,11 +3853,10 @@ static int ocfs2_extend_xattr_bucket(struct inode *inode,
 	 * We will touch all the buckets after the start_bh(include it).
 	 * Add one more bucket and modify the first_bh.
 	 */
-	credits = end_blk - start_blk + 2 * blk_per_bucket + 1;
-	handle = ocfs2_start_trans(osb, credits);
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		handle = NULL;
+	credits = end_blk - start_blk + 2 * blk_per_bucket + 1 +
+		  handle->h_buffer_credits;
+	ret = ocfs2_extend_trans(handle, credits);
+	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
@@ -3743,14 +3865,14 @@ static int ocfs2_extend_xattr_bucket(struct inode *inode,
 				   OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
-		goto commit;
+		goto out;
 	}
 
 	while (end_blk != start_blk) {
 		ret = ocfs2_cp_xattr_bucket(inode, handle, end_blk,
 					    end_blk + blk_per_bucket, 0);
 		if (ret)
-			goto commit;
+			goto out;
 		end_blk -= blk_per_bucket;
 	}
 
@@ -3761,8 +3883,6 @@ static int ocfs2_extend_xattr_bucket(struct inode *inode,
 	le16_add_cpu(&first_xh->xh_num_buckets, 1);
 	ocfs2_journal_dirty(handle, first_bh);
 
-commit:
-	ocfs2_commit_trans(osb, handle);
 out:
 	return ret;
 }
@@ -3777,8 +3897,10 @@ out:
  * header_bh and first_bh if the insert place is moved to the new cluster.
  */
 static int ocfs2_add_new_xattr_bucket(struct inode *inode,
+				      handle_t *handle,
 				      struct buffer_head *xb_bh,
-				      struct buffer_head *header_bh)
+				      struct buffer_head *header_bh,
+				      struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	struct ocfs2_xattr_header *first_xh = NULL;
 	struct buffer_head *first_bh = NULL;
@@ -3823,13 +3945,15 @@ static int ocfs2_add_new_xattr_bucket(struct inode *inode,
 
 	if (num_buckets == le16_to_cpu(first_xh->xh_num_buckets)) {
 		ret = ocfs2_add_new_xattr_cluster(inode,
+						  handle,
 						  xb_bh,
 						  &first_bh,
 						  &header_bh,
 						  &num_clusters,
 						  e_cpos,
 						  p_blkno,
-						  &extend);
+						  &extend,
+						  ctxt);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
@@ -3838,6 +3962,7 @@ static int ocfs2_add_new_xattr_bucket(struct inode *inode,
 
 	if (extend)
 		ret = ocfs2_extend_xattr_bucket(inode,
+						handle,
 						first_bh,
 						header_bh,
 						num_clusters);
@@ -4039,15 +4164,14 @@ static int ocfs2_xattr_bucket_handle_journal(struct inode *inode,
  * space for the xattr insertion.
  */
 static int ocfs2_xattr_set_entry_in_bucket(struct inode *inode,
+					   handle_t *handle,
 					   struct ocfs2_xattr_info *xi,
 					   struct ocfs2_xattr_search *xs,
 					   u32 name_hash,
 					   int local)
 {
 	int i, ret;
-	handle_t *handle = NULL;
 	u16 blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
-	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 
 	mlog(0, "Set xattr entry len = %lu index = %d in bucket %llu\n",
 	     (unsigned long)xi->value_len, xi->name_index,
@@ -4064,14 +4188,6 @@ static int ocfs2_xattr_set_entry_in_bucket(struct inode *inode,
 		}
 	}
 
-	handle = ocfs2_start_trans(osb, blk_per_bucket);
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		handle = NULL;
-		mlog_errno(ret);
-		goto out;
-	}
-
 	for (i = 0; i < blk_per_bucket; i++) {
 		ret = ocfs2_journal_access(handle, inode, xs->bucket.bhs[i],
 					   OCFS2_JOURNAL_ACCESS_WRITE);
@@ -4089,32 +4205,22 @@ static int ocfs2_xattr_set_entry_in_bucket(struct inode *inode,
 	if (ret)
 		mlog_errno(ret);
 out:
-	ocfs2_commit_trans(osb, handle);
-
 	return ret;
 }
 
 static int ocfs2_xattr_value_update_size(struct inode *inode,
+					 handle_t *handle,
 					 struct buffer_head *xe_bh,
 					 struct ocfs2_xattr_entry *xe,
 					 u64 new_size)
 {
 	int ret;
-	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
-	handle_t *handle = NULL;
-
-	handle = ocfs2_start_trans(osb, 1);
-	if (handle == NULL) {
-		ret = -ENOMEM;
-		mlog_errno(ret);
-		goto out;
-	}
 
 	ret = ocfs2_journal_access(handle, inode, xe_bh,
 				   OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret < 0) {
 		mlog_errno(ret);
-		goto out_commit;
+		goto out;
 	}
 
 	xe->xe_value_size = cpu_to_le64(new_size);
@@ -4123,8 +4229,6 @@ static int ocfs2_xattr_value_update_size(struct inode *inode,
 	if (ret < 0)
 		mlog_errno(ret);
 
-out_commit:
-	ocfs2_commit_trans(osb, handle);
 out:
 	return ret;
 }
@@ -4137,9 +4241,11 @@ out:
  * Copy the new updated xe and xe_value_root to new_xe and new_xv if needed.
  */
 static int ocfs2_xattr_bucket_value_truncate(struct inode *inode,
+					     handle_t *handle,
 					     struct buffer_head *header_bh,
 					     int xe_off,
-					     int len)
+					     int len,
+					     struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	int ret, offset;
 	u64 value_blk;
@@ -4174,13 +4280,14 @@ static int ocfs2_xattr_bucket_value_truncate(struct inode *inode,
 
 	mlog(0, "truncate %u in xattr bucket %llu to %d bytes.\n",
 	     xe_off, (unsigned long long)header_bh->b_blocknr, len);
-	ret = ocfs2_xattr_value_truncate(inode, value_bh, xv, len);
+	ret = ocfs2_xattr_value_truncate(inode, handle, value_bh,
+					 xv, len, ctxt);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
 
-	ret = ocfs2_xattr_value_update_size(inode, header_bh, xe, len);
+	ret = ocfs2_xattr_value_update_size(inode, handle, header_bh, xe, len);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
@@ -4192,8 +4299,10 @@ out:
 }
 
 static int ocfs2_xattr_bucket_value_truncate_xs(struct inode *inode,
-						struct ocfs2_xattr_search *xs,
-						int len)
+					handle_t *handle,
+					struct ocfs2_xattr_search *xs,
+					int len,
+					struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	int ret, offset;
 	struct ocfs2_xattr_entry *xe = xs->here;
@@ -4202,8 +4311,9 @@ static int ocfs2_xattr_bucket_value_truncate_xs(struct inode *inode,
 	BUG_ON(!xs->bucket.bhs[0] || !xe || ocfs2_xattr_is_local(xe));
 
 	offset = xe - xh->xh_entries;
-	ret = ocfs2_xattr_bucket_value_truncate(inode, xs->bucket.bhs[0],
-						offset, len);
+	ret = ocfs2_xattr_bucket_value_truncate(inode, handle,
+						xs->bucket.bhs[0],
+						offset, len, ctxt);
 	if (ret)
 		mlog_errno(ret);
 
@@ -4211,6 +4321,7 @@ static int ocfs2_xattr_bucket_value_truncate_xs(struct inode *inode,
 }
 
 static int ocfs2_xattr_bucket_set_value_outside(struct inode *inode,
+						handle_t *handle,
 						struct ocfs2_xattr_search *xs,
 						char *val,
 						int value_len)
@@ -4226,7 +4337,8 @@ static int ocfs2_xattr_bucket_set_value_outside(struct inode *inode,
 
 	xv = (struct ocfs2_xattr_value_root *)(xs->base + offset);
 
-	return __ocfs2_xattr_set_value_outside(inode, xv, val, value_len);
+	return __ocfs2_xattr_set_value_outside(inode, handle,
+					       xv, val, value_len);
 }
 
 static int ocfs2_rm_xattr_cluster(struct inode *inode,
@@ -4319,26 +4431,19 @@ out:
 }
 
 static void ocfs2_xattr_bucket_remove_xs(struct inode *inode,
+					 handle_t *handle,
 					 struct ocfs2_xattr_search *xs)
 {
-	handle_t *handle = NULL;
 	struct ocfs2_xattr_header *xh = xs->bucket.xh;
 	struct ocfs2_xattr_entry *last = &xh->xh_entries[
 						le16_to_cpu(xh->xh_count) - 1];
 	int ret = 0;
 
-	handle = ocfs2_start_trans((OCFS2_SB(inode->i_sb)), 1);
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		mlog_errno(ret);
-		return;
-	}
-
 	ret = ocfs2_journal_access(handle, inode, xs->bucket.bhs[0],
 				   OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
-		goto out_commit;
+		return;
 	}
 
 	/* Remove the old entry. */
@@ -4350,8 +4455,6 @@ static void ocfs2_xattr_bucket_remove_xs(struct inode *inode,
 	ret = ocfs2_journal_dirty(handle, xs->bucket.bhs[0]);
 	if (ret < 0)
 		mlog_errno(ret);
-out_commit:
-	ocfs2_commit_trans(OCFS2_SB(inode->i_sb), handle);
 }
 
 /*
@@ -4366,8 +4469,10 @@ out_commit:
  *    to free the xattr we allocated in set.
  */
 static int ocfs2_xattr_set_in_bucket(struct inode *inode,
+				     handle_t *handle,
 				     struct ocfs2_xattr_info *xi,
-				     struct ocfs2_xattr_search *xs)
+				     struct ocfs2_xattr_search *xs,
+				     struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	int ret, local = 1;
 	size_t value_len;
@@ -4394,8 +4499,8 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
 		else
 			value_len = 0;
 
-		ret = ocfs2_xattr_bucket_value_truncate_xs(inode, xs,
-							   value_len);
+		ret = ocfs2_xattr_bucket_value_truncate_xs(inode, handle,
+							   xs, value_len, ctxt);
 		if (ret)
 			goto out;
 
@@ -4415,7 +4520,8 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
 		xi->value_len = OCFS2_XATTR_ROOT_SIZE;
 	}
 
-	ret = ocfs2_xattr_set_entry_in_bucket(inode, xi, xs, name_hash, local);
+	ret = ocfs2_xattr_set_entry_in_bucket(inode, handle, xi, xs,
+					      name_hash, local);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
@@ -4425,8 +4531,8 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
 		goto out;
 
 	/* allocate the space now for the outside block storage. */
-	ret = ocfs2_xattr_bucket_value_truncate_xs(inode, xs,
-						   value_len);
+	ret = ocfs2_xattr_bucket_value_truncate_xs(inode, handle,
+						   xs, value_len, ctxt);
 	if (ret) {
 		mlog_errno(ret);
 
@@ -4436,13 +4542,14 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
 			 * storage and we have allocated xattr already,
 			 * so need to remove it.
 			 */
-			ocfs2_xattr_bucket_remove_xs(inode, xs);
+			ocfs2_xattr_bucket_remove_xs(inode, handle, xs);
 		}
 		goto out;
 	}
 
 set_value_outside:
-	ret = ocfs2_xattr_bucket_set_value_outside(inode, xs, val, value_len);
+	ret = ocfs2_xattr_bucket_set_value_outside(inode, handle,
+						   xs, val, value_len);
 out:
 	return ret;
 }
@@ -4466,8 +4573,10 @@ static int ocfs2_check_xattr_bucket_collision(struct inode *inode,
 }
 
 static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
+					     handle_t *handle,
 					     struct ocfs2_xattr_info *xi,
-					     struct ocfs2_xattr_search *xs)
+					     struct ocfs2_xattr_search *xs,
+					     struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	struct ocfs2_xattr_header *xh;
 	struct ocfs2_xattr_entry *xe;
@@ -4543,7 +4652,8 @@ try_again:
 			 * name/value will be moved, the xe shouldn't be changed
 			 * in xs.
 			 */
-			ret = ocfs2_defrag_xattr_bucket(inode, &xs->bucket);
+			ret = ocfs2_defrag_xattr_bucket(inode, handle,
+							&xs->bucket);
 			if (ret) {
 				mlog_errno(ret);
 				goto out;
@@ -4581,8 +4691,10 @@ try_again:
 		}
 
 		ret = ocfs2_add_new_xattr_bucket(inode,
+						 handle,
 						 xs->xattr_bh,
-						 xs->bucket.bhs[0]);
+						 xs->bucket.bhs[0],
+						 ctxt);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
@@ -4604,12 +4716,17 @@ try_again:
 	}
 
 xattr_set:
-	ret = ocfs2_xattr_set_in_bucket(inode, xi, xs);
+	ret = ocfs2_xattr_set_in_bucket(inode, handle, xi, xs, ctxt);
 out:
 	mlog_exit(ret);
 	return ret;
 }
 
+struct ocfs2_delete_xattr_ctxt {
+	struct ocfs2_xattr_set_ctxt *set_ctxt;
+	handle_t *handle;
+};
+
 static int ocfs2_delete_xattr_in_bucket(struct inode *inode,
 					struct ocfs2_xattr_bucket *bucket,
 					void *para)
@@ -4618,6 +4735,23 @@ static int ocfs2_delete_xattr_in_bucket(struct inode *inode,
 	struct ocfs2_xattr_header *xh = bucket->xh;
 	u16 i;
 	struct ocfs2_xattr_entry *xe;
+	handle_t *handle = NULL;
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	struct ocfs2_xattr_set_ctxt ctxt;
+
+	memset(&ctxt, 0, sizeof(ctxt));
+	ocfs2_init_dealloc_ctxt(&ctxt.dealloc);
+
+	/*
+	 * actually ofs2_xattr_value_truncate will extend trans
+	 * if needed, so just 1 is enough.
+	 */
+	handle = ocfs2_start_trans(osb, 1);
+	if (IS_ERR(handle)) {
+		ret = PTR_ERR(handle);
+		mlog_errno(ret);
+		goto out;
+	}
 
 	for (i = 0; i < le16_to_cpu(xh->xh_count); i++) {
 		xe = &xh->xh_entries[i];
@@ -4625,14 +4759,19 @@ static int ocfs2_delete_xattr_in_bucket(struct inode *inode,
 			continue;
 
 		ret = ocfs2_xattr_bucket_value_truncate(inode,
+							handle,
 							bucket->bhs[0],
-							i, 0);
+							i, 0, &ctxt);
 		if (ret) {
 			mlog_errno(ret);
 			break;
 		}
 	}
 
+	ret = ocfs2_commit_trans(osb, handle);
+	ocfs2_schedule_truncate_log_flush(osb, 1);
+	ocfs2_run_deallocs(osb, &ctxt.dealloc);
+out:
 	return ret;
 }
 
-- 
1.5.4.GIT

[Ocfs2-devel] [PATCH 2/3] ocfs2/xattr: Merge xattr set transaction.

[Joel Becker]

On Fri, Oct 17, 2008 at 12:44:37PM +0800, Tao Ma wrote:
> In current ocfs2/xattr, the whole xattr set is divided into
> many steps are many transaction are used, this make the
> xattr set process isn't like a real transaction, so this
> patch try to merge all the transaction into one. Another
> benefit is that acl can use it easily now.

	I love the concept of this change.  I didn't like the old code,
where random hunks of the operation would open their own handle and
close it right away.
 
> I don't merge the transaction of deleting xattr when we
> remove an inode. The reason is that if we have a large number
> of xattrs and every xattrs has large values(large enough
> for outside storage), the whole transaction will be very
> huge and it looks like jbd can't handle it(I meet with a
> jbd complain once). And the old inode removal is also divided
> into many steps, so I'd like to leave as it is.

	That's just fine.  Piecemeal is the right way to go there.  As
long as we have a sane inode after every step, it is easily recovered
from the orphan dir after a crash.

> Signed-off-by: Tao Ma <tao.ma@oracle.com>
> ---
>  fs/ocfs2/xattr.c |  889 +++++++++++++++++++++++++++++++-----------------------
>  1 files changed, 514 insertions(+), 375 deletions(-)

	I'm trying to think of a way you could break this patch up.
It's huge.

> @@ -128,11 +134,15 @@ static int ocfs2_xattr_tree_list_index_block(struct inode *inode,
>  					size_t buffer_size);
>  
>  static int ocfs2_xattr_create_index_block(struct inode *inode,
> -					  struct ocfs2_xattr_search *xs);
> +					  handle_t *handle,
> +					  struct ocfs2_xattr_search *xs,
> +					  struct ocfs2_xattr_set_ctxt *ctxt);
>  
>  static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
> +					     handle_t *handle,
>  					     struct ocfs2_xattr_info *xi,
> -					     struct ocfs2_xattr_search *xs);
> +					     struct ocfs2_xattr_search *xs,
> +					     struct ocfs2_xattr_set_ctxt *ctxt);

	You pass 'handle' and 'ctxt' to all the same functions.  Why not
put the handle on the ctxt?

> @@ -203,21 +211,10 @@ static int ocfs2_xattr_extend_allocation(struct inode *inode,
>  
>  	ocfs2_init_xattr_value_extent_tree(&et, inode, xattr_bh, xv);
>  
> -restart_all:
> -
> -	status = ocfs2_lock_allocators(inode, &et, clusters_to_add, 0,
> -				       &data_ac, &meta_ac);
> -	if (status) {
> -		mlog_errno(status);
> -		goto leave;
> -	}
> -
>  	credits = ocfs2_calc_extend_credits(osb->sb, et.et_root_el,
> -					    clusters_to_add);
> -	handle = ocfs2_start_trans(osb, credits);
> -	if (IS_ERR(handle)) {
> -		status = PTR_ERR(handle);
> -		handle = NULL;
> +			clusters_to_add) + handle->h_buffer_credits;
> +	status = ocfs2_extend_trans(handle, credits);

	Um, you just made 'credits' be double the existing transaction +
the result of ocfs2_calc_extend_credits().  Did you mean that?
ocfs2_extend_trans() takes the additional credits, not the total.  Later
you do an eocfs2_extend_trans() with merely the new credits, so I wonder
if you meant this here?

> @@ -280,62 +278,30 @@ restarted_transaction:
>  	}
>  
>  leave:
> -	if (handle) {
> -		ocfs2_commit_trans(osb, handle);
> -		handle = NULL;
> -	}
> -	if (data_ac) {
> -		ocfs2_free_alloc_context(data_ac);
> -		data_ac = NULL;
> -	}
> -	if (meta_ac) {
> -		ocfs2_free_alloc_context(meta_ac);
> -		meta_ac = NULL;
> -	}
> -	if ((!status) && restart_func) {
> -		restart_func = 0;
> -		goto restart_all;
> -	}

	I love watching repeated code like this disappear.

> @@ -896,14 +858,12 @@ static int __ocfs2_xattr_set_value_outside(struct inode *inode,
>  	u32 clusters = ocfs2_clusters_for_bytes(inode->i_sb, value_len);
>  	u64 blkno;
>  	struct buffer_head *bh = NULL;
> -	handle_t *handle;
>  
>  	BUG_ON(clusters > le32_to_cpu(xv->xr_clusters));
>  
>  	credits = clusters * bpc;
> -	handle = ocfs2_start_trans(OCFS2_SB(inode->i_sb), credits);
> -	if (IS_ERR(handle)) {
> -		ret = PTR_ERR(handle);
> +	ret = ocfs2_extend_trans(handle, credits);
> +	if (ret) {

	I also worry about all the places you are extending the
transaction - we would love to see this all as one transaction so we
don't leak things on crash.  That said, sometimes it is hard to do, and
this is a complex operation.

> +static int ocfs2_calc_xattr_set_credits(struct inode *inode,
> +					struct ocfs2_xattr_info *xi,
> +					struct ocfs2_xattr_search *xis,
> +					struct ocfs2_xattr_search *xbs)
> +{
> +	int credits = OCFS2_INODE_UPDATE_CREDITS;
> +
> +	/* calculate xattr metadata update credits. */
> +	if (xbs->xattr_bh) {
> +		struct ocfs2_xattr_block *xb =
> +			(struct ocfs2_xattr_block *)xbs->xattr_bh->b_data;
> +
> +		if (!(le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED))
> +			credits += OCFS2_XATTR_BLOCK_UPDATE_CREDITS;
> +		else
> +			credits += ocfs2_blocks_per_xattr_bucket(inode->i_sb);
> +	}
> +
> +	return credits;
> +}
> +
> +static int ocfs2_init_xattr_set_ctxt(struct inode *inode,
> +				     struct ocfs2_dinode *di,
> +				     struct ocfs2_xattr_info *xi,
> +				     struct ocfs2_xattr_search *xis,
> +				     struct ocfs2_xattr_search *xbs,
> +				     struct ocfs2_xattr_set_ctxt *ctxt)
> +{
> +	int ret = 0, clusters_add = 0, meta_add = 0;
> +	struct buffer_head *bh = NULL;
> +	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> +	struct ocfs2_xattr_block *xb = NULL;
> +	struct ocfs2_xattr_entry *xe;
> +
> +	memset(ctxt, 0, sizeof(struct ocfs2_xattr_set_ctxt));
> +
> +	ocfs2_init_dealloc_ctxt(&ctxt->dealloc);
> +
> +	if (di->i_xattr_loc) {
> +		if (!xbs->xattr_bh) {
> +			ret = ocfs2_read_block(inode,
> +					       le64_to_cpu(di->i_xattr_loc),
> +					       &bh);
> +			if (ret) {
> +				mlog_errno(ret);
> +				goto out;
> +			}
> +
> +			xb = (struct ocfs2_xattr_block *)bh->b_data;
> +		} else
> +			xb = (struct ocfs2_xattr_block *)xbs->xattr_bh->b_data;
> +
> +		if (le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED) {
> +			struct ocfs2_extent_list *el =
> +				 &xb->xb_attrs.xb_root.xt_list;
> +			meta_add += ocfs2_extend_meta_needed(el);
> +		}
> +		/*
> +		 * This cluster will be used either for new bucket or for
> +		 * new xattr block.
> +		 * If the cluster size is the same as the bucket size, one
> +		 * more is needed since we may need to extend the bucket
> +		 * also.
> +		 */
> +		clusters_add += 1;
> +		if (OCFS2_XATTR_BUCKET_SIZE == osb->s_clustersize)
> +			clusters_add += 1;
> +	} else
> +		meta_add += 1;
> +
> +	/* calculate xattr value update. */
> +	if (xi->value && xi->value_len > OCFS2_XATTR_INLINE_SIZE) {
> +		char *base;
> +		int name_offset, name_len, block_off, i;
> +		u32 new_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
> +							    xi->value_len);
> +
> +		xe = NULL;
> +		if (!xis->not_found) {
> +			xe = xis->here;
> +			name_offset = le16_to_cpu(xe->xe_name_offset);
> +			name_len = OCFS2_XATTR_SIZE(xe->xe_name_len);
> +			base = xis->base;
> +		} else if (!xbs->not_found) {
> +			xe = xbs->here;
> +			name_offset = le16_to_cpu(xe->xe_name_offset);
> +			name_len = OCFS2_XATTR_SIZE(xe->xe_name_len);
> +			i = xbs->here - xbs->header->xh_entries;
> +
> +			if (le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED) {
> +				ret = ocfs2_xattr_bucket_get_name_value(inode,
> +								xbs->bucket.xh,
> +								i,
> +								&block_off,
> +								&name_offset);
> +				base = xbs->bucket.bhs[block_off]->b_data;
> +			} else
> +				base = xbs->base;
> +		}
> +
> +		/* calc value clusters and value tree metadata change. */
> +		if (!xe ||
> +		    (le64_to_cpu(xe->xe_value_size) <= OCFS2_XATTR_INLINE_SIZE))
> +			clusters_add += new_clusters;
> +		else {
> +			u32 old_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
> +						le64_to_cpu(xe->xe_value_size));
> +
> +			if (old_clusters < new_clusters) {
> +				struct ocfs2_xattr_value_root *xv =
> +					(struct ocfs2_xattr_value_root *)
> +					(base + name_offset + name_len);
> +				meta_add +=
> +					ocfs2_extend_meta_needed(&xv->xr_list);
> +				clusters_add += new_clusters - old_clusters;
> +
> +			}
> +		}
> +	}
> +
> +	mlog(0, "Set xattr %s, reserve meta blocks = %d, clusters = %d\n",
> +	     xi->name, meta_add, clusters_add);
> +	if (meta_add) {
> +		ret = ocfs2_reserve_new_metadata_blocks(osb, meta_add,
> +							&ctxt->meta_ac);
> +		if (ret)
> +			mlog_errno(ret);
> +	}
> +
> +	if (clusters_add) {
> +		ret = ocfs2_reserve_clusters(osb, clusters_add, &ctxt->data_ac);
> +		if (ret) {
> +			mlog_errno(ret);
> +			goto out;
> +		}
> +	}
> +
> +out:
> +	return ret;
> +}

	Here you have almost all the information to calculate the
credits you need for your transaction?  Any chance you can pass that to
ocfs2_start_trans() up front and not have to extend later?

> @@ -1940,8 +2098,12 @@ int ocfs2_xattr_set(struct inode *inode,
>  {
>  	struct buffer_head *di_bh = NULL;
>  	struct ocfs2_dinode *di;
> -	int ret;
> +	int ret, credits;
>  	u16 i, blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
> +	struct ocfs2_xattr_set_ctxt ctxt;
> +	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> +	struct inode *tl_inode = osb->osb_tl_inode;
> +	handle_t *handle = NULL;
>  
>  	struct ocfs2_xattr_info xi = {
>  		.name_index = name_index,
> @@ -1996,48 +2158,45 @@ int ocfs2_xattr_set(struct inode *inode,
>  			goto cleanup;
>  	}
>  
> -	if (!value) {
> -		/* Remove existing extended attribute */
> -		if (!xis.not_found)
> -			ret = ocfs2_xattr_ibody_set(inode, &xi, &xis);
> -		else if (!xbs.not_found)
> -			ret = ocfs2_xattr_block_set(inode, &xi, &xbs);
> -	} else {
> -		/* We always try to set extended attribute into inode first*/
> -		ret = ocfs2_xattr_ibody_set(inode, &xi, &xis);
> -		if (!ret && !xbs.not_found) {
> -			/*
> -			 * If succeed and that extended attribute existing in
> -			 * external block, then we will remove it.
> -			 */
> -			xi.value = NULL;
> -			xi.value_len = 0;
> -			ret = ocfs2_xattr_block_set(inode, &xi, &xbs);
> -		} else if (ret == -ENOSPC) {
> -			if (di->i_xattr_loc && !xbs.xattr_bh) {
> -				ret = ocfs2_xattr_block_find(inode, name_index,
> -							     name, &xbs);
> -				if (ret)
> -					goto cleanup;
> -			}
> -			/*
> -			 * If no space in inode, we will set extended attribute
> -			 * into external block.
> -			 */
> -			ret = ocfs2_xattr_block_set(inode, &xi, &xbs);
> -			if (ret)
> -				goto cleanup;
> -			if (!xis.not_found) {
> -				/*
> -				 * If succeed and that extended attribute
> -				 * existing in inode, we will remove it.
> -				 */
> -				xi.value = NULL;
> -				xi.value_len = 0;
> -				ret = ocfs2_xattr_ibody_set(inode, &xi, &xis);
> -			}
> +
> +	mutex_lock(&tl_inode->i_mutex);
> +
> +	if (ocfs2_truncate_log_needs_flush(osb)) {
> +		ret = __ocfs2_flush_truncate_log(osb);
> +		if (ret < 0) {
> +			mutex_unlock(&tl_inode->i_mutex);
> +			mlog_errno(ret);
> +			goto cleanup;
>  		}
>  	}
> +	mutex_unlock(&tl_inode->i_mutex);
> +
> +	ret = ocfs2_init_xattr_set_ctxt(inode, di, &xi, &xis, &xbs, &ctxt);
> +	if (ret) {
> +		mlog_errno(ret);
> +		goto cleanup;
> +	}
> +
> +	credits = ocfs2_calc_xattr_set_credits(inode, &xi, &xis, &xbs);

	You recalculate this inside __ocfs2_xattr_set_handle().  Why do
you need to calculate it twice?

> +	handle = ocfs2_start_trans(osb, credits);
> +	if (IS_ERR(handle)) {
> +		ret = PTR_ERR(handle);
> +		mlog_errno(ret);
> +		goto cleanup;
> +	}
> +
> +	ret = __ocfs2_xattr_set_handle(inode, handle, di,
> +				       &xi, &xis, &xbs, &ctxt);
> +
> +	ocfs2_commit_trans(osb, handle);
> +
> +	if (ctxt.data_ac)
> +		ocfs2_free_alloc_context(ctxt.data_ac);
> +	if (ctxt.meta_ac)
> +		ocfs2_free_alloc_context(ctxt.meta_ac);
> +	ocfs2_schedule_truncate_log_flush(osb, 1);
> +	ocfs2_run_deallocs(osb, &ctxt.dealloc);
> +
>  cleanup:
>  	up_write(&OCFS2_I(inode)->ip_xattr_sem);
>  	ocfs2_inode_unlock(inode, 1);

<snip>

> @@ -2707,24 +2860,24 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
>  	 * of the new xattr bucket and one for the value/data.
>  	 */
>  	credits += 3;
> -	handle = ocfs2_start_trans(osb, credits);
> -	if (IS_ERR(handle)) {
> -		ret = PTR_ERR(handle);
> +	ret = ocfs2_extend_trans(handle, credits + handle->h_buffer_credits);

	Again, you extend by double + credits.  How come?  I'm going to
stop mentioning these, though they appear other places in the code.
	Overall, I like where this is going.

Joel

-- 

"Nothing is wrong with California that a rise in the ocean level
 wouldn't cure."
        - Ross MacDonald

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

[Ocfs2-devel] [PATCH 2/3] ocfs2/xattr: Merge xattr set transaction.

[Tao Ma]

Joel Becker wrote:
> On Fri, Oct 17, 2008 at 12:44:37PM +0800, Tao Ma wrote:
>> In current ocfs2/xattr, the whole xattr set is divided into
>> many steps are many transaction are used, this make the
>> xattr set process isn't like a real transaction, so this
>> patch try to merge all the transaction into one. Another
>> benefit is that acl can use it easily now.
> 
> 	I love the concept of this change.  I didn't like the old code,
> where random hunks of the operation would open their own handle and
> close it right away.
>  
>> I don't merge the transaction of deleting xattr when we
>> remove an inode. The reason is that if we have a large number
>> of xattrs and every xattrs has large values(large enough
>> for outside storage), the whole transaction will be very
>> huge and it looks like jbd can't handle it(I meet with a
>> jbd complain once). And the old inode removal is also divided
>> into many steps, so I'd like to leave as it is.
> 
> 	That's just fine.  Piecemeal is the right way to go there.  As
> long as we have a sane inode after every step, it is easily recovered
> from the orphan dir after a crash.
> 
>> Signed-off-by: Tao Ma <tao.ma@oracle.com>
>> ---
>>  fs/ocfs2/xattr.c |  889 +++++++++++++++++++++++++++++++-----------------------
>>  1 files changed, 514 insertions(+), 375 deletions(-)
> 
> 	I'm trying to think of a way you could break this patch up.
> It's huge.
Actually, most of the modification are just changing ocfs2_start_trans 
to ocfs2_extend_trans and passing 'handle_t *' to the function. So 
although the patch is a little large, but I think it is easy to review, 
does it? ;)
> 
>> @@ -128,11 +134,15 @@ static int ocfs2_xattr_tree_list_index_block(struct inode *inode,
>>  					size_t buffer_size);
>>  
>>  static int ocfs2_xattr_create_index_block(struct inode *inode,
>> -					  struct ocfs2_xattr_search *xs);
>> +					  handle_t *handle,
>> +					  struct ocfs2_xattr_search *xs,
>> +					  struct ocfs2_xattr_set_ctxt *ctxt);
>>  
>>  static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
>> +					     handle_t *handle,
>>  					     struct ocfs2_xattr_info *xi,
>> -					     struct ocfs2_xattr_search *xs);
>> +					     struct ocfs2_xattr_search *xs,
>> +					     struct ocfs2_xattr_set_ctxt *ctxt);
> 
> 	You pass 'handle' and 'ctxt' to all the same functions.  Why not
> put the handle on the ctxt?
Most functions doesn't need 'ctxt' actually, only the functions which 
use cluster/metadata allocation/free use it. So I'd like to separate them.
> 
>> @@ -203,21 +211,10 @@ static int ocfs2_xattr_extend_allocation(struct inode *inode,
>>  
>>  	ocfs2_init_xattr_value_extent_tree(&et, inode, xattr_bh, xv);
>>  
>> -restart_all:
>> -
>> -	status = ocfs2_lock_allocators(inode, &et, clusters_to_add, 0,
>> -				       &data_ac, &meta_ac);
>> -	if (status) {
>> -		mlog_errno(status);
>> -		goto leave;
>> -	}
>> -
>>  	credits = ocfs2_calc_extend_credits(osb->sb, et.et_root_el,
>> -					    clusters_to_add);
>> -	handle = ocfs2_start_trans(osb, credits);
>> -	if (IS_ERR(handle)) {
>> -		status = PTR_ERR(handle);
>> -		handle = NULL;
>> +			clusters_to_add) + handle->h_buffer_credits;
>> +	status = ocfs2_extend_trans(handle, credits);
> 
> 	Um, you just made 'credits' be double the existing transaction +
> the result of ocfs2_calc_extend_credits().  Did you mean that?
> ocfs2_extend_trans() takes the additional credits, not the total.  Later
> you do an eocfs2_extend_trans() with merely the new credits, so I wonder
> if you meant this here?
Yes, I do this intentionally. Because ocfs2_extend_trans sometimes 
doesn't work like its name. ;) If the first extension fails, it will 
commit the dirty blocks and then extend it, that make the credits which 
isn't used lost. In some cases it is OK(see some of my following 
modification), while in other cases when some metadata will be updated 
after this function, it will fails because of no credits. I have once 
meet with a bug for this. And actually tree operation do the same thing 
as I do. See  ocfs2_extend_rotate_transaction.
>> +static int ocfs2_init_xattr_set_ctxt(struct inode *inode,
>> +				     struct ocfs2_dinode *di,
>> +				     struct ocfs2_xattr_info *xi,
>> +				     struct ocfs2_xattr_search *xis,
>> +				     struct ocfs2_xattr_search *xbs,
>> +				     struct ocfs2_xattr_set_ctxt *ctxt)
>> +{
>> +	int ret = 0, clusters_add = 0, meta_add = 0;
>> +	struct buffer_head *bh = NULL;
>> +	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>> +	struct ocfs2_xattr_block *xb = NULL;
>> +	struct ocfs2_xattr_entry *xe;
>> +
>> +	memset(ctxt, 0, sizeof(struct ocfs2_xattr_set_ctxt));
>> +
>> +	ocfs2_init_dealloc_ctxt(&ctxt->dealloc);
>> +
>> +	if (di->i_xattr_loc) {
>> +		if (!xbs->xattr_bh) {
>> +			ret = ocfs2_read_block(inode,
>> +					       le64_to_cpu(di->i_xattr_loc),
>> +					       &bh);
>> +			if (ret) {
>> +				mlog_errno(ret);
>> +				goto out;
>> +			}
>> +
>> +			xb = (struct ocfs2_xattr_block *)bh->b_data;
>> +		} else
>> +			xb = (struct ocfs2_xattr_block *)xbs->xattr_bh->b_data;
>> +
>> +		if (le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED) {
>> +			struct ocfs2_extent_list *el =
>> +				 &xb->xb_attrs.xb_root.xt_list;
>> +			meta_add += ocfs2_extend_meta_needed(el);
>> +		}
>> +		/*
>> +		 * This cluster will be used either for new bucket or for
>> +		 * new xattr block.
>> +		 * If the cluster size is the same as the bucket size, one
>> +		 * more is needed since we may need to extend the bucket
>> +		 * also.
>> +		 */
>> +		clusters_add += 1;
>> +		if (OCFS2_XATTR_BUCKET_SIZE == osb->s_clustersize)
>> +			clusters_add += 1;
>> +	} else
>> +		meta_add += 1;
>> +
>> +	/* calculate xattr value update. */
>> +	if (xi->value && xi->value_len > OCFS2_XATTR_INLINE_SIZE) {
>> +		char *base;
>> +		int name_offset, name_len, block_off, i;
>> +		u32 new_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
>> +							    xi->value_len);
>> +
>> +		xe = NULL;
>> +		if (!xis->not_found) {
>> +			xe = xis->here;
>> +			name_offset = le16_to_cpu(xe->xe_name_offset);
>> +			name_len = OCFS2_XATTR_SIZE(xe->xe_name_len);
>> +			base = xis->base;
>> +		} else if (!xbs->not_found) {
>> +			xe = xbs->here;
>> +			name_offset = le16_to_cpu(xe->xe_name_offset);
>> +			name_len = OCFS2_XATTR_SIZE(xe->xe_name_len);
>> +			i = xbs->here - xbs->header->xh_entries;
>> +
>> +			if (le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED) {
>> +				ret = ocfs2_xattr_bucket_get_name_value(inode,
>> +								xbs->bucket.xh,
>> +								i,
>> +								&block_off,
>> +								&name_offset);
>> +				base = xbs->bucket.bhs[block_off]->b_data;
>> +			} else
>> +				base = xbs->base;
>> +		}
>> +
>> +		/* calc value clusters and value tree metadata change. */
>> +		if (!xe ||
>> +		    (le64_to_cpu(xe->xe_value_size) <= OCFS2_XATTR_INLINE_SIZE))
>> +			clusters_add += new_clusters;
>> +		else {
>> +			u32 old_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
>> +						le64_to_cpu(xe->xe_value_size));
>> +
>> +			if (old_clusters < new_clusters) {
>> +				struct ocfs2_xattr_value_root *xv =
>> +					(struct ocfs2_xattr_value_root *)
>> +					(base + name_offset + name_len);
>> +				meta_add +=
>> +					ocfs2_extend_meta_needed(&xv->xr_list);
>> +				clusters_add += new_clusters - old_clusters;
>> +
>> +			}
>> +		}
>> +	}
>> +
>> +	mlog(0, "Set xattr %s, reserve meta blocks = %d, clusters = %d\n",
>> +	     xi->name, meta_add, clusters_add);
>> +	if (meta_add) {
>> +		ret = ocfs2_reserve_new_metadata_blocks(osb, meta_add,
>> +							&ctxt->meta_ac);
>> +		if (ret)
>> +			mlog_errno(ret);
>> +	}
>> +
>> +	if (clusters_add) {
>> +		ret = ocfs2_reserve_clusters(osb, clusters_add, &ctxt->data_ac);
>> +		if (ret) {
>> +			mlog_errno(ret);
>> +			goto out;
>> +		}
>> +	}
>> +
>> +out:
>> +	return ret;
>> +}
> 
> 	Here you have almost all the information to calculate the
> credits you need for your transaction?  Any chance you can pass that to
> ocfs2_start_trans() up front and not have to extend later?
yes, you are right. I will try to merge, but it isn't a deal. ;)
> 
>> @@ -1940,8 +2098,12 @@ int ocfs2_xattr_set(struct inode *inode,
>>  {
>>  	struct buffer_head *di_bh = NULL;
>>  	struct ocfs2_dinode *di;
>> -	int ret;
>> +	int ret, credits;
>>  	u16 i, blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
>> +	struct ocfs2_xattr_set_ctxt ctxt;
>> +	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>> +	struct inode *tl_inode = osb->osb_tl_inode;
>> +	handle_t *handle = NULL;
>>  
>>  	struct ocfs2_xattr_info xi = {
>>  		.name_index = name_index,
>> @@ -1996,48 +2158,45 @@ int ocfs2_xattr_set(struct inode *inode,
>>  			goto cleanup;
>>  	}
>>  
>> -	if (!value) {
>> -		/* Remove existing extended attribute */
>> -		if (!xis.not_found)
>> -			ret = ocfs2_xattr_ibody_set(inode, &xi, &xis);
>> -		else if (!xbs.not_found)
>> -			ret = ocfs2_xattr_block_set(inode, &xi, &xbs);
>> -	} else {
>> -		/* We always try to set extended attribute into inode first*/
>> -		ret = ocfs2_xattr_ibody_set(inode, &xi, &xis);
>> -		if (!ret && !xbs.not_found) {
>> -			/*
>> -			 * If succeed and that extended attribute existing in
>> -			 * external block, then we will remove it.
>> -			 */
>> -			xi.value = NULL;
>> -			xi.value_len = 0;
>> -			ret = ocfs2_xattr_block_set(inode, &xi, &xbs);
>> -		} else if (ret == -ENOSPC) {
>> -			if (di->i_xattr_loc && !xbs.xattr_bh) {
>> -				ret = ocfs2_xattr_block_find(inode, name_index,
>> -							     name, &xbs);
>> -				if (ret)
>> -					goto cleanup;
>> -			}
>> -			/*
>> -			 * If no space in inode, we will set extended attribute
>> -			 * into external block.
>> -			 */
>> -			ret = ocfs2_xattr_block_set(inode, &xi, &xbs);
>> -			if (ret)
>> -				goto cleanup;
>> -			if (!xis.not_found) {
>> -				/*
>> -				 * If succeed and that extended attribute
>> -				 * existing in inode, we will remove it.
>> -				 */
>> -				xi.value = NULL;
>> -				xi.value_len = 0;
>> -				ret = ocfs2_xattr_ibody_set(inode, &xi, &xis);
>> -			}
>> +
>> +	mutex_lock(&tl_inode->i_mutex);
>> +
>> +	if (ocfs2_truncate_log_needs_flush(osb)) {
>> +		ret = __ocfs2_flush_truncate_log(osb);
>> +		if (ret < 0) {
>> +			mutex_unlock(&tl_inode->i_mutex);
>> +			mlog_errno(ret);
>> +			goto cleanup;
>>  		}
>>  	}
>> +	mutex_unlock(&tl_inode->i_mutex);
>> +
>> +	ret = ocfs2_init_xattr_set_ctxt(inode, di, &xi, &xis, &xbs, &ctxt);
>> +	if (ret) {
>> +		mlog_errno(ret);
>> +		goto cleanup;
>> +	}
>> +
>> +	credits = ocfs2_calc_xattr_set_credits(inode, &xi, &xis, &xbs);
> 
> 	You recalculate this inside __ocfs2_xattr_set_handle().  Why do
> you need to calculate it twice?
I calculate twice because the situation has been changed. If the xattr 
is in inode, the ocfs2_xattr_set will only search inode, so the first 
calc will get 1(for inode update). While if the __ocfs2_xattr_set_handle 
can update the xattr successfully it will try to move it to bucket, then 
we have to calc again since we are going to update a bucket.

Regards,
Tao

[Ocfs2-devel] [PATCH 2/3] ocfs2/xattr: Merge xattr set transaction.

[Joel Becker]

On Fri, Oct 24, 2008 at 08:44:25AM +0800, Tao Ma wrote:
> Joel Becker wrote:
>>>  fs/ocfs2/xattr.c |  889 +++++++++++++++++++++++++++++++-----------------------
>>>  1 files changed, 514 insertions(+), 375 deletions(-)
>>
>> 	I'm trying to think of a way you could break this patch up.
>> It's huge.
> Actually, most of the modification are just changing ocfs2_start_trans  
> to ocfs2_extend_trans and passing 'handle_t *' to the function. So  
> although the patch is a little large, but I think it is easy to review,  
> does it? ;)

	No, I found it incredibly hard to wade through, because I'd get
to a point where I'd lost track of where I was, and I would look and see
I'd only viewed 44% of the patch :-)
	There's a lot going on in here.  You're moving the transaction
calculations around, moving locks, reorganizing the calls to the various
set handlers, and that's just for starters.

>>>   static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
>>> +					     handle_t *handle,
>>>  					     struct ocfs2_xattr_info *xi,
>>> -					     struct ocfs2_xattr_search *xs);
>>> +					     struct ocfs2_xattr_search *xs,
>>> +					     struct ocfs2_xattr_set_ctxt *ctxt);
>>
>> 	You pass 'handle' and 'ctxt' to all the same functions.  Why not
>> put the handle on the ctxt?
> Most functions doesn't need 'ctxt' actually, only the functions which  
> use cluster/metadata allocation/free use it. So I'd like to separate 
> them.

	Almost every function I saw that took 'handle' also took 'ctxt'.
But that's not such a big deal.

>>> +			clusters_to_add) + handle->h_buffer_credits;
>>> +	status = ocfs2_extend_trans(handle, credits);
>>
>> 	Um, you just made 'credits' be double the existing transaction +
>> the result of ocfs2_calc_extend_credits().  Did you mean that?
>> ocfs2_extend_trans() takes the additional credits, not the total.  Later
>> you do an eocfs2_extend_trans() with merely the new credits, so I wonder
>> if you meant this here?
> Yes, I do this intentionally. Because ocfs2_extend_trans sometimes  
> doesn't work like its name. ;) If the first extension fails, it will  
> commit the dirty blocks and then extend it, that make the credits which  
> isn't used lost. In some cases it is OK(see some of my following  
> modification), while in other cases when some metadata will be updated  
> after this function, it will fails because of no credits. I have once  
> meet with a bug for this. And actually tree operation do the same thing  
> as I do. See  ocfs2_extend_rotate_transaction.

	Ok, so the basic thought is that we're going to need
h_buffer_credits after we return from this function in other places -
we're not done with them just because we commit inside
ocfs2_extend_trans().
	The big concern here is that:

1) ocfs2_extend_trans() commits the half-completed work so it can
   free up some space in the journal.
2) The system crashes.
3) jbd2 replays the half-completed work.
4) We mount again, and our code can't handle the half-completed work.

	The alloc.c code is written to handle this half-completed state
of our trees.  Actually, our trees almost never end up in half-completed
states.  They are always valid trees at the point we might extend/commit
a transaction.   I'm not worried about the xattr trees (index or value)
in this regard.  I'm worried about any fields we've updated that get
written due to these unexpected commits.
	For example, when setting an xattr you try ibody_set() first.
If that succeeds, you then go ahead and clear the same xattr out of the
external block/tree.  So if there is an extend_trans() somewhere in
there, and it commits a change, you could crash and have the xattr in
both the ibody and the external block/tree.  The same with the reverse
(set in external, commit, clear in ibody, crash).  We need to make sure
we either don't actually have these cases happen or handle them
gracefully.

>>> +	credits = ocfs2_calc_xattr_set_credits(inode, &xi, &xis, &xbs);
>>
>> 	You recalculate this inside __ocfs2_xattr_set_handle().  Why do
>> you need to calculate it twice?
> I calculate twice because the situation has been changed. If the xattr  
> is in inode, the ocfs2_xattr_set will only search inode, so the first  
> calc will get 1(for inode update). While if the __ocfs2_xattr_set_handle  
> can update the xattr successfully it will try to move it to bucket, then  
> we have to calc again since we are going to update a bucket.

	Makes sense.

Joel

-- 

"Baby, even the losers
 Get luck sometimes.
 Even the losers
 Keep a little bit of pride."

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

[Ocfs2-devel] [PATCH 2/3] ocfs2/xattr: Merge xattr set transaction.

[Tao Ma]

Joel Becker wrote:
> On Fri, Oct 24, 2008 at 08:44:25AM +0800, Tao Ma wrote:
>> Joel Becker wrote:
>>>>  fs/ocfs2/xattr.c |  889 +++++++++++++++++++++++++++++++-----------------------
>>>>  1 files changed, 514 insertions(+), 375 deletions(-)
>>> 	I'm trying to think of a way you could break this patch up.
>>> It's huge.
>> Actually, most of the modification are just changing ocfs2_start_trans  
>> to ocfs2_extend_trans and passing 'handle_t *' to the function. So  
>> although the patch is a little large, but I think it is easy to review,  
>> does it? ;)
> 
> 	No, I found it incredibly hard to wade through, because I'd get
> to a point where I'd lost track of where I was, and I would look and see
> I'd only viewed 44% of the patch :-)
> 	There's a lot going on in here.  You're moving the transaction
> calculations around, moving locks, reorganizing the calls to the various
> set handlers, and that's just for starters.
OK, I will try to separate it into small ones.
> 
>>>>   static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
>>>> +					     handle_t *handle,
>>>>  					     struct ocfs2_xattr_info *xi,
>>>> -					     struct ocfs2_xattr_search *xs);
>>>> +					     struct ocfs2_xattr_search *xs,
>>>> +					     struct ocfs2_xattr_set_ctxt *ctxt);
>>> 	You pass 'handle' and 'ctxt' to all the same functions.  Why not
>>> put the handle on the ctxt?
>> Most functions doesn't need 'ctxt' actually, only the functions which  
>> use cluster/metadata allocation/free use it. So I'd like to separate 
>> them.
> 
> 	Almost every function I saw that took 'handle' also took 'ctxt'.
> But that's not such a big deal.
> 
>>>> +			clusters_to_add) + handle->h_buffer_credits;
>>>> +	status = ocfs2_extend_trans(handle, credits);
>>> 	Um, you just made 'credits' be double the existing transaction +
>>> the result of ocfs2_calc_extend_credits().  Did you mean that?
>>> ocfs2_extend_trans() takes the additional credits, not the total.  Later
>>> you do an eocfs2_extend_trans() with merely the new credits, so I wonder
>>> if you meant this here?
>> Yes, I do this intentionally. Because ocfs2_extend_trans sometimes  
>> doesn't work like its name. ;) If the first extension fails, it will  
>> commit the dirty blocks and then extend it, that make the credits which  
>> isn't used lost. In some cases it is OK(see some of my following  
>> modification), while in other cases when some metadata will be updated  
>> after this function, it will fails because of no credits. I have once  
>> meet with a bug for this. And actually tree operation do the same thing  
>> as I do. See  ocfs2_extend_rotate_transaction.
> 
> 	Ok, so the basic thought is that we're going to need
> h_buffer_credits after we return from this function in other places -
> we're not done with them just because we commit inside
> ocfs2_extend_trans().
> 	The big concern here is that:
> 
> 1) ocfs2_extend_trans() commits the half-completed work so it can
>    free up some space in the journal.
> 2) The system crashes.
> 3) jbd2 replays the half-completed work.
> 4) We mount again, and our code can't handle the half-completed work.
> 
> 	The alloc.c code is written to handle this half-completed state
> of our trees.  Actually, our trees almost never end up in half-completed
> states.  They are always valid trees at the point we might extend/commit
> a transaction.   I'm not worried about the xattr trees (index or value)
> in this regard.  I'm worried about any fields we've updated that get
> written due to these unexpected commits.
> 	For example, when setting an xattr you try ibody_set() first.
> If that succeeds, you then go ahead and clear the same xattr out of the
> external block/tree.  So if there is an extend_trans() somewhere in
> there, and it commits a change, you could crash and have the xattr in
> both the ibody and the external block/tree.  The same with the reverse
> (set in external, commit, clear in ibody, crash).  We need to make sure
> we either don't actually have these cases happen or handle them
> gracefully.
yeah, you may be right. As my previous e-mail said, I will try to 
calculate all the credits in the beginning. Let's see how it works.

Regards,
Tao

[Ocfs2-devel] [PATCH 2/3] ocfs2/xattr: Merge xattr set transaction.

[Joel Becker]

On Fri, Oct 24, 2008 at 09:17:40AM +0800, Tao Ma wrote:
> Joel Becker wrote:
>> On Fri, Oct 24, 2008 at 08:44:25AM +0800, Tao Ma wrote:
>>> Joel Becker wrote:
>> 	No, I found it incredibly hard to wade through, because I'd get
>> to a point where I'd lost track of where I was, and I would look and see
>> I'd only viewed 44% of the patch :-)
>> 	There's a lot going on in here.  You're moving the transaction
>> calculations around, moving locks, reorganizing the calls to the various
>> set handlers, and that's just for starters.
> OK, I will try to separate it into small ones.

	Thanks.  Don't worry about making them tiny - there's a lot
there.  But see if you can think of a couple logical steps.

>> 	For example, when setting an xattr you try ibody_set() first.
>> If that succeeds, you then go ahead and clear the same xattr out of the
>> external block/tree.  So if there is an extend_trans() somewhere in
>> there, and it commits a change, you could crash and have the xattr in
>> both the ibody and the external block/tree.  The same with the reverse
>> (set in external, commit, clear in ibody, crash).  We need to make sure
>> we either don't actually have these cases happen or handle them
>> gracefully.
> yeah, you may be right. As my previous e-mail said, I will try to calculate 
> all the credits in the beginning. Let's see how it works.

	Cool.  If you can, awesome.  If not, let's make sure that the
remounted filesystem can handle whatever's there.

Joel

-- 

"Nearly all men can stand adversity, but if you really want to
 test a man's character, give him power."
	- Abraham Lincoln

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

[Ocfs2-devel] [PATCH 3/3] ocfs2/xattr: Proper hash collision handle in bucket division.

[Tao Ma]

In ocfs2/xattr, we must make sure the xattrs which have the same
hash value exist in the same bucket so that the search schema can
work. But in the old implementation, when we want to extend a bucket,
we just move half number of xattrs to the new bucket. This works
in most cases, but if we are lucky enough we will make 2 xattrs
into 2 different buckets. This cause a problem that the xattr
existing in the previous bucket can be found any more. This patch
fix this problem by finding the right position during extending
the bucket and extend an empty bucket if needed.

Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/ocfs2/xattr.c |  167 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 134 insertions(+), 33 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 1bf72da..57b51fc 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -3267,25 +3267,92 @@ static int ocfs2_read_xattr_bucket(struct inode *inode,
 }
 
 /*
- * Move half num of the xattrs in old bucket(blk) to new bucket(new_blk).
+ * Find the suitable pos when we divide a bucket into 2.
+ *
+ * We have to make sure the xattrs with the same hash value exist in the same
+ * bucket. So we start from the middle pos and go backward first and then
+ * forward. If all the xattrs in this bucket have the same hash value, just
+ * return count-1 and let the caller handle this.
+ */
+static u16 ocfs2_xattr_find_divide_pos(struct ocfs2_xattr_header *xh)
+{
+	u16 middle, pos, count = le16_to_cpu(xh->xh_count);
+	struct ocfs2_xattr_entry *xe, *xe_low;
+
+	BUG_ON(count == 0);
+	middle = count / 2;
+
+	/*
+	 * If the bucket only have one xattr(for blocksize == 512 and large
+	 * xattr name, it could be possible), let it go.
+	 */
+	if (middle == 0)
+		return middle;
+
+	/*
+	 * Find a xe before middle which doesn't have the same hash value
+	 * with the previous one.
+	 */
+	pos = middle;
+	while (pos > 0) {
+		xe = &xh->xh_entries[pos];
+		xe_low = &xh->xh_entries[pos - 1];
+		if (le32_to_cpu(xe_low->xe_name_hash) !=
+		    le32_to_cpu(xe->xe_name_hash))
+			return pos;
+
+		pos--;
+	}
+
+	/* now all the xe before middle(include it) has the same hash value. */
+	if (middle == count - 1)
+		return middle;
+
+	/*
+	 * Find a xe after middle which doesn't have the same hash value
+	 * with the later one.
+	 */
+	pos = middle;
+	while (pos < count - 1) {
+		xe_low = &xh->xh_entries[pos];
+		xe = &xh->xh_entries[pos + 1];
+		if (le32_to_cpu(xe_low->xe_name_hash) !=
+		    le32_to_cpu(xe->xe_name_hash))
+			return pos + 1;
+
+		pos++;
+	}
+
+	/* now all the xe in the bucket hash the same hash value. */
+	return count - 1;
+}
+
+/*
+ * Move some xattrs in old bucket(blk) to new bucket(new_blk).
  * first_hash will record the 1st hash of the new bucket.
+ *
+ * Normally half nums will be moved. But we have to make sure
+ * that the xattr with same hash value is stored in the same
+ * bucket. If all the xattrs in this bucket has the same hash
+ * value, the new bucket will be initialized as an empty one
+ * and the first_hash will be initialized as (hash_value+1).
  */
-static int ocfs2_half_xattr_bucket(struct inode *inode,
-				   handle_t *handle,
-				   u64 blk,
-				   u64 new_blk,
-				   u32 *first_hash,
-				   int new_bucket_head)
+static int ocfs2_divide_xattr_bucket(struct inode *inode,
+				    handle_t *handle,
+				    u64 blk,
+				    u64 new_blk,
+				    u32 *first_hash,
+				    int new_bucket_head)
 {
 	int ret, i;
-	u16 count, start, len, name_value_len, xe_len, name_offset;
+	u16 count, start, len, name_value_len = 0, xe_len, name_offset = 0;
 	u16 blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
 	struct buffer_head **s_bhs, **t_bhs = NULL;
 	struct ocfs2_xattr_header *xh;
 	struct ocfs2_xattr_entry *xe;
 	int blocksize = inode->i_sb->s_blocksize;
 
-	mlog(0, "move half of xattrs from bucket %llu to %llu\n",
+	mlog(0, "move some of xattrs from bucket %llu to %llu\n",
 	     blk, new_blk);
 
 	s_bhs = kcalloc(blk_per_bucket, sizeof(struct buffer_head *), GFP_NOFS);
@@ -3326,14 +3393,33 @@ static int ocfs2_half_xattr_bucket(struct inode *inode,
 		}
 	}
 
-	/* copy the whole bucket to the new first. */
-	for (i = 0; i < blk_per_bucket; i++)
-		memcpy(t_bhs[i]->b_data, s_bhs[i]->b_data, blocksize);
+	xh = (struct ocfs2_xattr_header *)s_bhs[0]->b_data;
+	count = le16_to_cpu(xh->xh_count);
+	start = ocfs2_xattr_find_divide_pos(xh);
+	xe = &xh->xh_entries[start];
 
 	/* update the new bucket. */
 	xh = (struct ocfs2_xattr_header *)t_bhs[0]->b_data;
-	count = le16_to_cpu(xh->xh_count);
-	start = count / 2;
+
+	if (start == count - 1) {
+		/*
+		 * initialized a new empty bucket here.
+		 * The hash value is set as one larger than
+		 * that of the last entry in the previous bucket.
+		 */
+		for (i = 0; i < blk_per_bucket; i++)
+			memset(t_bhs[i]->b_data, 0, blocksize);
+
+		xh->xh_free_start = cpu_to_le16(blocksize);
+		xh->xh_entries[0].xe_name_hash = xe->xe_name_hash;
+		le32_add_cpu(&xh->xh_entries[0].xe_name_hash, 1);
+
+		goto set_num_buckets;
+	}
+
+	/* copy the whole bucket to the new first. */
+	for (i = 0; i < blk_per_bucket; i++)
+		memcpy(t_bhs[i]->b_data, s_bhs[i]->b_data, blocksize);
 
 	/*
 	 * Calculate the total name/value len and xh_free_start for
@@ -3390,6 +3476,7 @@ static int ocfs2_half_xattr_bucket(struct inode *inode,
 			xh->xh_free_start = xe->xe_name_offset;
 	}
 
+set_num_buckets:
 	/* set xh->xh_num_buckets for the new xh. */
 	if (new_bucket_head)
 		xh->xh_num_buckets = cpu_to_le16(1);
@@ -3406,10 +3493,12 @@ static int ocfs2_half_xattr_bucket(struct inode *inode,
 	if (first_hash)
 		*first_hash = le32_to_cpu(xh->xh_entries[0].xe_name_hash);
 
-	/*
-	 * Now only update the 1st block of the old bucket.
-	 * Please note that the entry has been sorted already above.
+	/* Now only update the 1st block of the old bucket.
+	 * If we just add a new empty bucket after it, no needs to modify it.
 	 */
+	if (start == count - 1)
+		goto out;
+
 	xh = (struct ocfs2_xattr_header *)s_bhs[0]->b_data;
 	memset(&xh->xh_entries[start], 0,
 	       sizeof(struct ocfs2_xattr_entry) * (count - start));
@@ -3592,15 +3681,15 @@ out:
 }
 
 /*
- * Move half of the xattrs in this cluster to the new cluster.
+ * Move some xattrs in this cluster to the new cluster.
  * This function should only be called when bucket size == cluster size.
  * Otherwise ocfs2_mv_xattr_bucket_cross_cluster should be used instead.
  */
-static int ocfs2_half_xattr_cluster(struct inode *inode,
-				    handle_t *handle,
-				    u64 prev_blk,
-				    u64 new_blk,
-				    u32 *first_hash)
+static int ocfs2_divide_xattr_cluster(struct inode *inode,
+				      handle_t *handle,
+				      u64 prev_blk,
+				      u64 new_blk,
+				      u32 *first_hash)
 {
 	u16 blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
 	int ret, credits = 2 * blk_per_bucket;
@@ -3614,8 +3703,8 @@ static int ocfs2_half_xattr_cluster(struct inode *inode,
 	}
 
 	/* Move half of the xattr in start_blk to the next bucket. */
-	return  ocfs2_half_xattr_bucket(inode, handle, prev_blk,
-					new_blk, first_hash, 1);
+	return  ocfs2_divide_xattr_bucket(inode, handle, prev_blk,
+					  new_blk, first_hash, 1);
 }
 
 /*
@@ -3677,9 +3766,9 @@ static int ocfs2_adjust_xattr_cross_cluster(struct inode *inode,
 						     last_blk, new_blk,
 						     v_start);
 		else {
-			ret = ocfs2_half_xattr_cluster(inode, handle,
-						       last_blk, new_blk,
-						       v_start);
+			ret = ocfs2_divide_xattr_cluster(inode, handle,
+							 last_blk, new_blk,
+							 v_start);
 
 			if ((*header_bh)->b_blocknr == last_blk && extend)
 				*extend = 0;
@@ -3877,8 +3966,8 @@ static int ocfs2_extend_xattr_bucket(struct inode *inode,
 	}
 
 	/* Move half of the xattr in start_blk to the next bucket. */
-	ret = ocfs2_half_xattr_bucket(inode, handle, start_blk,
-				      start_blk + blk_per_bucket, NULL, 0);
+	ret = ocfs2_divide_xattr_bucket(inode, handle, start_blk,
+					start_blk + blk_per_bucket, NULL, 0);
 
 	le16_add_cpu(&first_xh->xh_num_buckets, 1);
 	ocfs2_journal_dirty(handle, first_bh);
@@ -4554,11 +4643,21 @@ out:
 	return ret;
 }
 
-/* check whether the xattr bucket is filled up with the same hash value. */
+/*
+ * check whether the xattr bucket is filled up with the same hash value.
+ * If we want to insert the xattr with the same hash, return -ENOSPC.
+ * If we want to insert a xattr with different hash value, go ahead
+ * and ocfs2_divide_xattr_bucket will handle this.
+ */
 static int ocfs2_check_xattr_bucket_collision(struct inode *inode,
-					      struct ocfs2_xattr_bucket *bucket)
+					      struct ocfs2_xattr_bucket *bucket,
+					      const char *name)
 {
 	struct ocfs2_xattr_header *xh = bucket->xh;
+	u32 name_hash = ocfs2_xattr_name_hash(inode, name, strlen(name));
+
+	if (name_hash != le16_to_cpu(xh->xh_entries[0].xe_name_hash))
+		return 0;
 
 	if (xh->xh_entries[le16_to_cpu(xh->xh_count) - 1].xe_name_hash ==
 	    xh->xh_entries[0].xe_name_hash) {
@@ -4684,7 +4783,9 @@ try_again:
 		 * one bucket's worth, so check it here whether we need to
 		 * add a new bucket for the insert.
 		 */
-		ret = ocfs2_check_xattr_bucket_collision(inode, &xs->bucket);
+		ret = ocfs2_check_xattr_bucket_collision(inode,
+							 &xs->bucket,
+							 xi->name);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
-- 
1.5.4.GIT

[Ocfs2-devel] [PATCH 3/3] ocfs2/xattr: Proper hash collision handle in bucket division.v2

[Tao Ma]

Fix a minor sparse warning. Thank tiger for pointing it out.

In ocfs2/xattr, we must make sure the xattrs which have the same
hash value exist in the same bucket so that the search schema can
work. But in the old implementation, when we want to extend a bucket,
we just move half number of xattrs to the new bucket. This works
in most cases, but if we are lucky enough we will make 2 xattrs
into 2 different buckets. This cause a problem that the xattr
existing in the previous bucket can be found any more. This patch
fix this problem by finding the right position during extending
the bucket and extend an empty bucket if needed.

Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/ocfs2/xattr.c |  167 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 134 insertions(+), 33 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 1bf72da..a2d58a9 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -3267,25 +3267,92 @@ static int ocfs2_read_xattr_bucket(struct inode *inode,
 }
 
 /*
- * Move half num of the xattrs in old bucket(blk) to new bucket(new_blk).
+ * Find the suitable pos when we divide a bucket into 2.
+ *
+ * We have to make sure the xattrs with the same hash value exist in the same
+ * bucket. So we start from the middle pos and go backward first and then
+ * forward. If all the xattrs in this bucket have the same hash value, just
+ * return count-1 and let the caller handle this.
+ */
+static u16 ocfs2_xattr_find_divide_pos(struct ocfs2_xattr_header *xh)
+{
+	u16 middle, pos, count = le16_to_cpu(xh->xh_count);
+	struct ocfs2_xattr_entry *xe, *xe_low;
+
+	BUG_ON(count == 0);
+	middle = count / 2;
+
+	/*
+	 * If the bucket only have one xattr(for blocksize == 512 and large
+	 * xattr name, it could be possible), let it go.
+	 */
+	if (middle == 0)
+		return middle;
+
+	/*
+	 * Find a xe before middle which doesn't have the same hash value
+	 * with the previous one.
+	 */
+	pos = middle;
+	while (pos > 0) {
+		xe = &xh->xh_entries[pos];
+		xe_low = &xh->xh_entries[pos - 1];
+		if (le32_to_cpu(xe_low->xe_name_hash) !=
+		    le32_to_cpu(xe->xe_name_hash))
+			return pos;
+
+		pos--;
+	}
+
+	/* now all the xe before middle(include it) has the same hash value. */
+	if (middle == count - 1)
+		return middle;
+
+	/*
+	 * Find a xe after middle which doesn't have the same hash value
+	 * with the later one.
+	 */
+	pos = middle;
+	while (pos < count - 1) {
+		xe_low = &xh->xh_entries[pos];
+		xe = &xh->xh_entries[pos + 1];
+		if (le32_to_cpu(xe_low->xe_name_hash) !=
+		    le32_to_cpu(xe->xe_name_hash))
+			return pos + 1;
+
+		pos++;
+	}
+
+	/* now all the xe in the bucket hash the same hash value. */
+	return count - 1;
+}
+
+/*
+ * Move some xattrs in old bucket(blk) to new bucket(new_blk).
  * first_hash will record the 1st hash of the new bucket.
+ *
+ * Normally half nums will be moved. But we have to make sure
+ * that the xattr with same hash value is stored in the same
+ * bucket. If all the xattrs in this bucket has the same hash
+ * value, the new bucket will be initialized as an empty one
+ * and the first_hash will be initialized as (hash_value+1).
  */
-static int ocfs2_half_xattr_bucket(struct inode *inode,
-				   handle_t *handle,
-				   u64 blk,
-				   u64 new_blk,
-				   u32 *first_hash,
-				   int new_bucket_head)
+static int ocfs2_divide_xattr_bucket(struct inode *inode,
+				    handle_t *handle,
+				    u64 blk,
+				    u64 new_blk,
+				    u32 *first_hash,
+				    int new_bucket_head)
 {
 	int ret, i;
-	u16 count, start, len, name_value_len, xe_len, name_offset;
+	u16 count, start, len, name_value_len = 0, xe_len, name_offset = 0;
 	u16 blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
 	struct buffer_head **s_bhs, **t_bhs = NULL;
 	struct ocfs2_xattr_header *xh;
 	struct ocfs2_xattr_entry *xe;
 	int blocksize = inode->i_sb->s_blocksize;
 
-	mlog(0, "move half of xattrs from bucket %llu to %llu\n",
+	mlog(0, "move some of xattrs from bucket %llu to %llu\n",
 	     blk, new_blk);
 
 	s_bhs = kcalloc(blk_per_bucket, sizeof(struct buffer_head *), GFP_NOFS);
@@ -3326,14 +3393,33 @@ static int ocfs2_half_xattr_bucket(struct inode *inode,
 		}
 	}
 
-	/* copy the whole bucket to the new first. */
-	for (i = 0; i < blk_per_bucket; i++)
-		memcpy(t_bhs[i]->b_data, s_bhs[i]->b_data, blocksize);
+	xh = (struct ocfs2_xattr_header *)s_bhs[0]->b_data;
+	count = le16_to_cpu(xh->xh_count);
+	start = ocfs2_xattr_find_divide_pos(xh);
+	xe = &xh->xh_entries[start];
 
 	/* update the new bucket. */
 	xh = (struct ocfs2_xattr_header *)t_bhs[0]->b_data;
-	count = le16_to_cpu(xh->xh_count);
-	start = count / 2;
+
+	if (start == count - 1) {
+		/*
+		 * initialized a new empty bucket here.
+		 * The hash value is set as one larger than
+		 * that of the last entry in the previous bucket.
+		 */
+		for (i = 0; i < blk_per_bucket; i++)
+			memset(t_bhs[i]->b_data, 0, blocksize);
+
+		xh->xh_free_start = cpu_to_le16(blocksize);
+		xh->xh_entries[0].xe_name_hash = xe->xe_name_hash;
+		le32_add_cpu(&xh->xh_entries[0].xe_name_hash, 1);
+
+		goto set_num_buckets;
+	}
+
+	/* copy the whole bucket to the new first. */
+	for (i = 0; i < blk_per_bucket; i++)
+		memcpy(t_bhs[i]->b_data, s_bhs[i]->b_data, blocksize);
 
 	/*
 	 * Calculate the total name/value len and xh_free_start for
@@ -3390,6 +3476,7 @@ static int ocfs2_half_xattr_bucket(struct inode *inode,
 			xh->xh_free_start = xe->xe_name_offset;
 	}
 
+set_num_buckets:
 	/* set xh->xh_num_buckets for the new xh. */
 	if (new_bucket_head)
 		xh->xh_num_buckets = cpu_to_le16(1);
@@ -3406,10 +3493,12 @@ static int ocfs2_half_xattr_bucket(struct inode *inode,
 	if (first_hash)
 		*first_hash = le32_to_cpu(xh->xh_entries[0].xe_name_hash);
 
-	/*
-	 * Now only update the 1st block of the old bucket.
-	 * Please note that the entry has been sorted already above.
+	/* Now only update the 1st block of the old bucket.
+	 * If we just add a new empty bucket after it, no needs to modify it.
 	 */
+	if (start == count - 1)
+		goto out;
+
 	xh = (struct ocfs2_xattr_header *)s_bhs[0]->b_data;
 	memset(&xh->xh_entries[start], 0,
 	       sizeof(struct ocfs2_xattr_entry) * (count - start));
@@ -3592,15 +3681,15 @@ out:
 }
 
 /*
- * Move half of the xattrs in this cluster to the new cluster.
+ * Move some xattrs in this cluster to the new cluster.
  * This function should only be called when bucket size == cluster size.
  * Otherwise ocfs2_mv_xattr_bucket_cross_cluster should be used instead.
  */
-static int ocfs2_half_xattr_cluster(struct inode *inode,
-				    handle_t *handle,
-				    u64 prev_blk,
-				    u64 new_blk,
-				    u32 *first_hash)
+static int ocfs2_divide_xattr_cluster(struct inode *inode,
+				      handle_t *handle,
+				      u64 prev_blk,
+				      u64 new_blk,
+				      u32 *first_hash)
 {
 	u16 blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
 	int ret, credits = 2 * blk_per_bucket;
@@ -3614,8 +3703,8 @@ static int ocfs2_half_xattr_cluster(struct inode *inode,
 	}
 
 	/* Move half of the xattr in start_blk to the next bucket. */
-	return  ocfs2_half_xattr_bucket(inode, handle, prev_blk,
-					new_blk, first_hash, 1);
+	return  ocfs2_divide_xattr_bucket(inode, handle, prev_blk,
+					  new_blk, first_hash, 1);
 }
 
 /*
@@ -3677,9 +3766,9 @@ static int ocfs2_adjust_xattr_cross_cluster(struct inode *inode,
 						     last_blk, new_blk,
 						     v_start);
 		else {
-			ret = ocfs2_half_xattr_cluster(inode, handle,
-						       last_blk, new_blk,
-						       v_start);
+			ret = ocfs2_divide_xattr_cluster(inode, handle,
+							 last_blk, new_blk,
+							 v_start);
 
 			if ((*header_bh)->b_blocknr == last_blk && extend)
 				*extend = 0;
@@ -3877,8 +3966,8 @@ static int ocfs2_extend_xattr_bucket(struct inode *inode,
 	}
 
 	/* Move half of the xattr in start_blk to the next bucket. */
-	ret = ocfs2_half_xattr_bucket(inode, handle, start_blk,
-				      start_blk + blk_per_bucket, NULL, 0);
+	ret = ocfs2_divide_xattr_bucket(inode, handle, start_blk,
+					start_blk + blk_per_bucket, NULL, 0);
 
 	le16_add_cpu(&first_xh->xh_num_buckets, 1);
 	ocfs2_journal_dirty(handle, first_bh);
@@ -4554,11 +4643,21 @@ out:
 	return ret;
 }
 
-/* check whether the xattr bucket is filled up with the same hash value. */
+/*
+ * check whether the xattr bucket is filled up with the same hash value.
+ * If we want to insert the xattr with the same hash, return -ENOSPC.
+ * If we want to insert a xattr with different hash value, go ahead
+ * and ocfs2_divide_xattr_bucket will handle this.
+ */
 static int ocfs2_check_xattr_bucket_collision(struct inode *inode,
-					      struct ocfs2_xattr_bucket *bucket)
+					      struct ocfs2_xattr_bucket *bucket,
+					      const char *name)
 {
 	struct ocfs2_xattr_header *xh = bucket->xh;
+	u32 name_hash = ocfs2_xattr_name_hash(inode, name, strlen(name));
+
+	if (name_hash != le32_to_cpu(xh->xh_entries[0].xe_name_hash))
+		return 0;
 
 	if (xh->xh_entries[le16_to_cpu(xh->xh_count) - 1].xe_name_hash ==
 	    xh->xh_entries[0].xe_name_hash) {
@@ -4684,7 +4783,9 @@ try_again:
 		 * one bucket's worth, so check it here whether we need to
 		 * add a new bucket for the insert.
 		 */
-		ret = ocfs2_check_xattr_bucket_collision(inode, &xs->bucket);
+		ret = ocfs2_check_xattr_bucket_collision(inode,
+							 &xs->bucket,
+							 xi->name);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
-- 
1.5.4.GIT

[Ocfs2-devel] [PATCH 3/3] ocfs2/xattr: Proper hash collision handle in bucket division.v2

[Joel Becker]

On Fri, Oct 17, 2008 at 08:54:36AM +0800, Tao Ma wrote:
> In ocfs2/xattr, we must make sure the xattrs which have the same
> hash value exist in the same bucket so that the search schema can
> work. But in the old implementation, when we want to extend a bucket,
> we just move half number of xattrs to the new bucket. This works
> in most cases, but if we are lucky enough we will make 2 xattrs
> into 2 different buckets. This cause a problem that the xattr
> existing in the previous bucket can be found any more. This patch
> fix this problem by finding the right position during extending
> the bucket and extend an empty bucket if needed.

	This totally needs fixing, yes.

> @@ -3267,25 +3267,92 @@ static int ocfs2_read_xattr_bucket(struct inode *inode,
>  }
>  
>  /*
> - * Move half num of the xattrs in old bucket(blk) to new bucket(new_blk).
> + * Find the suitable pos when we divide a bucket into 2.
> + *
> + * We have to make sure the xattrs with the same hash value exist in the same
> + * bucket. So we start from the middle pos and go backward first and then
> + * forward. If all the xattrs in this bucket have the same hash value, just
> + * return count-1 and let the caller handle this.
> + */
> +static u16 ocfs2_xattr_find_divide_pos(struct ocfs2_xattr_header *xh)
> +{
> +	u16 middle, pos, count = le16_to_cpu(xh->xh_count);
> +	struct ocfs2_xattr_entry *xe, *xe_low;
> +
> +	BUG_ON(count == 0);
> +	middle = count / 2;
> +
> +	/*
> +	 * If the bucket only have one xattr(for blocksize == 512 and large
> +	 * xattr name, it could be possible), let it go.
> +	 */
> +	if (middle == 0)
> +		return middle;
> +
> +	/*
> +	 * Find a xe before middle which doesn't have the same hash value
> +	 * with the previous one.
> +	 */
> +	pos = middle;
> +	while (pos > 0) {
> +		xe = &xh->xh_entries[pos];
> +		xe_low = &xh->xh_entries[pos - 1];
> +		if (le32_to_cpu(xe_low->xe_name_hash) !=
> +		    le32_to_cpu(xe->xe_name_hash))
> +			return pos;
> +
> +		pos--;
> +	}
> +
> +	/* now all the xe before middle(include it) has the same hash value. */
> +	if (middle == count - 1)
> +		return middle;
> +
> +	/*
> +	 * Find a xe after middle which doesn't have the same hash value
> +	 * with the later one.
> +	 */
> +	pos = middle;
> +	while (pos < count - 1) {
> +		xe_low = &xh->xh_entries[pos];
> +		xe = &xh->xh_entries[pos + 1];
> +		if (le32_to_cpu(xe_low->xe_name_hash) !=
> +		    le32_to_cpu(xe->xe_name_hash))
> +			return pos + 1;
> +
> +		pos++;
> +	}
> +
> +	/* now all the xe in the bucket hash the same hash value. */
> +	return count - 1;
> +}

	Do we really need such a complex function?  Some of your
bailouts don't even help much.  I *think* what you are doing here is
trying to find the hash value change closest to the middle.
Also, you don't need to pass around u16s.  It just makes the code
more complex.
	What about:

/* In an ocfs2_xattr_header, are the hashes of entries n and n+1 equal?. */
static inline int xe_cmp(struct ocfs2_xattr_entry *entres, int n)
{
	return le32_to_cpu(entries[n].xe_name_hash) ==
		le32_to_cpu(entries[n + 1].xe_name_hash);
}

/*
 * If this ocfs2_xattr_header covers more than one hash value, find a
 * place where the hash value changes.  Try to find the most even split.
 */
static int ocfs2_xattr_find_divide_pos(struct ocfs2_xattr_header *xh)
{
	int i, middle, pos = 0, count = le16_to_cpu(xh->xh_count);

	middle = count / 2;

	/* It's important that this loop does nothing for count < 2 */
	for (i = 0; i < (count - 1), i++) {
		if (xe_cmp(xh->xh_entries, i)) {
			/* If the hash flips at the middle, it's perfect */
			if (i == middle)
				return i;

			/* Cache the highest pos before middle */
			if (i < middle)
				pos = i;
			else {
				/* Which is closer to middle, pos or i? */
				if ((middle - pos) > (i - middle))
					pos = i;
				return pos;
			}
		}
	}

	/* If pos is 0, every entry had the same hash. */
	if (!pos)
		pos = count;

	return pos;
}

> +
> +/*
> + * Move some xattrs in old bucket(blk) to new bucket(new_blk).
>   * first_hash will record the 1st hash of the new bucket.
> + *
> + * Normally half nums will be moved. But we have to make sure
> + * that the xattr with same hash value is stored in the same
> + * bucket. If all the xattrs in this bucket has the same hash
> + * value, the new bucket will be initialized as an empty one
> + * and the first_hash will be initialized as (hash_value+1).
>   */
> -static int ocfs2_half_xattr_bucket(struct inode *inode,
> -				   handle_t *handle,
> -				   u64 blk,
> -				   u64 new_blk,
> -				   u32 *first_hash,
> -				   int new_bucket_head)
> +static int ocfs2_divide_xattr_bucket(struct inode *inode,
> +				    handle_t *handle,
> +				    u64 blk,
> +				    u64 new_blk,
> +				    u32 *first_hash,
> +				    int new_bucket_head)
>  {
>  	int ret, i;
> -	u16 count, start, len, name_value_len, xe_len, name_offset;
> +	u16 count, start, len, name_value_len = 0, xe_len, name_offset = 0;

	Most of these don't need to be u16s.  You ensure the range in
other ways.

>  	u16 blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
>  	struct buffer_head **s_bhs, **t_bhs = NULL;
>  	struct ocfs2_xattr_header *xh;
>  	struct ocfs2_xattr_entry *xe;
>  	int blocksize = inode->i_sb->s_blocksize;
>  
> -	mlog(0, "move half of xattrs from bucket %llu to %llu\n",
> +	mlog(0, "move some of xattrs from bucket %llu to %llu\n",
>  	     blk, new_blk);
>  
>  	s_bhs = kcalloc(blk_per_bucket, sizeof(struct buffer_head *), GFP_NOFS);
> @@ -3326,14 +3393,33 @@ static int ocfs2_half_xattr_bucket(struct inode *inode,
>  		}
>  	}
>  
> -	/* copy the whole bucket to the new first. */
> -	for (i = 0; i < blk_per_bucket; i++)
> -		memcpy(t_bhs[i]->b_data, s_bhs[i]->b_data, blocksize);
> +	xh = (struct ocfs2_xattr_header *)s_bhs[0]->b_data;
> +	count = le16_to_cpu(xh->xh_count);
> +	start = ocfs2_xattr_find_divide_pos(xh);
> +	xe = &xh->xh_entries[start];
>  
>  	/* update the new bucket. */
>  	xh = (struct ocfs2_xattr_header *)t_bhs[0]->b_data;
> -	count = le16_to_cpu(xh->xh_count);
> -	start = count / 2;
> +
> +	if (start == count - 1) {

	This becomes (start == count) if you use my function above.

> +		/*
> +		 * initialized a new empty bucket here.
> +		 * The hash value is set as one larger than
> +		 * that of the last entry in the previous bucket.
> +		 */
> +		for (i = 0; i < blk_per_bucket; i++)
> +			memset(t_bhs[i]->b_data, 0, blocksize);
> +
> +		xh->xh_free_start = cpu_to_le16(blocksize);
> +		xh->xh_entries[0].xe_name_hash = xe->xe_name_hash;
> +		le32_add_cpu(&xh->xh_entries[0].xe_name_hash, 1);
> +
> +		goto set_num_buckets;
> +	}
> +
> +	/* copy the whole bucket to the new first. */
> +	for (i = 0; i < blk_per_bucket; i++)
> +		memcpy(t_bhs[i]->b_data, s_bhs[i]->b_data, blocksize);
>  
>  	/*
>  	 * Calculate the total name/value len and xh_free_start for
> @@ -3390,6 +3476,7 @@ static int ocfs2_half_xattr_bucket(struct inode *inode,
>  			xh->xh_free_start = xe->xe_name_offset;
>  	}
>  
> +set_num_buckets:
>  	/* set xh->xh_num_buckets for the new xh. */
>  	if (new_bucket_head)
>  		xh->xh_num_buckets = cpu_to_le16(1);
> @@ -3406,10 +3493,12 @@ static int ocfs2_half_xattr_bucket(struct inode *inode,
>  	if (first_hash)
>  		*first_hash = le32_to_cpu(xh->xh_entries[0].xe_name_hash);
>  
> -	/*
> -	 * Now only update the 1st block of the old bucket.
> -	 * Please note that the entry has been sorted already above.
> +	/* Now only update the 1st block of the old bucket.
> +	 * If we just add a new empty bucket after it, no needs to modify it.
>  	 */
> +	if (start == count - 1)

	Same here.

> +		goto out;
> +
>  	xh = (struct ocfs2_xattr_header *)s_bhs[0]->b_data;
>  	memset(&xh->xh_entries[start], 0,
>  	       sizeof(struct ocfs2_xattr_entry) * (count - start));
> @@ -3592,15 +3681,15 @@ out:
>  }
>  
>  /*
> - * Move half of the xattrs in this cluster to the new cluster.
> + * Move some xattrs in this cluster to the new cluster.
>   * This function should only be called when bucket size == cluster size.
>   * Otherwise ocfs2_mv_xattr_bucket_cross_cluster should be used instead.
>   */
> -static int ocfs2_half_xattr_cluster(struct inode *inode,
> -				    handle_t *handle,
> -				    u64 prev_blk,
> -				    u64 new_blk,
> -				    u32 *first_hash)
> +static int ocfs2_divide_xattr_cluster(struct inode *inode,
> +				      handle_t *handle,
> +				      u64 prev_blk,
> +				      u64 new_blk,
> +				      u32 *first_hash)
>  {
>  	u16 blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
>  	int ret, credits = 2 * blk_per_bucket;
> @@ -3614,8 +3703,8 @@ static int ocfs2_half_xattr_cluster(struct inode *inode,
>  	}
>  
>  	/* Move half of the xattr in start_blk to the next bucket. */
> -	return  ocfs2_half_xattr_bucket(inode, handle, prev_blk,
> -					new_blk, first_hash, 1);
> +	return  ocfs2_divide_xattr_bucket(inode, handle, prev_blk,
> +					  new_blk, first_hash, 1);
>  }
>  
>  /*
> @@ -3677,9 +3766,9 @@ static int ocfs2_adjust_xattr_cross_cluster(struct inode *inode,
>  						     last_blk, new_blk,
>  						     v_start);
>  		else {
> -			ret = ocfs2_half_xattr_cluster(inode, handle,
> -						       last_blk, new_blk,
> -						       v_start);
> +			ret = ocfs2_divide_xattr_cluster(inode, handle,
> +							 last_blk, new_blk,
> +							 v_start);
>  
>  			if ((*header_bh)->b_blocknr == last_blk && extend)
>  				*extend = 0;
> @@ -3877,8 +3966,8 @@ static int ocfs2_extend_xattr_bucket(struct inode *inode,
>  	}
>  
>  	/* Move half of the xattr in start_blk to the next bucket. */
> -	ret = ocfs2_half_xattr_bucket(inode, handle, start_blk,
> -				      start_blk + blk_per_bucket, NULL, 0);
> +	ret = ocfs2_divide_xattr_bucket(inode, handle, start_blk,
> +					start_blk + blk_per_bucket, NULL, 0);
>  
>  	le16_add_cpu(&first_xh->xh_num_buckets, 1);
>  	ocfs2_journal_dirty(handle, first_bh);
> @@ -4554,11 +4643,21 @@ out:
>  	return ret;
>  }
>  
> -/* check whether the xattr bucket is filled up with the same hash value. */
> +/*
> + * check whether the xattr bucket is filled up with the same hash value.
> + * If we want to insert the xattr with the same hash, return -ENOSPC.
> + * If we want to insert a xattr with different hash value, go ahead
> + * and ocfs2_divide_xattr_bucket will handle this.
> + */
>  static int ocfs2_check_xattr_bucket_collision(struct inode *inode,
> -					      struct ocfs2_xattr_bucket *bucket)
> +					      struct ocfs2_xattr_bucket *bucket,
> +					      const char *name)
>  {
>  	struct ocfs2_xattr_header *xh = bucket->xh;
> +	u32 name_hash = ocfs2_xattr_name_hash(inode, name, strlen(name));
> +
> +	if (name_hash != le32_to_cpu(xh->xh_entries[0].xe_name_hash))
> +		return 0;
>  
>  	if (xh->xh_entries[le16_to_cpu(xh->xh_count) - 1].xe_name_hash ==
>  	    xh->xh_entries[0].xe_name_hash) {
> @@ -4684,7 +4783,9 @@ try_again:
>  		 * one bucket's worth, so check it here whether we need to
>  		 * add a new bucket for the insert.
>  		 */
> -		ret = ocfs2_check_xattr_bucket_collision(inode, &xs->bucket);
> +		ret = ocfs2_check_xattr_bucket_collision(inode,
> +							 &xs->bucket,
> +							 xi->name);
>  		if (ret) {
>  			mlog_errno(ret);
>  			goto out;
> -- 
> 1.5.4.GIT
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/o
-- 

"Vote early and vote often." 
        - Al Capone

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

[Ocfs2-devel] [PATCH 3/3] ocfs2/xattr: Proper hash collision handle in bucket division.v2

[Tao Ma]

Joel Becker wrote:
>> @@ -3267,25 +3267,92 @@ static int ocfs2_read_xattr_bucket(struct inode *inode,
>>  }
>>  
>>  /*
>> - * Move half num of the xattrs in old bucket(blk) to new bucket(new_blk).
>> + * Find the suitable pos when we divide a bucket into 2.
>> + *
>> + * We have to make sure the xattrs with the same hash value exist in the same
>> + * bucket. So we start from the middle pos and go backward first and then
>> + * forward. If all the xattrs in this bucket have the same hash value, just
>> + * return count-1 and let the caller handle this.
>> + */
>> +static u16 ocfs2_xattr_find_divide_pos(struct ocfs2_xattr_header *xh)
>> +{
>> +	u16 middle, pos, count = le16_to_cpu(xh->xh_count);
>> +	struct ocfs2_xattr_entry *xe, *xe_low;
>> +
>> +	BUG_ON(count == 0);
>> +	middle = count / 2;
>> +
>> +	/*
>> +	 * If the bucket only have one xattr(for blocksize == 512 and large
>> +	 * xattr name, it could be possible), let it go.
>> +	 */
>> +	if (middle == 0)
>> +		return middle;
>> +
>> +	/*
>> +	 * Find a xe before middle which doesn't have the same hash value
>> +	 * with the previous one.
>> +	 */
>> +	pos = middle;
>> +	while (pos > 0) {
>> +		xe = &xh->xh_entries[pos];
>> +		xe_low = &xh->xh_entries[pos - 1];
>> +		if (le32_to_cpu(xe_low->xe_name_hash) !=
>> +		    le32_to_cpu(xe->xe_name_hash))
>> +			return pos;
>> +
>> +		pos--;
>> +	}
>> +
>> +	/* now all the xe before middle(include it) has the same hash value. */
>> +	if (middle == count - 1)
>> +		return middle;
>> +
>> +	/*
>> +	 * Find a xe after middle which doesn't have the same hash value
>> +	 * with the later one.
>> +	 */
>> +	pos = middle;
>> +	while (pos < count - 1) {
>> +		xe_low = &xh->xh_entries[pos];
>> +		xe = &xh->xh_entries[pos + 1];
>> +		if (le32_to_cpu(xe_low->xe_name_hash) !=
>> +		    le32_to_cpu(xe->xe_name_hash))
>> +			return pos + 1;
>> +
>> +		pos++;
>> +	}
>> +
>> +	/* now all the xe in the bucket hash the same hash value. */
>> +	return count - 1;
>> +}
> 
> 	Do we really need such a complex function?  Some of your
> bailouts don't even help much.  I *think* what you are doing here is
> trying to find the hash value change closest to the middle.
> Also, you don't need to pass around u16s.  It just makes the code
> more complex.
> 	What about:
> 
> /* In an ocfs2_xattr_header, are the hashes of entries n and n+1 equal?. */
> static inline int xe_cmp(struct ocfs2_xattr_entry *entres, int n)
> {
> 	return le32_to_cpu(entries[n].xe_name_hash) ==
> 		le32_to_cpu(entries[n + 1].xe_name_hash);
> }
> 
> /*
>  * If this ocfs2_xattr_header covers more than one hash value, find a
>  * place where the hash value changes.  Try to find the most even split.
>  */
> static int ocfs2_xattr_find_divide_pos(struct ocfs2_xattr_header *xh)
> {
> 	int i, middle, pos = 0, count = le16_to_cpu(xh->xh_count);
> 
> 	middle = count / 2;
> 
> 	/* It's important that this loop does nothing for count < 2 */
> 	for (i = 0; i < (count - 1), i++) {
> 		if (xe_cmp(xh->xh_entries, i)) {
> 			/* If the hash flips at the middle, it's perfect */
> 			if (i == middle)
> 				return i;
> 
> 			/* Cache the highest pos before middle */
> 			if (i < middle)
> 				pos = i;
> 			else {
> 				/* Which is closer to middle, pos or i? */
> 				if ((middle - pos) > (i - middle))
> 					pos = i;
> 				return pos;
> 			}
> 		}
> 	}
> 
> 	/* If pos is 0, every entry had the same hash. */
> 	if (!pos)
> 		pos = count;
> 
> 	return pos;
> }
Don't agree. Actually in most cases we won't be so lucky to hit this 
situation(I run the test scripts many times and hit one), so I think 
most times we will just return "middle" and my function go out 
immediately, it is O(1). You function suppose that the hash collision 
happens frequently and iterate the half of the xh_entries so it is O(n).

Regards,
Tao

[Ocfs2-devel] [PATCH 3/3] ocfs2/xattr: Proper hash collision handle in bucket division.v2

[Joel Becker]

On Fri, Oct 24, 2008 at 08:52:33AM +0800, Tao Ma wrote:
> Joel Becker wrote:
>> 	Do we really need such a complex function?  Some of your
>> bailouts don't even help much.  I *think* what you are doing here is
>> trying to find the hash value change closest to the middle.
>> Also, you don't need to pass around u16s.  It just makes the code
>> more complex.
>> 	What about:
>>
>> /* In an ocfs2_xattr_header, are the hashes of entries n and n+1 equal?. */
>> static inline int xe_cmp(struct ocfs2_xattr_entry *entres, int n)
>> {
>> 	return le32_to_cpu(entries[n].xe_name_hash) ==
>> 		le32_to_cpu(entries[n + 1].xe_name_hash);
>> }

<snip>

> Don't agree. Actually in most cases we won't be so lucky to hit this  
> situation(I run the test scripts many times and hit one), so I think  
> most times we will just return "middle" and my function go out  
> immediately, it is O(1). You function suppose that the hash collision  
> happens frequently and iterate the half of the xh_entries so it is O(n).

	Are you saying that you usually have only one xattr per bucket,
even with many xattrs?  Because if we have n xattrs per bucket, your
function will walk down from middle to 0 (half the bucket) unless n is
1.
	Your function would be much more understandable even if it just
used my xe_cmp() function.  Then some comment about what you are trying
to achieve (I guessed at that), and perhaps a mention of the common
case.

Joel

-- 

"War doesn't determine who's right; war determines who's left."

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

[Ocfs2-devel] [PATCH 3/3] ocfs2/xattr: Proper hash collision handle in bucket division.v2

[Tao Ma]

Joel Becker wrote:
> On Fri, Oct 24, 2008 at 08:52:33AM +0800, Tao Ma wrote:
>> Joel Becker wrote:
>>> 	Do we really need such a complex function?  Some of your
>>> bailouts don't even help much.  I *think* what you are doing here is
>>> trying to find the hash value change closest to the middle.
>>> Also, you don't need to pass around u16s.  It just makes the code
>>> more complex.
>>> 	What about:
>>>
>>> /* In an ocfs2_xattr_header, are the hashes of entries n and n+1 equal?. */
>>> static inline int xe_cmp(struct ocfs2_xattr_entry *entres, int n)
>>> {
>>> 	return le32_to_cpu(entries[n].xe_name_hash) ==
>>> 		le32_to_cpu(entries[n + 1].xe_name_hash);
>>> }
> 
> <snip>
> 
>> Don't agree. Actually in most cases we won't be so lucky to hit this  
>> situation(I run the test scripts many times and hit one), so I think  
>> most times we will just return "middle" and my function go out  
>> immediately, it is O(1). You function suppose that the hash collision  
>> happens frequently and iterate the half of the xh_entries so it is O(n).
> 
> 	Are you saying that you usually have only one xattr per bucket,
> even with many xattrs?  Because if we have n xattrs per bucket, your
> function will walk down from middle to 0 (half the bucket) unless n is
> 1.
No. See ocfs2_xattr_find_divide_pos.
+	/*
+	 * Find a xe before middle which doesn't have the same hash alue
+	 * with the previous one.
+	 */
+	pos = middle;
+	while (pos > 0) {
+		xe = &xh->xh_entries[pos];
+		xe_low = &xh->xh_entries[pos - 1];
+		if (le32_to_cpu(xe_low->xe_name_hash) !=
+		    le32_to_cpu(xe->xe_name_hash))
+			return pos;
+
+		pos--;
+	}
In most cases, it will return 'pos' immediately since the hash value 
isn't the same.

> 	Your function would be much more understandable even if it just
> used my xe_cmp() function.  Then some comment about what you are trying
> to achieve (I guessed at that), and perhaps a mention of the common
> case.
OK, I will add some comments for it. As for xe_cmp, can I use "cmp_xe" 
which already exists in xattr.c?

Regards,
Tao

[Ocfs2-devel] [PATCH 3/3] ocfs2/xattr: Proper hash collision handle in bucket division.v2

[Joel Becker]

On Fri, Oct 24, 2008 at 09:28:00AM +0800, Tao Ma wrote:
> Joel Becker wrote:
>> 	Are you saying that you usually have only one xattr per bucket,
>> even with many xattrs?  Because if we have n xattrs per bucket, your
>> function will walk down from middle to 0 (half the bucket) unless n is
>> 1.
> No. See ocfs2_xattr_find_divide_pos.

	I realized on the drive home that, if the common case is no
duplicate hash values, the first loop triggers on its first step.  I
think that's what you are saying.
	But boy, it's not obvious that's what it is doing.  And then you
have that weird (middle == count - 1) check that makes no sense.  And
then you go the other way.  I'd love to find a set of comments and code
that makes it more understandable.

>> 	Your function would be much more understandable even if it just
>> used my xe_cmp() function.  Then some comment about what you are trying
>> to achieve (I guessed at that), and perhaps a mention of the common
>> case.
> OK, I will add some comments for it. As for xe_cmp, can I use "cmp_xe" 
> which already exists in xattr.c?

	I have an idea to do what you're doing, but cleaner.  I'll look
at cmp_xe and try again in the morning.

Joel

-- 

"The one important thing i have learned over the years is the
 difference between taking one's work seriously and taking one's self
 seriously.  The first is imperative and the second is disastrous."
	-Margot Fonteyn

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

[Ocfs2-devel] [PATCH 3/3] ocfs2/xattr: Proper hash collision handle in bucket division.v2

[Tao Ma]

Joel Becker wrote:
> On Fri, Oct 24, 2008 at 09:28:00AM +0800, Tao Ma wrote:
>> Joel Becker wrote:
>>> 	Are you saying that you usually have only one xattr per bucket,
>>> even with many xattrs?  Because if we have n xattrs per bucket, your
>>> function will walk down from middle to 0 (half the bucket) unless n is
>>> 1.
>> No. See ocfs2_xattr_find_divide_pos.
> 
> 	I realized on the drive home that, if the common case is no
> duplicate hash values, the first loop triggers on its first step.  I
> think that's what you are saying.
> 	But boy, it's not obvious that's what it is doing.  And then you
> have that weird (middle == count - 1) check that makes no sense.  And
> then you go the other way.  I'd love to find a set of comments and code
> that makes it more understandable.
> 
>>> 	Your function would be much more understandable even if it just
>>> used my xe_cmp() function.  Then some comment about what you are trying
>>> to achieve (I guessed at that), and perhaps a mention of the common
>>> case.
>> OK, I will add some comments for it. As for xe_cmp, can I use "cmp_xe" 
>> which already exists in xattr.c?
> 
> 	I have an idea to do what you're doing, but cleaner.  I'll look
> at cmp_xe and try again in the morning.
cool, so waiting for your perfect code. ;)

Regards,
Tao

[Ocfs2-devel] [PATCH 3/3] ocfs2/xattr: Proper hash collision handle in bucket division.v2

[Joel Becker]

On Fri, Oct 24, 2008 at 02:15:35PM +0800, Tao Ma wrote:
> Joel Becker wrote:
>> 	I have an idea to do what you're doing, but cleaner.  I'll look
>> at cmp_xe and try again in the morning.
> cool, so waiting for your perfect code. ;)

Zing!  You got me :-)

Joel

-- 

"It is not the function of our government to keep the citizen from
 falling into error; it is the function of the citizen to keep the
 government from falling into error."
	- Robert H. Jackson

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

[Ocfs2-devel] [PATCH 3/3] ocfs2/xattr: Proper hash collision handle in bucket division.v2

[Joel Becker]

On Fri, Oct 24, 2008 at 12:43:53AM -0700, Joel Becker wrote:
> On Fri, Oct 24, 2008 at 02:15:35PM +0800, Tao Ma wrote:
> > Joel Becker wrote:
> >> 	I have an idea to do what you're doing, but cleaner.  I'll look
> >> at cmp_xe and try again in the morning.
> > cool, so waiting for your perfect code. ;)
> 
> Zing!  You got me :-)

	How's this (untested)?

/*
 * If this ocfs2_xattr_header covers more than one hash value, find a
 * place where the hash value changes.  Try to find the most even split.
 * The most common case is that all entries have different hash values,
 * and the first check we make will find a place to split.
 */
static int ocfs2_xattr_find_divide_pos(struct ocfs2_xattr_header *xh)
{
	struct ocfs2_xattr_entry *entries = &xh->xh_entries;
	int count = le16_to_cpu(xh->xh_count);
	int delta, middle = count / 2;

	/*  
	 * We start at the middle.  Each step gets farther away in both
	 * directions.  We therefore hit the change in hash value
	 * nearest to the middle.  Note that this loop does not execute for
	 * count < 2.
	 */
	for (delta = 0; delta < middle; delta++) {
		/* Let's check delta earlier than middle */
		if (!cmp_ex(&entries[middle - delta - 1], 
			    &entries[middle - delta]))
			return middle - delta;
		/* For even counts, don't walk off the end */
		if ((middle + delta + 1) == count)
			continue;
		/* Now try delta past middle */
		if (!cmp_ex(&entries[middle + delta],
			    &entries[middle + delta + 1]))
			return middle + delta + 1;
	}

	/* Every entry had the same hash */
	return count;
}

Joel

-- 

"Reader, suppose you were and idiot.  And suppose you were a member of
 Congress.  But I repeat myself."
	- Mark Twain

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

[Ocfs2-devel] [PATCH 3/3] ocfs2/xattr: Proper hash collision handle in bucket division.v2

[Tao Ma]

Joel Becker wrote:
> On Fri, Oct 24, 2008 at 12:43:53AM -0700, Joel Becker wrote:
>> On Fri, Oct 24, 2008 at 02:15:35PM +0800, Tao Ma wrote:
>>> Joel Becker wrote:
>>>> 	I have an idea to do what you're doing, but cleaner.  I'll look
>>>> at cmp_xe and try again in the morning.
>>> cool, so waiting for your perfect code. ;)
>> Zing!  You got me :-)
> 
> 	How's this (untested)?
> 
> /*
>  * If this ocfs2_xattr_header covers more than one hash value, find a
>  * place where the hash value changes.  Try to find the most even split.
>  * The most common case is that all entries have different hash values,
>  * and the first check we make will find a place to split.
>  */
> static int ocfs2_xattr_find_divide_pos(struct ocfs2_xattr_header *xh)
> {
> 	struct ocfs2_xattr_entry *entries = &xh->xh_entries;
> 	int count = le16_to_cpu(xh->xh_count);
> 	int delta, middle = count / 2;
> 
> 	/*  
> 	 * We start at the middle.  Each step gets farther away in both
> 	 * directions.  We therefore hit the change in hash value
> 	 * nearest to the middle.  Note that this loop does not execute for
> 	 * count < 2.
> 	 */
> 	for (delta = 0; delta < middle; delta++) {
> 		/* Let's check delta earlier than middle */
> 		if (!cmp_ex(&entries[middle - delta - 1], 
> 			    &entries[middle - delta]))
> 			return middle - delta;
> 		/* For even counts, don't walk off the end */
> 		if ((middle + delta + 1) == count)
> 			continue;
> 		/* Now try delta past middle */
> 		if (!cmp_ex(&entries[middle + delta],
> 			    &entries[middle + delta + 1]))
> 			return middle + delta + 1;
> 	}
> 
> 	/* Every entry had the same hash */
> 	return count;
> }
yeah, this looks perfect and more efficient.
I will modify my patch and then test it.

Thanks.

Regards,
Tao

[Ocfs2-devel] [PATCH 3/3] ocfs2/xattr: Proper hash collision handle in bucket division.v2

[Joel Becker]

On Fri, Oct 24, 2008 at 05:02:28PM +0800, Tao Ma wrote:
>
>
> Joel Becker wrote:
>> On Fri, Oct 24, 2008 at 12:43:53AM -0700, Joel Becker wrote:
>>> On Fri, Oct 24, 2008 at 02:15:35PM +0800, Tao Ma wrote:
>>>> Joel Becker wrote:
>>>>> 	I have an idea to do what you're doing, but cleaner.  I'll look
>>>>> at cmp_xe and try again in the morning.
>>>> cool, so waiting for your perfect code. ;)
>>> Zing!  You got me :-)
>> 	How's this (untested)?
>> /*
>>  * If this ocfs2_xattr_header covers more than one hash value, find a
>>  * place where the hash value changes.  Try to find the most even split.
>>  * The most common case is that all entries have different hash values,
>>  * and the first check we make will find a place to split.
>>  */
>> static int ocfs2_xattr_find_divide_pos(struct ocfs2_xattr_header *xh)
>> {
>> 	struct ocfs2_xattr_entry *entries = &xh->xh_entries;
>> 	int count = le16_to_cpu(xh->xh_count);
>> 	int delta, middle = count / 2;
>> 	/*  	 * We start at the middle.  Each step gets farther away in both
>> 	 * directions.  We therefore hit the change in hash value
>> 	 * nearest to the middle.  Note that this loop does not execute for
>> 	 * count < 2.
>> 	 */
>> 	for (delta = 0; delta < middle; delta++) {
>> 		/* Let's check delta earlier than middle */
>> 		if (!cmp_ex(&entries[middle - delta - 1], 			    &entries[middle - 
>> delta]))
>> 			return middle - delta;
>> 		/* For even counts, don't walk off the end */
>> 		if ((middle + delta + 1) == count)
>> 			continue;
>> 		/* Now try delta past middle */
>> 		if (!cmp_ex(&entries[middle + delta],
>> 			    &entries[middle + delta + 1]))
>> 			return middle + delta + 1;
>> 	}
>> 	/* Every entry had the same hash */
>> 	return count;
>> }
> yeah, this looks perfect and more efficient.
> I will modify my patch and then test it.

	Haha, it has a bug!  Those cmp_ex() calls should not have the
'!' in front of them.  We're looking for differences, not matches.  Take
out those '!'s.

Joel (sorry about the bug)

-- 

"Every new beginning comes from some other beginning's end."

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

[Ocfs2-devel] [PATCH 3/3] ocfs2/xattr: Proper hash collision handle in bucket division.v2

[Tao Ma]

Joel Becker wrote:
> On Fri, Oct 24, 2008 at 05:02:28PM +0800, Tao Ma wrote:
>>
>> Joel Becker wrote:
>>> On Fri, Oct 24, 2008 at 12:43:53AM -0700, Joel Becker wrote:
>>>> On Fri, Oct 24, 2008 at 02:15:35PM +0800, Tao Ma wrote:
>>>>> Joel Becker wrote:
>>>>>> 	I have an idea to do what you're doing, but cleaner.  I'll look
>>>>>> at cmp_xe and try again in the morning.
>>>>> cool, so waiting for your perfect code. ;)
>>>> Zing!  You got me :-)
>>> 	How's this (untested)?
>>> /*
>>>  * If this ocfs2_xattr_header covers more than one hash value, find a
>>>  * place where the hash value changes.  Try to find the most even split.
>>>  * The most common case is that all entries have different hash values,
>>>  * and the first check we make will find a place to split.
>>>  */
>>> static int ocfs2_xattr_find_divide_pos(struct ocfs2_xattr_header *xh)
>>> {
>>> 	struct ocfs2_xattr_entry *entries = &xh->xh_entries;
>>> 	int count = le16_to_cpu(xh->xh_count);
>>> 	int delta, middle = count / 2;
>>> 	/*  	 * We start at the middle.  Each step gets farther away in both
>>> 	 * directions.  We therefore hit the change in hash value
>>> 	 * nearest to the middle.  Note that this loop does not execute for
>>> 	 * count < 2.
>>> 	 */
>>> 	for (delta = 0; delta < middle; delta++) {
>>> 		/* Let's check delta earlier than middle */
>>> 		if (!cmp_ex(&entries[middle - delta - 1], 			    &entries[middle - 
>>> delta]))
>>> 			return middle - delta;
>>> 		/* For even counts, don't walk off the end */
>>> 		if ((middle + delta + 1) == count)
>>> 			continue;
>>> 		/* Now try delta past middle */
>>> 		if (!cmp_ex(&entries[middle + delta],
>>> 			    &entries[middle + delta + 1]))
>>> 			return middle + delta + 1;
>>> 	}
>>> 	/* Every entry had the same hash */
>>> 	return count;
>>> }
>> yeah, this looks perfect and more efficient.
>> I will modify my patch and then test it.
> 
> 	Haha, it has a bug!  Those cmp_ex() calls should not have the
> '!' in front of them.  We're looking for differences, not matches.  Take
> out those '!'s.
> 
> Joel (sorry about the bug)
Never mind. I will just use your idea and test it. So don't worry about 
any bug. I will handle it. :)

Regards,
Tao