[Ocfs2-devel] [PATCH 0/10] Make ocfs2_extent_tree a first-class object

[Ocfs2-devel] [PATCH 0/10] Make ocfs2_extent_tree a first-class object

[Joel Becker]

The new xattr code introduced a great abstraction, ocfs2_extent_tree.
However, it's hidden in alloc.c.  Tao said he wanted to keep it there,
but I think it works great as a first-class object.

Callers into alloc.c can just fill in an ocfs2_extent_tree with the
appropriate ocfs2_get_*_extent_tree() function, then pass it to all
alloc.c functions.  Those functions no longer need individual root_bh,
root_el, et_type, or void*private arguments.  People filling in dinode
extent trees don't even need to pass NULL for that private.

The series first adds structure prefixes to ocfs2_extent_tree and
ocfs2_extent_tree_operations.  Next, it turns 'private' into 'object',
calculating object as bh->b_data if it was NULL.  Third, we calculate
the other fields (root_el, max_leaf_extents) in operations rather than
if blocks.  Finally, we remove the et_type, the if block, and start
using extent trees as first class objects.

Check out the last patch, which is really the payoff, to see what I'm
going for.

Joel

[Ocfs2-devel] [PATCH 01/10] ocfs2: Prefix the extent tree operations structure.

[Joel Becker]

The ocfs2_extent_tree_operations structure gains a field prefix on its
members.  The ->eo_sanity_check() operation gains a wrapper function for
completeness.  All of the extent tree operation wrappers gain a
consistent name (ocfs2_et_*()).

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/alloc.c |   85 +++++++++++++++++++++++++++++------------------------
 1 files changed, 46 insertions(+), 39 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index 16879bd..9fe49f2 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -65,12 +65,13 @@
 struct ocfs2_extent_tree;
 
 struct ocfs2_extent_tree_operations {
-	void (*set_last_eb_blk) (struct ocfs2_extent_tree *et, u64 blkno);
-	u64 (*get_last_eb_blk) (struct ocfs2_extent_tree *et);
-	void (*update_clusters) (struct inode *inode,
-				 struct ocfs2_extent_tree *et,
-				 u32 new_clusters);
-	int (*sanity_check) (struct inode *inode, struct ocfs2_extent_tree *et);
+	void (*eo_set_last_eb_blk)(struct ocfs2_extent_tree *et,
+				   u64 blkno);
+	u64 (*eo_get_last_eb_blk)(struct ocfs2_extent_tree *et);
+	void (*eo_update_clusters)(struct inode *inode,
+				   struct ocfs2_extent_tree *et,
+				   u32 new_clusters);
+	int (*eo_sanity_check)(struct inode *inode, struct ocfs2_extent_tree *et);
 };
 
 struct ocfs2_extent_tree {
@@ -132,10 +133,10 @@ static int ocfs2_dinode_sanity_check(struct inode *inode,
 }
 
 static struct ocfs2_extent_tree_operations ocfs2_dinode_et_ops = {
-	.set_last_eb_blk	= ocfs2_dinode_set_last_eb_blk,
-	.get_last_eb_blk	= ocfs2_dinode_get_last_eb_blk,
-	.update_clusters	= ocfs2_dinode_update_clusters,
-	.sanity_check		= ocfs2_dinode_sanity_check,
+	.eo_set_last_eb_blk	= ocfs2_dinode_set_last_eb_blk,
+	.eo_get_last_eb_blk	= ocfs2_dinode_get_last_eb_blk,
+	.eo_update_clusters	= ocfs2_dinode_update_clusters,
+	.eo_sanity_check	= ocfs2_dinode_sanity_check,
 };
 
 static void ocfs2_xattr_value_set_last_eb_blk(struct ocfs2_extent_tree *et,
@@ -172,10 +173,10 @@ static int ocfs2_xattr_value_sanity_check(struct inode *inode,
 }
 
 static struct ocfs2_extent_tree_operations ocfs2_xattr_et_ops = {
-	.set_last_eb_blk	= ocfs2_xattr_value_set_last_eb_blk,
-	.get_last_eb_blk	= ocfs2_xattr_value_get_last_eb_blk,
-	.update_clusters	= ocfs2_xattr_value_update_clusters,
-	.sanity_check		= ocfs2_xattr_value_sanity_check,
+	.eo_set_last_eb_blk	= ocfs2_xattr_value_set_last_eb_blk,
+	.eo_get_last_eb_blk	= ocfs2_xattr_value_get_last_eb_blk,
+	.eo_update_clusters	= ocfs2_xattr_value_update_clusters,
+	.eo_sanity_check	= ocfs2_xattr_value_sanity_check,
 };
 
 static void ocfs2_xattr_tree_set_last_eb_blk(struct ocfs2_extent_tree *et,
@@ -214,10 +215,10 @@ static int ocfs2_xattr_tree_sanity_check(struct inode *inode,
 }
 
 static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = {
-	.set_last_eb_blk	= ocfs2_xattr_tree_set_last_eb_blk,
-	.get_last_eb_blk	= ocfs2_xattr_tree_get_last_eb_blk,
-	.update_clusters	= ocfs2_xattr_tree_update_clusters,
-	.sanity_check		= ocfs2_xattr_tree_sanity_check,
+	.eo_set_last_eb_blk	= ocfs2_xattr_tree_set_last_eb_blk,
+	.eo_get_last_eb_blk	= ocfs2_xattr_tree_get_last_eb_blk,
+	.eo_update_clusters	= ocfs2_xattr_tree_update_clusters,
+	.eo_sanity_check	= ocfs2_xattr_tree_sanity_check,
 };
 
 static struct ocfs2_extent_tree*
@@ -265,22 +266,28 @@ static void ocfs2_free_extent_tree(struct ocfs2_extent_tree *et)
 	}
 }
 
-static inline void ocfs2_set_last_eb_blk(struct ocfs2_extent_tree *et,
-					 u64 new_last_eb_blk)
+static inline void ocfs2_et_set_last_eb_blk(struct ocfs2_extent_tree *et,
+					    u64 new_last_eb_blk)
 {
-	et->eops->set_last_eb_blk(et, new_last_eb_blk);
+	et->eops->eo_set_last_eb_blk(et, new_last_eb_blk);
 }
 
-static inline u64 ocfs2_get_last_eb_blk(struct ocfs2_extent_tree *et)
+static inline u64 ocfs2_et_get_last_eb_blk(struct ocfs2_extent_tree *et)
 {
-	return et->eops->get_last_eb_blk(et);
+	return et->eops->eo_get_last_eb_blk(et);
 }
 
-static inline void ocfs2_update_clusters(struct inode *inode,
-					 struct ocfs2_extent_tree *et,
-					 u32 clusters)
+static inline void ocfs2_et_update_clusters(struct inode *inode,
+					    struct ocfs2_extent_tree *et,
+					    u32 clusters)
+{
+	et->eops->eo_update_clusters(inode, et, clusters);
+}
+
+static inline int ocfs2_et_sanity_check(struct inode *inode,
+					struct ocfs2_extent_tree *et)
 {
-	et->eops->update_clusters(inode, et, clusters);
+	return et->eops->eo_sanity_check(inode, et);
 }
 
 static void ocfs2_free_truncate_context(struct ocfs2_truncate_context *tc);
@@ -913,7 +920,7 @@ static int ocfs2_add_branch(struct ocfs2_super *osb,
 
 	/* fe needs a new last extent block pointer, as does the
 	 * next_leaf on the previously last-extent-block. */
-	ocfs2_set_last_eb_blk(et, new_last_eb_blk);
+	ocfs2_et_set_last_eb_blk(et, new_last_eb_blk);
 
 	eb = (struct ocfs2_extent_block *) (*last_eb_bh)->b_data;
 	eb->h_next_leaf_blk = cpu_to_le64(new_last_eb_blk);
@@ -1029,7 +1036,7 @@ static int ocfs2_shift_tree_depth(struct ocfs2_super *osb,
 	/* If this is our 1st tree depth shift, then last_eb_blk
 	 * becomes the allocated extent block */
 	if (root_el->l_tree_depth == cpu_to_le16(1))
-		ocfs2_set_last_eb_blk(et, le64_to_cpu(eb->h_blkno));
+		ocfs2_et_set_last_eb_blk(et, le64_to_cpu(eb->h_blkno));
 
 	status = ocfs2_journal_dirty(handle, et->root_bh);
 	if (status < 0) {
@@ -2436,7 +2443,7 @@ static int ocfs2_rotate_subtree_left(struct inode *inode, handle_t *handle,
 		ocfs2_update_edge_lengths(inode, handle, left_path);
 
 		eb = (struct ocfs2_extent_block *)path_leaf_bh(left_path)->b_data;
-		ocfs2_set_last_eb_blk(et, le64_to_cpu(eb->h_blkno));
+		ocfs2_et_set_last_eb_blk(et, le64_to_cpu(eb->h_blkno));
 
 		/*
 		 * Removal of the extent in the left leaf was skipped
@@ -2697,7 +2704,7 @@ static int ocfs2_remove_rightmost_path(struct inode *inode, handle_t *handle,
 	struct ocfs2_extent_list *el;
 
 
-	ret = et->eops->sanity_check(inode, et);
+	ret = ocfs2_et_sanity_check(inode, et);
 	if (ret)
 		goto out;
 	/*
@@ -2756,7 +2763,7 @@ static int ocfs2_remove_rightmost_path(struct inode *inode, handle_t *handle,
 		ocfs2_update_edge_lengths(inode, handle, left_path);
 
 		eb = (struct ocfs2_extent_block *)path_leaf_bh(left_path)->b_data;
-		ocfs2_set_last_eb_blk(et, le64_to_cpu(eb->h_blkno));
+		ocfs2_et_set_last_eb_blk(et, le64_to_cpu(eb->h_blkno));
 	} else {
 		/*
 		 * 'path' is also the leftmost path which
@@ -2772,7 +2779,7 @@ static int ocfs2_remove_rightmost_path(struct inode *inode, handle_t *handle,
 		el->l_next_free_rec = 0;
 		memset(&el->l_recs[0], 0, sizeof(struct ocfs2_extent_rec));
 
-		ocfs2_set_last_eb_blk(et, 0);
+		ocfs2_et_set_last_eb_blk(et, 0);
 	}
 
 	ocfs2_journal_dirty(handle, path_root_bh(path));
@@ -3989,8 +3996,8 @@ static int ocfs2_do_insert_extent(struct inode *inode,
 
 out_update_clusters:
 	if (type->ins_split == SPLIT_NONE)
-		ocfs2_update_clusters(inode, et,
-				      le16_to_cpu(insert_rec->e_leaf_clusters));
+		ocfs2_et_update_clusters(inode, et,
+					 le16_to_cpu(insert_rec->e_leaf_clusters));
 
 	ret = ocfs2_journal_dirty(handle, et->root_bh);
 	if (ret)
@@ -4238,7 +4245,7 @@ static int ocfs2_figure_insert_type(struct inode *inode,
 		 * may want it later.
 		 */
 		ret = ocfs2_read_block(OCFS2_SB(inode->i_sb),
-				       ocfs2_get_last_eb_blk(et), &bh,
+				       ocfs2_et_get_last_eb_blk(et), &bh,
 				       OCFS2_BH_CACHED, inode);
 		if (ret) {
 			mlog_exit(ret);
@@ -4315,7 +4322,7 @@ static int ocfs2_figure_insert_type(struct inode *inode,
 	 * the case that we're doing a tail append, so maybe we can
 	 * take advantage of that information somehow.
 	 */
-	if (ocfs2_get_last_eb_blk(et) ==
+	if (ocfs2_et_get_last_eb_blk(et) ==
 	    path_leaf_bh(path)->b_blocknr) {
 		/*
 		 * Ok, ocfs2_find_path() returned us the rightmost
@@ -4823,7 +4830,7 @@ static int __ocfs2_mark_extent_written(struct inode *inode,
 		struct ocfs2_extent_block *eb;
 
 		ret = ocfs2_read_block(OCFS2_SB(inode->i_sb),
-				       ocfs2_get_last_eb_blk(et),
+				       ocfs2_et_get_last_eb_blk(et),
 				       &last_eb_bh, OCFS2_BH_CACHED, inode);
 		if (ret) {
 			mlog_exit(ret);
@@ -4990,7 +4997,7 @@ static int ocfs2_split_tree(struct inode *inode, struct ocfs2_extent_tree *et,
 	depth = path->p_tree_depth;
 	if (depth > 0) {
 		ret = ocfs2_read_block(OCFS2_SB(inode->i_sb),
-				       ocfs2_get_last_eb_blk(et),
+				       ocfs2_et_get_last_eb_blk(et),
 				       &last_eb_bh, OCFS2_BH_CACHED, inode);
 		if (ret < 0) {
 			mlog_errno(ret);
-- 
1.5.6.3

[Ocfs2-devel] [PATCH 01/10] ocfs2: Prefix the extent tree operations structure.

[TaoMa]

Joel Becker wrote:
> The ocfs2_extent_tree_operations structure gains a field prefix on its
> members.  The ->eo_sanity_check() operation gains a wrapper function for
> completeness.  All of the extent tree operation wrappers gain a
> consistent name (ocfs2_et_*()).
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
>   
Signed-off-by: Tao Ma <tao.ma@oracle.com>

[Ocfs2-devel] [PATCH 02/10] ocfs2: Prefix the ocfs2_extent_tree structure.

[Joel Becker]

The members of the ocfs2_extent_tree structure gain a prefix of 'et_'.
All users are updated.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/alloc.c |  118 ++++++++++++++++++++++++++++--------------------------
 1 files changed, 61 insertions(+), 57 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index 9fe49f2..ab16b89 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -75,28 +75,30 @@ struct ocfs2_extent_tree_operations {
 };
 
 struct ocfs2_extent_tree {
-	enum ocfs2_extent_tree_type type;
-	struct ocfs2_extent_tree_operations *eops;
-	struct buffer_head *root_bh;
-	struct ocfs2_extent_list *root_el;
-	void *private;
-	unsigned int max_leaf_clusters;
+	enum ocfs2_extent_tree_type		et_type;
+	struct ocfs2_extent_tree_operations	*et_ops;
+	struct buffer_head			*et_root_bh;
+	struct ocfs2_extent_list		*et_root_el;
+	void					*et_private;
+	unsigned int				et_max_leaf_clusters;
 };
 
 static void ocfs2_dinode_set_last_eb_blk(struct ocfs2_extent_tree *et,
 					 u64 blkno)
 {
-	struct ocfs2_dinode *di = (struct ocfs2_dinode *)et->root_bh->b_data;
+	struct ocfs2_dinode *di =
+		(struct ocfs2_dinode *)et->et_root_bh->b_data;
 
-	BUG_ON(et->type != OCFS2_DINODE_EXTENT);
+	BUG_ON(et->et_type != OCFS2_DINODE_EXTENT);
 	di->i_last_eb_blk = cpu_to_le64(blkno);
 }
 
 static u64 ocfs2_dinode_get_last_eb_blk(struct ocfs2_extent_tree *et)
 {
-	struct ocfs2_dinode *di = (struct ocfs2_dinode *)et->root_bh->b_data;
+	struct ocfs2_dinode *di =
+		(struct ocfs2_dinode *)et->et_root_bh->b_data;
 
-	BUG_ON(et->type != OCFS2_DINODE_EXTENT);
+	BUG_ON(et->et_type != OCFS2_DINODE_EXTENT);
 	return le64_to_cpu(di->i_last_eb_blk);
 }
 
@@ -105,7 +107,7 @@ static void ocfs2_dinode_update_clusters(struct inode *inode,
 					 u32 clusters)
 {
 	struct ocfs2_dinode *di =
-			(struct ocfs2_dinode *)et->root_bh->b_data;
+			(struct ocfs2_dinode *)et->et_root_bh->b_data;
 
 	le32_add_cpu(&di->i_clusters, clusters);
 	spin_lock(&OCFS2_I(inode)->ip_lock);
@@ -119,9 +121,9 @@ static int ocfs2_dinode_sanity_check(struct inode *inode,
 	int ret = 0;
 	struct ocfs2_dinode *di;
 
-	BUG_ON(et->type != OCFS2_DINODE_EXTENT);
+	BUG_ON(et->et_type != OCFS2_DINODE_EXTENT);
 
-	di = (struct ocfs2_dinode *)et->root_bh->b_data;
+	di = (struct ocfs2_dinode *)et->et_root_bh->b_data;
 	if (!OCFS2_IS_VALID_DINODE(di)) {
 		ret = -EIO;
 		ocfs2_error(inode->i_sb,
@@ -143,7 +145,7 @@ static void ocfs2_xattr_value_set_last_eb_blk(struct ocfs2_extent_tree *et,
 					      u64 blkno)
 {
 	struct ocfs2_xattr_value_root *xv =
-		(struct ocfs2_xattr_value_root *)et->private;
+		(struct ocfs2_xattr_value_root *)et->et_private;
 
 	xv->xr_last_eb_blk = cpu_to_le64(blkno);
 }
@@ -151,7 +153,7 @@ static void ocfs2_xattr_value_set_last_eb_blk(struct ocfs2_extent_tree *et,
 static u64 ocfs2_xattr_value_get_last_eb_blk(struct ocfs2_extent_tree *et)
 {
 	struct ocfs2_xattr_value_root *xv =
-		(struct ocfs2_xattr_value_root *) et->private;
+		(struct ocfs2_xattr_value_root *) et->et_private;
 
 	return le64_to_cpu(xv->xr_last_eb_blk);
 }
@@ -161,7 +163,7 @@ static void ocfs2_xattr_value_update_clusters(struct inode *inode,
 					      u32 clusters)
 {
 	struct ocfs2_xattr_value_root *xv =
-		(struct ocfs2_xattr_value_root *)et->private;
+		(struct ocfs2_xattr_value_root *)et->et_private;
 
 	le32_add_cpu(&xv->xr_clusters, clusters);
 }
@@ -183,7 +185,7 @@ static void ocfs2_xattr_tree_set_last_eb_blk(struct ocfs2_extent_tree *et,
 					     u64 blkno)
 {
 	struct ocfs2_xattr_block *xb =
-		(struct ocfs2_xattr_block *) et->root_bh->b_data;
+		(struct ocfs2_xattr_block *) et->et_root_bh->b_data;
 	struct ocfs2_xattr_tree_root *xt = &xb->xb_attrs.xb_root;
 
 	xt->xt_last_eb_blk = cpu_to_le64(blkno);
@@ -192,7 +194,7 @@ static void ocfs2_xattr_tree_set_last_eb_blk(struct ocfs2_extent_tree *et,
 static u64 ocfs2_xattr_tree_get_last_eb_blk(struct ocfs2_extent_tree *et)
 {
 	struct ocfs2_xattr_block *xb =
-		(struct ocfs2_xattr_block *) et->root_bh->b_data;
+		(struct ocfs2_xattr_block *) et->et_root_bh->b_data;
 	struct ocfs2_xattr_tree_root *xt = &xb->xb_attrs.xb_root;
 
 	return le64_to_cpu(xt->xt_last_eb_blk);
@@ -203,7 +205,7 @@ static void ocfs2_xattr_tree_update_clusters(struct inode *inode,
 					     u32 clusters)
 {
 	struct ocfs2_xattr_block *xb =
-			(struct ocfs2_xattr_block *)et->root_bh->b_data;
+			(struct ocfs2_xattr_block *)et->et_root_bh->b_data;
 
 	le32_add_cpu(&xb->xb_attrs.xb_root.xt_clusters, clusters);
 }
@@ -233,25 +235,26 @@ static struct ocfs2_extent_tree*
 	if (!et)
 		return NULL;
 
-	et->type = et_type;
+	et->et_type = et_type;
 	get_bh(bh);
-	et->root_bh = bh;
-	et->private = private;
+	et->et_root_bh = bh;
+	et->et_private = private;
 
 	if (et_type == OCFS2_DINODE_EXTENT) {
-		et->root_el = &((struct ocfs2_dinode *)bh->b_data)->id2.i_list;
-		et->eops = &ocfs2_dinode_et_ops;
+		et->et_root_el =
+			&((struct ocfs2_dinode *)bh->b_data)->id2.i_list;
+		et->et_ops = &ocfs2_dinode_et_ops;
 	} else if (et_type == OCFS2_XATTR_VALUE_EXTENT) {
 		struct ocfs2_xattr_value_root *xv =
 			(struct ocfs2_xattr_value_root *) private;
-		et->root_el = &xv->xr_list;
-		et->eops = &ocfs2_xattr_et_ops;
+		et->et_root_el = &xv->xr_list;
+		et->et_ops = &ocfs2_xattr_et_ops;
 	} else if (et_type == OCFS2_XATTR_TREE_EXTENT) {
 		struct ocfs2_xattr_block *xb =
 			(struct ocfs2_xattr_block *)bh->b_data;
-		et->root_el = &xb->xb_attrs.xb_root.xt_list;
-		et->eops = &ocfs2_xattr_tree_et_ops;
-		et->max_leaf_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
+		et->et_root_el = &xb->xb_attrs.xb_root.xt_list;
+		et->et_ops = &ocfs2_xattr_tree_et_ops;
+		et->et_max_leaf_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
 						OCFS2_MAX_XATTR_TREE_LEAF_SIZE);
 	}
 
@@ -261,7 +264,7 @@ static struct ocfs2_extent_tree*
 static void ocfs2_free_extent_tree(struct ocfs2_extent_tree *et)
 {
 	if (et) {
-		brelse(et->root_bh);
+		brelse(et->et_root_bh);
 		kfree(et);
 	}
 }
@@ -269,25 +272,25 @@ static void ocfs2_free_extent_tree(struct ocfs2_extent_tree *et)
 static inline void ocfs2_et_set_last_eb_blk(struct ocfs2_extent_tree *et,
 					    u64 new_last_eb_blk)
 {
-	et->eops->eo_set_last_eb_blk(et, new_last_eb_blk);
+	et->et_ops->eo_set_last_eb_blk(et, new_last_eb_blk);
 }
 
 static inline u64 ocfs2_et_get_last_eb_blk(struct ocfs2_extent_tree *et)
 {
-	return et->eops->eo_get_last_eb_blk(et);
+	return et->et_ops->eo_get_last_eb_blk(et);
 }
 
 static inline void ocfs2_et_update_clusters(struct inode *inode,
 					    struct ocfs2_extent_tree *et,
 					    u32 clusters)
 {
-	et->eops->eo_update_clusters(inode, et, clusters);
+	et->et_ops->eo_update_clusters(inode, et, clusters);
 }
 
 static inline int ocfs2_et_sanity_check(struct inode *inode,
 					struct ocfs2_extent_tree *et)
 {
-	return et->eops->eo_sanity_check(inode, et);
+	return et->et_ops->eo_sanity_check(inode, et);
 }
 
 static void ocfs2_free_truncate_context(struct ocfs2_truncate_context *tc);
@@ -805,7 +808,7 @@ static int ocfs2_add_branch(struct ocfs2_super *osb,
 		eb = (struct ocfs2_extent_block *) eb_bh->b_data;
 		el = &eb->h_list;
 	} else
-		el = et->root_el;
+		el = et->et_root_el;
 
 	/* we never add a branch to a leaf. */
 	BUG_ON(!el->l_tree_depth);
@@ -895,7 +898,7 @@ static int ocfs2_add_branch(struct ocfs2_super *osb,
 		mlog_errno(status);
 		goto bail;
 	}
-	status = ocfs2_journal_access(handle, inode, et->root_bh,
+	status = ocfs2_journal_access(handle, inode, et->et_root_bh,
 				      OCFS2_JOURNAL_ACCESS_WRITE);
 	if (status < 0) {
 		mlog_errno(status);
@@ -928,7 +931,7 @@ static int ocfs2_add_branch(struct ocfs2_super *osb,
 	status = ocfs2_journal_dirty(handle, *last_eb_bh);
 	if (status < 0)
 		mlog_errno(status);
-	status = ocfs2_journal_dirty(handle, et->root_bh);
+	status = ocfs2_journal_dirty(handle, et->et_root_bh);
 	if (status < 0)
 		mlog_errno(status);
 	if (eb_bh) {
@@ -994,7 +997,7 @@ static int ocfs2_shift_tree_depth(struct ocfs2_super *osb,
 	}
 
 	eb_el = &eb->h_list;
-	root_el = et->root_el;
+	root_el = et->et_root_el;
 
 	status = ocfs2_journal_access(handle, inode, new_eb_bh,
 				      OCFS2_JOURNAL_ACCESS_CREATE);
@@ -1015,7 +1018,7 @@ static int ocfs2_shift_tree_depth(struct ocfs2_super *osb,
 		goto bail;
 	}
 
-	status = ocfs2_journal_access(handle, inode, et->root_bh,
+	status = ocfs2_journal_access(handle, inode, et->et_root_bh,
 				      OCFS2_JOURNAL_ACCESS_WRITE);
 	if (status < 0) {
 		mlog_errno(status);
@@ -1038,7 +1041,7 @@ static int ocfs2_shift_tree_depth(struct ocfs2_super *osb,
 	if (root_el->l_tree_depth == cpu_to_le16(1))
 		ocfs2_et_set_last_eb_blk(et, le64_to_cpu(eb->h_blkno));
 
-	status = ocfs2_journal_dirty(handle, et->root_bh);
+	status = ocfs2_journal_dirty(handle, et->et_root_bh);
 	if (status < 0) {
 		mlog_errno(status);
 		goto bail;
@@ -1088,7 +1091,7 @@ static int ocfs2_find_branch_target(struct ocfs2_super *osb,
 
 	*target_bh = NULL;
 
-	el = et->root_el;
+	el = et->et_root_el;
 
 	while(le16_to_cpu(el->l_tree_depth) > 1) {
 		if (le16_to_cpu(el->l_next_free_rec) == 0) {
@@ -1140,7 +1143,7 @@ static int ocfs2_find_branch_target(struct ocfs2_super *osb,
 
 	/* If we didn't find one and the fe doesn't have any room,
 	 * then return '1' */
-	el = et->root_el;
+	el = et->et_root_el;
 	if (!lowest_bh && (el->l_next_free_rec == el->l_count))
 		status = 1;
 
@@ -1169,7 +1172,7 @@ static int ocfs2_grow_tree(struct inode *inode, handle_t *handle,
 			   struct ocfs2_alloc_context *meta_ac)
 {
 	int ret, shift;
-	struct ocfs2_extent_list *el = et->root_el;
+	struct ocfs2_extent_list *el = et->et_root_el;
 	int depth = le16_to_cpu(el->l_tree_depth);
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct buffer_head *bh = NULL;
@@ -2774,7 +2777,7 @@ static int ocfs2_remove_rightmost_path(struct inode *inode, handle_t *handle,
 		 */
 		ocfs2_unlink_path(inode, handle, dealloc, path, 1);
 
-		el = et->root_el;
+		el = et->et_root_el;
 		el->l_tree_depth = 0;
 		el->l_next_free_rec = 0;
 		memset(&el->l_recs[0], 0, sizeof(struct ocfs2_extent_rec));
@@ -3907,9 +3910,9 @@ static int ocfs2_do_insert_extent(struct inode *inode,
 	struct ocfs2_path *left_path = NULL;
 	struct ocfs2_extent_list *el;
 
-	el = et->root_el;
+	el = et->et_root_el;
 
-	ret = ocfs2_journal_access(handle, inode, et->root_bh,
+	ret = ocfs2_journal_access(handle, inode, et->et_root_bh,
 				   OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
@@ -3921,7 +3924,7 @@ static int ocfs2_do_insert_extent(struct inode *inode,
 		goto out_update_clusters;
 	}
 
-	right_path = ocfs2_new_path(et->root_bh, et->root_el);
+	right_path = ocfs2_new_path(et->et_root_bh, et->et_root_el);
 	if (!right_path) {
 		ret = -ENOMEM;
 		mlog_errno(ret);
@@ -3971,7 +3974,7 @@ static int ocfs2_do_insert_extent(struct inode *inode,
 		 * ocfs2_rotate_tree_right() might have extended the
 		 * transaction without re-journaling our tree root.
 		 */
-		ret = ocfs2_journal_access(handle, inode, et->root_bh,
+		ret = ocfs2_journal_access(handle, inode, et->et_root_bh,
 					   OCFS2_JOURNAL_ACCESS_WRITE);
 		if (ret) {
 			mlog_errno(ret);
@@ -3999,7 +4002,7 @@ out_update_clusters:
 		ocfs2_et_update_clusters(inode, et,
 					 le16_to_cpu(insert_rec->e_leaf_clusters));
 
-	ret = ocfs2_journal_dirty(handle, et->root_bh);
+	ret = ocfs2_journal_dirty(handle, et->et_root_bh);
 	if (ret)
 		mlog_errno(ret);
 
@@ -4157,7 +4160,8 @@ static void ocfs2_figure_contig_type(struct inode *inode,
 		 * Caller might want us to limit the size of extents, don't
 		 * calculate contiguousness if we might exceed that limit.
 		 */
-		if (et->max_leaf_clusters && len > et->max_leaf_clusters)
+		if (et->et_max_leaf_clusters &&
+		    (len > et->et_max_leaf_clusters))
 			insert->ins_contig = CONTIG_NONE;
 	}
 }
@@ -4234,7 +4238,7 @@ static int ocfs2_figure_insert_type(struct inode *inode,
 
 	insert->ins_split = SPLIT_NONE;
 
-	el = et->root_el;
+	el = et->et_root_el;
 	insert->ins_tree_depth = le16_to_cpu(el->l_tree_depth);
 
 	if (el->l_tree_depth) {
@@ -4272,7 +4276,7 @@ static int ocfs2_figure_insert_type(struct inode *inode,
 		return 0;
 	}
 
-	path = ocfs2_new_path(et->root_bh, et->root_el);
+	path = ocfs2_new_path(et->et_root_bh, et->et_root_el);
 	if (!path) {
 		ret = -ENOMEM;
 		mlog_errno(ret);
@@ -4413,7 +4417,7 @@ static int ocfs2_insert_extent(struct ocfs2_super *osb,
 	status = ocfs2_do_insert_extent(inode, handle, et, &rec, &insert);
 	if (status < 0)
 		mlog_errno(status);
-	else if (et->type == OCFS2_DINODE_EXTENT)
+	else if (et->et_type == OCFS2_DINODE_EXTENT)
 		ocfs2_extent_map_insert_rec(inode, &rec);
 
 bail:
@@ -4687,7 +4691,7 @@ leftright:
 	 */
 	rec = path_leaf_el(path)->l_recs[split_index];
 
-	rightmost_el = et->root_el;
+	rightmost_el = et->et_root_el;
 
 	depth = le16_to_cpu(rightmost_el->l_tree_depth);
 	if (depth) {
@@ -4930,7 +4934,7 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
 	if (et_type == OCFS2_DINODE_EXTENT)
 		ocfs2_extent_map_trunc(inode, 0);
 
-	left_path = ocfs2_new_path(et->root_bh, et->root_el);
+	left_path = ocfs2_new_path(et->et_root_bh, et->et_root_el);
 	if (!left_path) {
 		ret = -ENOMEM;
 		mlog_errno(ret);
@@ -5010,7 +5014,7 @@ static int ocfs2_split_tree(struct inode *inode, struct ocfs2_extent_tree *et,
 		rightmost_el = path_leaf_el(path);
 
 	credits += path->p_tree_depth +
-		   ocfs2_extend_meta_needed(et->root_el);
+		   ocfs2_extend_meta_needed(et->et_root_el);
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
 		mlog_errno(ret);
@@ -5223,7 +5227,7 @@ int ocfs2_remove_extent(struct inode *inode, struct buffer_head *root_bh,
 
 	ocfs2_extent_map_trunc(inode, 0);
 
-	path = ocfs2_new_path(et->root_bh, et->root_el);
+	path = ocfs2_new_path(et->et_root_bh, et->et_root_el);
 	if (!path) {
 		ret = -ENOMEM;
 		mlog_errno(ret);
-- 
1.5.6.3

[Ocfs2-devel] [PATCH 02/10] ocfs2: Prefix the ocfs2_extent_tree structure.

[TaoMa]

Joel Becker wrote:
> The members of the ocfs2_extent_tree structure gain a prefix of 'et_'.
> All users are updated.
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
>   
Signed-off-by: Tao Ma <tao.ma@oracle.com>

[Ocfs2-devel] [PATCH 03/10] ocfs2: Make ocfs2_extent_tree get/put instead of alloc.

[Joel Becker]

Rather than allocating a struct ocfs2_extent_tree, just put it on the
stack.  Fill it with ocfs2_get_extent_tree() and drop it with
ocfs2_put_extent_tree().  Now the callers don't have to ENOMEM, yet
still safely ref the root_bh.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/alloc.c |  117 ++++++++++++++++-------------------------------------
 1 files changed, 36 insertions(+), 81 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index ab16b89..0abf11e 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -223,22 +223,17 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = {
 	.eo_sanity_check	= ocfs2_xattr_tree_sanity_check,
 };
 
-static struct ocfs2_extent_tree*
-	 ocfs2_new_extent_tree(struct inode *inode,
-			       struct buffer_head *bh,
-			       enum ocfs2_extent_tree_type et_type,
-			       void *private)
+static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
+				  struct inode *inode,
+				  struct buffer_head *bh,
+				  enum ocfs2_extent_tree_type et_type,
+				  void *private)
 {
-	struct ocfs2_extent_tree *et;
-
-	et = kzalloc(sizeof(*et), GFP_NOFS);
-	if (!et)
-		return NULL;
-
 	et->et_type = et_type;
 	get_bh(bh);
 	et->et_root_bh = bh;
 	et->et_private = private;
+	et->et_max_leaf_clusters = 0;
 
 	if (et_type == OCFS2_DINODE_EXTENT) {
 		et->et_root_el =
@@ -257,16 +252,11 @@ static struct ocfs2_extent_tree*
 		et->et_max_leaf_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
 						OCFS2_MAX_XATTR_TREE_LEAF_SIZE);
 	}
-
-	return et;
 }
 
-static void ocfs2_free_extent_tree(struct ocfs2_extent_tree *et)
+static void ocfs2_put_extent_tree(struct ocfs2_extent_tree *et)
 {
-	if (et) {
-		brelse(et->et_root_bh);
-		kfree(et);
-	}
+	brelse(et->et_root_bh);
 }
 
 static inline void ocfs2_et_set_last_eb_blk(struct ocfs2_extent_tree *et,
@@ -4439,22 +4429,15 @@ int ocfs2_dinode_insert_extent(struct ocfs2_super *osb,
 			       struct ocfs2_alloc_context *meta_ac)
 {
 	int status;
-	struct ocfs2_extent_tree *et = NULL;
-
-	et = ocfs2_new_extent_tree(inode, root_bh, OCFS2_DINODE_EXTENT, NULL);
-	if (!et) {
-		status = -ENOMEM;
-		mlog_errno(status);
-		goto bail;
-	}
+	struct ocfs2_extent_tree et;
 
+	ocfs2_get_extent_tree(&et, inode, root_bh, OCFS2_DINODE_EXTENT,
+			      NULL);
 	status = ocfs2_insert_extent(osb, handle, inode, root_bh,
 				     cpos, start_blk, new_clusters,
-				     flags, meta_ac, et);
+				     flags, meta_ac, &et);
+	ocfs2_put_extent_tree(&et);
 
-	if (et)
-		ocfs2_free_extent_tree(et);
-bail:
 	return status;
 }
 
@@ -4470,23 +4453,15 @@ int ocfs2_xattr_value_insert_extent(struct ocfs2_super *osb,
 				    void *private)
 {
 	int status;
-	struct ocfs2_extent_tree *et = NULL;
-
-	et = ocfs2_new_extent_tree(inode, root_bh,
-				   OCFS2_XATTR_VALUE_EXTENT, private);
-	if (!et) {
-		status = -ENOMEM;
-		mlog_errno(status);
-		goto bail;
-	}
+	struct ocfs2_extent_tree et;
 
+	ocfs2_get_extent_tree(&et, inode, root_bh,
+			      OCFS2_XATTR_VALUE_EXTENT, private);
 	status = ocfs2_insert_extent(osb, handle, inode, root_bh,
 				     cpos, start_blk, new_clusters,
-				     flags, meta_ac, et);
+				     flags, meta_ac, &et);
+	ocfs2_put_extent_tree(&et);
 
-	if (et)
-		ocfs2_free_extent_tree(et);
-bail:
 	return status;
 }
 
@@ -4501,23 +4476,15 @@ int ocfs2_xattr_tree_insert_extent(struct ocfs2_super *osb,
 				   struct ocfs2_alloc_context *meta_ac)
 {
 	int status;
-	struct ocfs2_extent_tree *et = NULL;
-
-	et = ocfs2_new_extent_tree(inode, root_bh, OCFS2_XATTR_TREE_EXTENT,
-				   NULL);
-	if (!et) {
-		status = -ENOMEM;
-		mlog_errno(status);
-		goto bail;
-	}
+	struct ocfs2_extent_tree et;
 
+	ocfs2_get_extent_tree(&et, inode, root_bh, OCFS2_XATTR_TREE_EXTENT,
+			      NULL);
 	status = ocfs2_insert_extent(osb, handle, inode, root_bh,
 				     cpos, start_blk, new_clusters,
-				     flags, meta_ac, et);
+				     flags, meta_ac, &et);
+	ocfs2_put_extent_tree(&et);
 
-	if (et)
-		ocfs2_free_extent_tree(et);
-bail:
 	return status;
 }
 
@@ -4906,11 +4873,13 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
 	struct ocfs2_extent_rec split_rec;
 	struct ocfs2_path *left_path = NULL;
 	struct ocfs2_extent_list *el;
-	struct ocfs2_extent_tree *et = NULL;
+	struct ocfs2_extent_tree et;
 
 	mlog(0, "Inode %lu cpos %u, len %u, phys %u (%llu)\n",
 	     inode->i_ino, cpos, len, phys, (unsigned long long)start_blkno);
 
+	ocfs2_get_extent_tree(&et, inode, root_bh, et_type, private);
+
 	if (!ocfs2_writes_unwritten_extents(OCFS2_SB(inode->i_sb))) {
 		ocfs2_error(inode->i_sb, "Inode %llu has unwritten extents "
 			    "that are being written to, but the feature bit "
@@ -4920,13 +4889,6 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
 		goto out;
 	}
 
-	et = ocfs2_new_extent_tree(inode, root_bh, et_type, private);
-	if (!et) {
-		ret = -ENOMEM;
-		mlog_errno(ret);
-		goto out;
-	}
-
 	/*
 	 * XXX: This should be fixed up so that we just re-insert the
 	 * next extent records.
@@ -4934,7 +4896,7 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
 	if (et_type == OCFS2_DINODE_EXTENT)
 		ocfs2_extent_map_trunc(inode, 0);
 
-	left_path = ocfs2_new_path(et->et_root_bh, et->et_root_el);
+	left_path = ocfs2_new_path(et.et_root_bh, et.et_root_el);
 	if (!left_path) {
 		ret = -ENOMEM;
 		mlog_errno(ret);
@@ -4965,7 +4927,7 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
 	split_rec.e_flags = path_leaf_el(left_path)->l_recs[index].e_flags;
 	split_rec.e_flags &= ~OCFS2_EXT_UNWRITTEN;
 
-	ret = __ocfs2_mark_extent_written(inode, et, handle, left_path,
+	ret = __ocfs2_mark_extent_written(inode, &et, handle, left_path,
 					  index, &split_rec, meta_ac,
 					  dealloc);
 	if (ret)
@@ -4973,8 +4935,7 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
 
 out:
 	ocfs2_free_path(left_path);
-	if (et)
-		ocfs2_free_extent_tree(et);
+	ocfs2_put_extent_tree(&et);
 	return ret;
 }
 
@@ -5216,18 +5177,13 @@ int ocfs2_remove_extent(struct inode *inode, struct buffer_head *root_bh,
 	struct ocfs2_extent_rec *rec;
 	struct ocfs2_extent_list *el;
 	struct ocfs2_path *path = NULL;
-	struct ocfs2_extent_tree *et = NULL;
+	struct ocfs2_extent_tree et;
 
-	et = ocfs2_new_extent_tree(inode, root_bh, et_type, private);
-	if (!et) {
-		ret = -ENOMEM;
-		mlog_errno(ret);
-		goto out;
-	}
+	ocfs2_get_extent_tree(&et, inode, root_bh, et_type, private);
 
 	ocfs2_extent_map_trunc(inode, 0);
 
-	path = ocfs2_new_path(et->et_root_bh, et->et_root_el);
+	path = ocfs2_new_path(et.et_root_bh, et.et_root_el);
 	if (!path) {
 		ret = -ENOMEM;
 		mlog_errno(ret);
@@ -5280,13 +5236,13 @@ int ocfs2_remove_extent(struct inode *inode, struct buffer_head *root_bh,
 
 	if (le32_to_cpu(rec->e_cpos) == cpos || rec_range == trunc_range) {
 		ret = ocfs2_truncate_rec(inode, handle, path, index, dealloc,
-					 cpos, len, et);
+					 cpos, len, &et);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
 		}
 	} else {
-		ret = ocfs2_split_tree(inode, et, handle, path, index,
+		ret = ocfs2_split_tree(inode, &et, handle, path, index,
 				       trunc_range, meta_ac);
 		if (ret) {
 			mlog_errno(ret);
@@ -5335,7 +5291,7 @@ int ocfs2_remove_extent(struct inode *inode, struct buffer_head *root_bh,
 		}
 
 		ret = ocfs2_truncate_rec(inode, handle, path, index, dealloc,
-					 cpos, len, et);
+					 cpos, len, &et);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
@@ -5344,8 +5300,7 @@ int ocfs2_remove_extent(struct inode *inode, struct buffer_head *root_bh,
 
 out:
 	ocfs2_free_path(path);
-	if (et)
-		ocfs2_free_extent_tree(et);
+	ocfs2_put_extent_tree(&et);
 	return ret;
 }
 
-- 
1.5.6.3

[Ocfs2-devel] [PATCH 03/10] ocfs2: Make ocfs2_extent_tree get/put instead of alloc.

[TaoMa]

Joel Becker wrote:
> Rather than allocating a struct ocfs2_extent_tree, just put it on the
> stack.  Fill it with ocfs2_get_extent_tree() and drop it with
> ocfs2_put_extent_tree().  Now the callers don't have to ENOMEM, yet
> still safely ref the root_bh.
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
>  fs/ocfs2/alloc.c |  117 ++++++++++++++++-------------------------------------
>  1 files changed, 36 insertions(+), 81 deletions(-)
>
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index ab16b89..0abf11e 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -223,22 +223,17 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = {
>  	.eo_sanity_check	= ocfs2_xattr_tree_sanity_check,
>  };
>  
>
>  
> -static void ocfs2_free_extent_tree(struct ocfs2_extent_tree *et)
> +static void ocfs2_put_extent_tree(struct ocfs2_extent_tree *et)
>  {
> -	if (et) {
> -		brelse(et->et_root_bh);
> -		kfree(et);
> -	}
> +	brelse(et->et_root_bh);
>  }
>   
Can we inline this since it is just a one-line function?
>  
>  static inline void ocfs2_et_set_last_eb_blk(struct ocfs2_extent_tree *et,
> @@ -4439,22 +4429,15 @@ int ocfs2_dinode_insert_extent(struct ocfs2_super *osb,
>  			       struct ocfs2_alloc_context *meta_ac)
>  {
>  	int status;
> -	struct ocfs2_extent_tree *et = NULL;
> -
> -	et = ocfs2_new_extent_tree(inode, root_bh, OCFS2_DINODE_EXTENT, NULL);
> -	if (!et) {
> -		status = -ENOMEM;
> -		mlog_errno(status);
> -		goto bail;
> -	}
> +	struct ocfs2_extent_tree et;
>  
> +	ocfs2_get_extent_tree(&et, inode, root_bh, OCFS2_DINODE_EXTENT,
> +			      NULL);
>  	status = ocfs2_insert_extent(osb, handle, inode, root_bh,
>  				     cpos, start_blk, new_clusters,
> -				     flags, meta_ac, et);
> +				     flags, meta_ac, &et);
> +	ocfs2_put_extent_tree(&et);
>  
> -	if (et)
> -		ocfs2_free_extent_tree(et);
> -bail:
>  	return status;
>  }
>  
> @@ -4470,23 +4453,15 @@ int ocfs2_xattr_value_insert_extent(struct ocfs2_super *osb,
>  				    void *private)
>  {
>  	int status;
> -	struct ocfs2_extent_tree *et = NULL;
> -
> -	et = ocfs2_new_extent_tree(inode, root_bh,
> -				   OCFS2_XATTR_VALUE_EXTENT, private);
> -	if (!et) {
> -		status = -ENOMEM;
> -		mlog_errno(status);
> -		goto bail;
> -	}
> +	struct ocfs2_extent_tree et;
>  
> +	ocfs2_get_extent_tree(&et, inode, root_bh,
> +			      OCFS2_XATTR_VALUE_EXTENT, private);
>  	status = ocfs2_insert_extent(osb, handle, inode, root_bh,
>  				     cpos, start_blk, new_clusters,
> -				     flags, meta_ac, et);
> +				     flags, meta_ac, &et);
> +	ocfs2_put_extent_tree(&et);
>  
> -	if (et)
> -		ocfs2_free_extent_tree(et);
> -bail:
>  	return status;
>  }
>  
> @@ -4501,23 +4476,15 @@ int ocfs2_xattr_tree_insert_extent(struct ocfs2_super *osb,
>  				   struct ocfs2_alloc_context *meta_ac)
>  {
>  	int status;
> -	struct ocfs2_extent_tree *et = NULL;
> -
> -	et = ocfs2_new_extent_tree(inode, root_bh, OCFS2_XATTR_TREE_EXTENT,
> -				   NULL);
> -	if (!et) {
> -		status = -ENOMEM;
> -		mlog_errno(status);
> -		goto bail;
> -	}
> +	struct ocfs2_extent_tree et;
>  
> +	ocfs2_get_extent_tree(&et, inode, root_bh, OCFS2_XATTR_TREE_EXTENT,
> +			      NULL);
>  	status = ocfs2_insert_extent(osb, handle, inode, root_bh,
>  				     cpos, start_blk, new_clusters,
> -				     flags, meta_ac, et);
> +				     flags, meta_ac, &et);
> +	ocfs2_put_extent_tree(&et);
>  
> -	if (et)
> -		ocfs2_free_extent_tree(et);
> -bail:
>  	return status;
>  }
>  
> @@ -4906,11 +4873,13 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
>  	struct ocfs2_extent_rec split_rec;
>  	struct ocfs2_path *left_path = NULL;
>  	struct ocfs2_extent_list *el;
> -	struct ocfs2_extent_tree *et = NULL;
> +	struct ocfs2_extent_tree et;
>  
>  	mlog(0, "Inode %lu cpos %u, len %u, phys %u (%llu)\n",
>  	     inode->i_ino, cpos, len, phys, (unsigned long long)start_blkno);
>  
> +	ocfs2_get_extent_tree(&et, inode, root_bh, et_type, private);
> +
>  	if (!ocfs2_writes_unwritten_extents(OCFS2_SB(inode->i_sb))) {
>  		ocfs2_error(inode->i_sb, "Inode %llu has unwritten extents "
>  			    "that are being written to, but the feature bit "
> @@ -4920,13 +4889,6 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
>  		goto out;
>  	}
>  
> -	et = ocfs2_new_extent_tree(inode, root_bh, et_type, private);
> -	if (!et) {
> -		ret = -ENOMEM;
> -		mlog_errno(ret);
> -		goto out;
> -	}
> -
>   
I think you can get the extent tree here, since if the above judgement 
returns error, we can go out directly without initializing the extent tree.

Regards,
Tao

[Ocfs2-devel] [PATCH 03/10] ocfs2: Make ocfs2_extent_tree get/put instead of alloc.

[Joel Becker]

On Thu, Aug 21, 2008 at 11:55:08AM +0800, TaoMa wrote:
> Joel Becker wrote:
>> Rather than allocating a struct ocfs2_extent_tree, just put it on the
>> stack.  Fill it with ocfs2_get_extent_tree() and drop it with
>> ocfs2_put_extent_tree().  Now the callers don't have to ENOMEM, yet
>> still safely ref the root_bh.
>>
>> Signed-off-by: Joel Becker <joel.becker@oracle.com>

>> --- a/fs/ocfs2/alloc.c
>> +++ b/fs/ocfs2/alloc.c
>> @@ -223,22 +223,17 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = {
>>  	.eo_sanity_check	= ocfs2_xattr_tree_sanity_check,
>>  };
>>  
>>
>>  -static void ocfs2_free_extent_tree(struct ocfs2_extent_tree *et)
>> +static void ocfs2_put_extent_tree(struct ocfs2_extent_tree *et)
>>  {
>> -	if (et) {
>> -		brelse(et->et_root_bh);
>> -		kfree(et);
>> -	}
>> +	brelse(et->et_root_bh);
>>  }
>>   
> Can we inline this since it is just a one-line function?

	No, because it's going to be an exported function in the last
patch.  Plus, inlining is kind of pointless - we're not on any fastpaths
here.  Inlining isn't always necessary, and shouldn't be the default for
all tiny functions.  For something like this, gcc might even figure it
out.

>>  @@ -4906,11 +4873,13 @@ int ocfs2_mark_extent_written(struct inode 
>> *inode, struct buffer_head *root_bh,
>>  	struct ocfs2_extent_rec split_rec;
>>  	struct ocfs2_path *left_path = NULL;
>>  	struct ocfs2_extent_list *el;
>> -	struct ocfs2_extent_tree *et = NULL;
>> +	struct ocfs2_extent_tree et;
>>   	mlog(0, "Inode %lu cpos %u, len %u, phys %u (%llu)\n",
>>  	     inode->i_ino, cpos, len, phys, (unsigned long long)start_blkno);
>>  +	ocfs2_get_extent_tree(&et, inode, root_bh, et_type, private);
>> +
>>  	if (!ocfs2_writes_unwritten_extents(OCFS2_SB(inode->i_sb))) {
>>  		ocfs2_error(inode->i_sb, "Inode %llu has unwritten extents "
>>  			    "that are being written to, but the feature bit "
>> @@ -4920,13 +4889,6 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
>>  		goto out;
>>  	}
>>  -	et = ocfs2_new_extent_tree(inode, root_bh, et_type, private);
>> -	if (!et) {
>> -		ret = -ENOMEM;
>> -		mlog_errno(ret);
>> -		goto out;
>> -	}
>> -
>>   
> I think you can get the extent tree here, since if the above judgement  
> returns error, we can go out directly without initializing the extent 
> tree.

	I do it up top because I unconditionally put it at the bottom.
This is a brelse(), which needs the matching get_bh().  Since we're
munging a stack variable, it doesn't hurt us to do the get_extent_tree()
at the top.

Joel

-- 

"Born under a bad sign.
 I been down since I began to crawl.
 If it wasn't for bad luck,
 I wouldn't have no luck at all."

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

[Ocfs2-devel] [PATCH 03/10] ocfs2: Make ocfs2_extent_tree get/put instead of alloc.

[Mark Fasheh]

On Wed, Aug 20, 2008 at 07:48:18PM -0700, Joel Becker wrote:
> Rather than allocating a struct ocfs2_extent_tree, just put it on the
> stack.  Fill it with ocfs2_get_extent_tree() and drop it with
> ocfs2_put_extent_tree().  Now the callers don't have to ENOMEM, yet
> still safely ref the root_bh.
> 
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
>  fs/ocfs2/alloc.c |  117 ++++++++++++++++-------------------------------------
>  1 files changed, 36 insertions(+), 81 deletions(-)
> 
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index ab16b89..0abf11e 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -223,22 +223,17 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = {
>  	.eo_sanity_check	= ocfs2_xattr_tree_sanity_check,
>  };
>  
> -static struct ocfs2_extent_tree*
> -	 ocfs2_new_extent_tree(struct inode *inode,
> -			       struct buffer_head *bh,
> -			       enum ocfs2_extent_tree_type et_type,
> -			       void *private)
> +static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
> +				  struct inode *inode,
> +				  struct buffer_head *bh,
> +				  enum ocfs2_extent_tree_type et_type,
> +				  void *private)

Actually, maybe "ocfs2_init_extent_tree()" would be more straightforward.
I'm fine with this too though.


>  {
> -	struct ocfs2_extent_tree *et;
> -
> -	et = kzalloc(sizeof(*et), GFP_NOFS);
> -	if (!et)
> -		return NULL;
> -

>  	et->et_type = et_type;
>  	get_bh(bh);
>  	et->et_root_bh = bh;
>  	et->et_private = private;
> +	et->et_max_leaf_clusters = 0;
>  
>  	if (et_type == OCFS2_DINODE_EXTENT) {
>  		et->et_root_el =
> @@ -257,16 +252,11 @@ static struct ocfs2_extent_tree*
>  		et->et_max_leaf_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
>  						OCFS2_MAX_XATTR_TREE_LEAF_SIZE);
>  	}

Can you add a check, down here probably, that we got an 'et_type' value
passed in which is valid? If not, then we might continue with on-the-stack
versions of et_root_el, etc. This won't fail as nice as the kzalloc would
have before.

Btw, I'm going by the source I have in my tree, where there's no default
action other than allowing them to be null via kzalloc.
	--Mark

--
Mark Fasheh

[Ocfs2-devel] [PATCH 03/10] ocfs2: Make ocfs2_extent_tree get/put instead of alloc.

[Joel Becker]

On Thu, Aug 21, 2008 at 03:10:59PM -0700, Mark Fasheh wrote:
> On Wed, Aug 20, 2008 at 07:48:18PM -0700, Joel Becker wrote:
> > Rather than allocating a struct ocfs2_extent_tree, just put it on the
> > stack.  Fill it with ocfs2_get_extent_tree() and drop it with
> > ocfs2_put_extent_tree().  Now the callers don't have to ENOMEM, yet
> > still safely ref the root_bh.
> > 
> > Signed-off-by: Joel Becker <joel.becker@oracle.com>
> > ---
> >  fs/ocfs2/alloc.c |  117 ++++++++++++++++-------------------------------------
> >  1 files changed, 36 insertions(+), 81 deletions(-)
> > 
> > diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> > index ab16b89..0abf11e 100644
> > --- a/fs/ocfs2/alloc.c
> > +++ b/fs/ocfs2/alloc.c
> > @@ -223,22 +223,17 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = {
> >  	.eo_sanity_check	= ocfs2_xattr_tree_sanity_check,
> >  };
> >  
> > -static struct ocfs2_extent_tree*
> > -	 ocfs2_new_extent_tree(struct inode *inode,
> > -			       struct buffer_head *bh,
> > -			       enum ocfs2_extent_tree_type et_type,
> > -			       void *private)
> > +static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
> > +				  struct inode *inode,
> > +				  struct buffer_head *bh,
> > +				  enum ocfs2_extent_tree_type et_type,
> > +				  void *private)
> 
> Actually, maybe "ocfs2_init_extent_tree()" would be more straightforward.
> I'm fine with this too though.

	I originally had "fill", but I chose "get" because we get_bh()
and thus require a "put" to brelse().  I'm not tied to it, so if you
like "init" I'll change it.

> > @@ -257,16 +252,11 @@ static struct ocfs2_extent_tree*
> >  		et->et_max_leaf_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
> >  						OCFS2_MAX_XATTR_TREE_LEAF_SIZE);
> >  	}
> 
> Can you add a check, down here probably, that we got an 'et_type' value
> passed in which is valid? If not, then we might continue with on-the-stack
> versions of et_root_el, etc. This won't fail as nice as the kzalloc would
> have before.

	The final patch removes the et_type completely.  If you want, I
can have the intermediate patches do stricter et_type checking (I think
a later one BUG()s).

Joel

-- 

"When choosing between two evils, I always like to try the one
 I've never tried before."
        - Mae West

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

[Ocfs2-devel] [PATCH 03/10] ocfs2: Make ocfs2_extent_tree get/put instead of alloc.

[Mark Fasheh]

On Thu, Aug 21, 2008 at 04:05:33PM -0700, Joel Becker wrote:
> On Thu, Aug 21, 2008 at 03:10:59PM -0700, Mark Fasheh wrote:
> > On Wed, Aug 20, 2008 at 07:48:18PM -0700, Joel Becker wrote:
> > > Rather than allocating a struct ocfs2_extent_tree, just put it on the
> > > stack.  Fill it with ocfs2_get_extent_tree() and drop it with
> > > ocfs2_put_extent_tree().  Now the callers don't have to ENOMEM, yet
> > > still safely ref the root_bh.
> > > 
> > > Signed-off-by: Joel Becker <joel.becker@oracle.com>
> > > ---
> > >  fs/ocfs2/alloc.c |  117 ++++++++++++++++-------------------------------------
> > >  1 files changed, 36 insertions(+), 81 deletions(-)
> > > 
> > > diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> > > index ab16b89..0abf11e 100644
> > > --- a/fs/ocfs2/alloc.c
> > > +++ b/fs/ocfs2/alloc.c
> > > @@ -223,22 +223,17 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = {
> > >  	.eo_sanity_check	= ocfs2_xattr_tree_sanity_check,
> > >  };
> > >  
> > > -static struct ocfs2_extent_tree*
> > > -	 ocfs2_new_extent_tree(struct inode *inode,
> > > -			       struct buffer_head *bh,
> > > -			       enum ocfs2_extent_tree_type et_type,
> > > -			       void *private)
> > > +static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
> > > +				  struct inode *inode,
> > > +				  struct buffer_head *bh,
> > > +				  enum ocfs2_extent_tree_type et_type,
> > > +				  void *private)
> > 
> > Actually, maybe "ocfs2_init_extent_tree()" would be more straightforward.
> > I'm fine with this too though.
> 
> 	I originally had "fill", but I chose "get" because we get_bh()
> and thus require a "put" to brelse().  I'm not tied to it, so if you
> like "init" I'll change it.

Nah, this is ok.


> > > @@ -257,16 +252,11 @@ static struct ocfs2_extent_tree*
> > >  		et->et_max_leaf_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
> > >  						OCFS2_MAX_XATTR_TREE_LEAF_SIZE);
> > >  	}
> > 
> > Can you add a check, down here probably, that we got an 'et_type' value
> > passed in which is valid? If not, then we might continue with on-the-stack
> > versions of et_root_el, etc. This won't fail as nice as the kzalloc would
> > have before.
> 
> 	The final patch removes the et_type completely.  If you want, I
> can have the intermediate patches do stricter et_type checking (I think
> a later one BUG()s).

Yep, just caught that one. We don't have to be super strict in the early
patches so long as it gets caught by the end of the series.
	--Mark

--
Mark Fasheh

[Ocfs2-devel] [PATCH 03/10] ocfs2: Make ocfs2_extent_tree get/put instead of alloc.

[Joel Becker]

On Thu, Aug 21, 2008 at 04:11:00PM -0700, Mark Fasheh wrote:
> On Thu, Aug 21, 2008 at 04:05:33PM -0700, Joel Becker wrote:
> > > Can you add a check, down here probably, that we got an 'et_type' value
> > > passed in which is valid? If not, then we might continue with on-the-stack
> > > versions of et_root_el, etc. This won't fail as nice as the kzalloc would
> > > have before.
> > 
> > 	The final patch removes the et_type completely.  If you want, I
> > can have the intermediate patches do stricter et_type checking (I think
> > a later one BUG()s).
> 
> Yep, just caught that one. We don't have to be super strict in the early
> patches so long as it gets caught by the end of the series.

	You know, if we move eo_remove_check() back to
eo_sanity_check(), we could do those at the end of __get_extent_tree().
Add in IS_XATTR_VALUE_ROOT() etc, and it's a clean place to check.  It
probably doesn't catch disk corruption (Most of these bhs are already
pre-checked), but it will catch people passing the wrong kind of bh.

Joel

-- 

"The cynics are right nine times out of ten."  
        - H. L. Mencken

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

[Ocfs2-devel] [PATCH 04/10] ocfs2: Make 'private' into 'object' on ocfs2_extent_tree.

[Joel Becker]

The 'private' pointer was a way to store off xattr values, which don't
live at a set place in the bh.  But the concept of "the object
containing the extent tree" is much more generic.  For an inode it's the
struct ocfs2_dinode, for an xattr value its the value.  Let's save off
the 'object' at all times.  If NULL is passed to
ocfs2_get_extent_tree(), 'object' is set to bh->b_data;

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/alloc.c |   62 +++++++++++++++++++++++++----------------------------
 1 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index 0abf11e..93f44f4 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -79,15 +79,14 @@ struct ocfs2_extent_tree {
 	struct ocfs2_extent_tree_operations	*et_ops;
 	struct buffer_head			*et_root_bh;
 	struct ocfs2_extent_list		*et_root_el;
-	void					*et_private;
+	void					*et_object;
 	unsigned int				et_max_leaf_clusters;
 };
 
 static void ocfs2_dinode_set_last_eb_blk(struct ocfs2_extent_tree *et,
 					 u64 blkno)
 {
-	struct ocfs2_dinode *di =
-		(struct ocfs2_dinode *)et->et_root_bh->b_data;
+	struct ocfs2_dinode *di = et->et_object;
 
 	BUG_ON(et->et_type != OCFS2_DINODE_EXTENT);
 	di->i_last_eb_blk = cpu_to_le64(blkno);
@@ -95,8 +94,7 @@ static void ocfs2_dinode_set_last_eb_blk(struct ocfs2_extent_tree *et,
 
 static u64 ocfs2_dinode_get_last_eb_blk(struct ocfs2_extent_tree *et)
 {
-	struct ocfs2_dinode *di =
-		(struct ocfs2_dinode *)et->et_root_bh->b_data;
+	struct ocfs2_dinode *di = et->et_object;
 
 	BUG_ON(et->et_type != OCFS2_DINODE_EXTENT);
 	return le64_to_cpu(di->i_last_eb_blk);
@@ -106,8 +104,7 @@ static void ocfs2_dinode_update_clusters(struct inode *inode,
 					 struct ocfs2_extent_tree *et,
 					 u32 clusters)
 {
-	struct ocfs2_dinode *di =
-			(struct ocfs2_dinode *)et->et_root_bh->b_data;
+	struct ocfs2_dinode *di = et->et_object;
 
 	le32_add_cpu(&di->i_clusters, clusters);
 	spin_lock(&OCFS2_I(inode)->ip_lock);
@@ -123,7 +120,7 @@ static int ocfs2_dinode_sanity_check(struct inode *inode,
 
 	BUG_ON(et->et_type != OCFS2_DINODE_EXTENT);
 
-	di = (struct ocfs2_dinode *)et->et_root_bh->b_data;
+	di = et->et_object;
 	if (!OCFS2_IS_VALID_DINODE(di)) {
 		ret = -EIO;
 		ocfs2_error(inode->i_sb,
@@ -145,7 +142,7 @@ static void ocfs2_xattr_value_set_last_eb_blk(struct ocfs2_extent_tree *et,
 					      u64 blkno)
 {
 	struct ocfs2_xattr_value_root *xv =
-		(struct ocfs2_xattr_value_root *)et->et_private;
+		(struct ocfs2_xattr_value_root *)et->et_object;
 
 	xv->xr_last_eb_blk = cpu_to_le64(blkno);
 }
@@ -153,7 +150,7 @@ static void ocfs2_xattr_value_set_last_eb_blk(struct ocfs2_extent_tree *et,
 static u64 ocfs2_xattr_value_get_last_eb_blk(struct ocfs2_extent_tree *et)
 {
 	struct ocfs2_xattr_value_root *xv =
-		(struct ocfs2_xattr_value_root *) et->et_private;
+		(struct ocfs2_xattr_value_root *) et->et_object;
 
 	return le64_to_cpu(xv->xr_last_eb_blk);
 }
@@ -163,7 +160,7 @@ static void ocfs2_xattr_value_update_clusters(struct inode *inode,
 					      u32 clusters)
 {
 	struct ocfs2_xattr_value_root *xv =
-		(struct ocfs2_xattr_value_root *)et->et_private;
+		(struct ocfs2_xattr_value_root *)et->et_object;
 
 	le32_add_cpu(&xv->xr_clusters, clusters);
 }
@@ -184,8 +181,7 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_et_ops = {
 static void ocfs2_xattr_tree_set_last_eb_blk(struct ocfs2_extent_tree *et,
 					     u64 blkno)
 {
-	struct ocfs2_xattr_block *xb =
-		(struct ocfs2_xattr_block *) et->et_root_bh->b_data;
+	struct ocfs2_xattr_block *xb = et->et_object;
 	struct ocfs2_xattr_tree_root *xt = &xb->xb_attrs.xb_root;
 
 	xt->xt_last_eb_blk = cpu_to_le64(blkno);
@@ -193,8 +189,7 @@ static void ocfs2_xattr_tree_set_last_eb_blk(struct ocfs2_extent_tree *et,
 
 static u64 ocfs2_xattr_tree_get_last_eb_blk(struct ocfs2_extent_tree *et)
 {
-	struct ocfs2_xattr_block *xb =
-		(struct ocfs2_xattr_block *) et->et_root_bh->b_data;
+	struct ocfs2_xattr_block *xb = et->et_object;
 	struct ocfs2_xattr_tree_root *xt = &xb->xb_attrs.xb_root;
 
 	return le64_to_cpu(xt->xt_last_eb_blk);
@@ -204,8 +199,7 @@ static void ocfs2_xattr_tree_update_clusters(struct inode *inode,
 					     struct ocfs2_extent_tree *et,
 					     u32 clusters)
 {
-	struct ocfs2_xattr_block *xb =
-			(struct ocfs2_xattr_block *)et->et_root_bh->b_data;
+	struct ocfs2_xattr_block *xb = et->et_object;
 
 	le32_add_cpu(&xb->xb_attrs.xb_root.xt_clusters, clusters);
 }
@@ -227,26 +221,28 @@ static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
 				  struct inode *inode,
 				  struct buffer_head *bh,
 				  enum ocfs2_extent_tree_type et_type,
-				  void *private)
+				  void *obj)
 {
 	et->et_type = et_type;
 	get_bh(bh);
 	et->et_root_bh = bh;
-	et->et_private = private;
 	et->et_max_leaf_clusters = 0;
+	if (!obj)
+		obj = (void *)bh->b_data;
+	et->et_object = obj;
 
 	if (et_type == OCFS2_DINODE_EXTENT) {
 		et->et_root_el =
-			&((struct ocfs2_dinode *)bh->b_data)->id2.i_list;
+			&((struct ocfs2_dinode *)obj)->id2.i_list;
 		et->et_ops = &ocfs2_dinode_et_ops;
 	} else if (et_type == OCFS2_XATTR_VALUE_EXTENT) {
 		struct ocfs2_xattr_value_root *xv =
-			(struct ocfs2_xattr_value_root *) private;
+			(struct ocfs2_xattr_value_root *)obj;
 		et->et_root_el = &xv->xr_list;
 		et->et_ops = &ocfs2_xattr_et_ops;
 	} else if (et_type == OCFS2_XATTR_TREE_EXTENT) {
 		struct ocfs2_xattr_block *xb =
-			(struct ocfs2_xattr_block *)bh->b_data;
+			(struct ocfs2_xattr_block *)obj;
 		et->et_root_el = &xb->xb_attrs.xb_root.xt_list;
 		et->et_ops = &ocfs2_xattr_tree_et_ops;
 		et->et_max_leaf_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
@@ -593,7 +589,7 @@ int ocfs2_num_free_extents(struct ocfs2_super *osb,
 			   struct inode *inode,
 			   struct buffer_head *root_bh,
 			   enum ocfs2_extent_tree_type type,
-			   void *private)
+			   void *obj)
 {
 	int retval;
 	struct ocfs2_extent_list *el = NULL;
@@ -617,7 +613,7 @@ int ocfs2_num_free_extents(struct ocfs2_super *osb,
 		el = &fe->id2.i_list;
 	} else if (type == OCFS2_XATTR_VALUE_EXTENT) {
 		struct ocfs2_xattr_value_root *xv =
-			(struct ocfs2_xattr_value_root *) private;
+			(struct ocfs2_xattr_value_root *) obj;
 
 		last_eb_blk = le64_to_cpu(xv->xr_last_eb_blk);
 		el = &xv->xr_list;
@@ -4450,13 +4446,13 @@ int ocfs2_xattr_value_insert_extent(struct ocfs2_super *osb,
 				    u32 new_clusters,
 				    u8 flags,
 				    struct ocfs2_alloc_context *meta_ac,
-				    void *private)
+				    void *obj)
 {
 	int status;
 	struct ocfs2_extent_tree et;
 
 	ocfs2_get_extent_tree(&et, inode, root_bh,
-			      OCFS2_XATTR_VALUE_EXTENT, private);
+			      OCFS2_XATTR_VALUE_EXTENT, obj);
 	status = ocfs2_insert_extent(osb, handle, inode, root_bh,
 				     cpos, start_blk, new_clusters,
 				     flags, meta_ac, &et);
@@ -4507,7 +4503,7 @@ int ocfs2_add_clusters_in_btree(struct ocfs2_super *osb,
 				struct ocfs2_alloc_context *meta_ac,
 				enum ocfs2_alloc_restarted *reason_ret,
 				enum ocfs2_extent_tree_type type,
-				void *private)
+				void *obj)
 {
 	int status = 0;
 	int free_extents;
@@ -4522,7 +4518,7 @@ int ocfs2_add_clusters_in_btree(struct ocfs2_super *osb,
 		flags = OCFS2_EXT_UNWRITTEN;
 
 	free_extents = ocfs2_num_free_extents(osb, inode, root_bh, type,
-					      private);
+					      obj);
 	if (free_extents < 0) {
 		status = free_extents;
 		mlog_errno(status);
@@ -4584,7 +4580,7 @@ int ocfs2_add_clusters_in_btree(struct ocfs2_super *osb,
 							 inode, root_bh,
 							 *logical_offset,
 							 block, num_bits, flags,
-							 meta_ac, private);
+							 meta_ac, obj);
 	if (status < 0) {
 		mlog_errno(status);
 		goto leave;
@@ -4866,7 +4862,7 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
 			      struct ocfs2_alloc_context *meta_ac,
 			      struct ocfs2_cached_dealloc_ctxt *dealloc,
 			      enum ocfs2_extent_tree_type et_type,
-			      void *private)
+			      void *obj)
 {
 	int ret, index;
 	u64 start_blkno = ocfs2_clusters_to_blocks(inode->i_sb, phys);
@@ -4878,7 +4874,7 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
 	mlog(0, "Inode %lu cpos %u, len %u, phys %u (%llu)\n",
 	     inode->i_ino, cpos, len, phys, (unsigned long long)start_blkno);
 
-	ocfs2_get_extent_tree(&et, inode, root_bh, et_type, private);
+	ocfs2_get_extent_tree(&et, inode, root_bh, et_type, obj);
 
 	if (!ocfs2_writes_unwritten_extents(OCFS2_SB(inode->i_sb))) {
 		ocfs2_error(inode->i_sb, "Inode %llu has unwritten extents "
@@ -5170,7 +5166,7 @@ int ocfs2_remove_extent(struct inode *inode, struct buffer_head *root_bh,
 			struct ocfs2_alloc_context *meta_ac,
 			struct ocfs2_cached_dealloc_ctxt *dealloc,
 			enum ocfs2_extent_tree_type et_type,
-			void *private)
+			void *obj)
 {
 	int ret, index;
 	u32 rec_range, trunc_range;
@@ -5179,7 +5175,7 @@ int ocfs2_remove_extent(struct inode *inode, struct buffer_head *root_bh,
 	struct ocfs2_path *path = NULL;
 	struct ocfs2_extent_tree et;
 
-	ocfs2_get_extent_tree(&et, inode, root_bh, et_type, private);
+	ocfs2_get_extent_tree(&et, inode, root_bh, et_type, obj);
 
 	ocfs2_extent_map_trunc(inode, 0);
 
-- 
1.5.6.3

[Ocfs2-devel] [PATCH 04/10] ocfs2: Make 'private' into 'object' on ocfs2_extent_tree.

[TaoMa]

Joel Becker wrote:
> The 'private' pointer was a way to store off xattr values, which don't
> live at a set place in the bh.  But the concept of "the object
> containing the extent tree" is much more generic.  For an inode it's the
> struct ocfs2_dinode, for an xattr value its the value.  Let's save off
> the 'object' at all times.  If NULL is passed to
> ocfs2_get_extent_tree(), 'object' is set to bh->b_data;
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
>   
Signed-off-by: Tao Ma <tao.ma@oracle.com>

[Ocfs2-devel] [PATCH 05/10] ocfs2: Provide the get_root_el() method to ocfs2_extent_tree_operations.

[Joel Becker]

The root_el of an ocfs2_extent_tree needs to be calculated from
et->et_object.  Make it an operation on et->et_ops.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/alloc.c |   38 ++++++++++++++++++++++++++++++--------
 1 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index 93f44f4..fb6ae67 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -72,6 +72,10 @@ struct ocfs2_extent_tree_operations {
 				   struct ocfs2_extent_tree *et,
 				   u32 new_clusters);
 	int (*eo_sanity_check)(struct inode *inode, struct ocfs2_extent_tree *et);
+
+	/* These are internal to ocfs2_extent_tree and don't have
+	 * accessor functions */
+	void (*eo_fill_root_el)(struct ocfs2_extent_tree *et);
 };
 
 struct ocfs2_extent_tree {
@@ -83,6 +87,13 @@ struct ocfs2_extent_tree {
 	unsigned int				et_max_leaf_clusters;
 };
 
+static void ocfs2_dinode_fill_root_el(struct ocfs2_extent_tree *et)
+{
+	struct ocfs2_dinode *di = et->et_object;
+
+	et->et_root_el = &di->id2.i_list;
+}
+
 static void ocfs2_dinode_set_last_eb_blk(struct ocfs2_extent_tree *et,
 					 u64 blkno)
 {
@@ -136,8 +147,16 @@ static struct ocfs2_extent_tree_operations ocfs2_dinode_et_ops = {
 	.eo_get_last_eb_blk	= ocfs2_dinode_get_last_eb_blk,
 	.eo_update_clusters	= ocfs2_dinode_update_clusters,
 	.eo_sanity_check	= ocfs2_dinode_sanity_check,
+	.eo_fill_root_el	= ocfs2_dinode_fill_root_el,
 };
 
+static void ocfs2_xattr_value_fill_root_el(struct ocfs2_extent_tree *et)
+{
+	struct ocfs2_xattr_value_root *xv = et->et_object;
+
+	et->et_root_el = &xv->xr_list;
+}
+
 static void ocfs2_xattr_value_set_last_eb_blk(struct ocfs2_extent_tree *et,
 					      u64 blkno)
 {
@@ -176,8 +195,16 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_et_ops = {
 	.eo_get_last_eb_blk	= ocfs2_xattr_value_get_last_eb_blk,
 	.eo_update_clusters	= ocfs2_xattr_value_update_clusters,
 	.eo_sanity_check	= ocfs2_xattr_value_sanity_check,
+	.eo_fill_root_el	= ocfs2_xattr_value_fill_root_el,
 };
 
+static void ocfs2_xattr_tree_fill_root_el(struct ocfs2_extent_tree *et)
+{
+	struct ocfs2_xattr_block *xb = et->et_object;
+
+	et->et_root_el = &xb->xb_attrs.xb_root.xt_list;
+}
+
 static void ocfs2_xattr_tree_set_last_eb_blk(struct ocfs2_extent_tree *et,
 					     u64 blkno)
 {
@@ -215,6 +242,7 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = {
 	.eo_get_last_eb_blk	= ocfs2_xattr_tree_get_last_eb_blk,
 	.eo_update_clusters	= ocfs2_xattr_tree_update_clusters,
 	.eo_sanity_check	= ocfs2_xattr_tree_sanity_check,
+	.eo_fill_root_el	= ocfs2_xattr_tree_fill_root_el,
 };
 
 static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
@@ -232,22 +260,16 @@ static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
 	et->et_object = obj;
 
 	if (et_type == OCFS2_DINODE_EXTENT) {
-		et->et_root_el =
-			&((struct ocfs2_dinode *)obj)->id2.i_list;
 		et->et_ops = &ocfs2_dinode_et_ops;
 	} else if (et_type == OCFS2_XATTR_VALUE_EXTENT) {
-		struct ocfs2_xattr_value_root *xv =
-			(struct ocfs2_xattr_value_root *)obj;
-		et->et_root_el = &xv->xr_list;
 		et->et_ops = &ocfs2_xattr_et_ops;
 	} else if (et_type == OCFS2_XATTR_TREE_EXTENT) {
-		struct ocfs2_xattr_block *xb =
-			(struct ocfs2_xattr_block *)obj;
-		et->et_root_el = &xb->xb_attrs.xb_root.xt_list;
 		et->et_ops = &ocfs2_xattr_tree_et_ops;
 		et->et_max_leaf_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
 						OCFS2_MAX_XATTR_TREE_LEAF_SIZE);
 	}
+
+	et->et_ops->eo_fill_root_el(et);
 }
 
 static void ocfs2_put_extent_tree(struct ocfs2_extent_tree *et)
-- 
1.5.6.3

[Ocfs2-devel] [PATCH 05/10] ocfs2: Provide the get_root_el() method to ocfs2_extent_tree_operations.

[TaoMa]

Joel Becker wrote:
> The root_el of an ocfs2_extent_tree needs to be calculated from
> et->et_object.  Make it an operation on et->et_ops.
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
>
>  
>  static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
> @@ -232,22 +260,16 @@ static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
>  	et->et_object = obj;
>  
>  	if (et_type == OCFS2_DINODE_EXTENT) {
> -		et->et_root_el =
> -			&((struct ocfs2_dinode *)obj)->id2.i_list;
>  		et->et_ops = &ocfs2_dinode_et_ops;
>  	} else if (et_type == OCFS2_XATTR_VALUE_EXTENT) {
> -		struct ocfs2_xattr_value_root *xv =
> -			(struct ocfs2_xattr_value_root *)obj;
> -		et->et_root_el = &xv->xr_list;
>  		et->et_ops = &ocfs2_xattr_et_ops;
>   
Since there is only one line, the brackets are unnecessary. I don't 
recall whether checkpatch or "make sparse" will complain this. ;)

Regards,
Tao

[Ocfs2-devel] [PATCH 05/10] ocfs2: Provide the get_root_el() method to ocfs2_extent_tree_operations.

[Joel Becker]

On Thu, Aug 21, 2008 at 12:07:48PM +0800, TaoMa wrote:
> Joel Becker wrote:
>> The root_el of an ocfs2_extent_tree needs to be calculated from
>> et->et_object.  Make it an operation on et->et_ops.
>>
>> Signed-off-by: Joel Becker <joel.becker@oracle.com>
>>
>>   static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
>> @@ -232,22 +260,16 @@ static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
>>  	et->et_object = obj;
>>   	if (et_type == OCFS2_DINODE_EXTENT) {
>> -		et->et_root_el =
>> -			&((struct ocfs2_dinode *)obj)->id2.i_list;
>>  		et->et_ops = &ocfs2_dinode_et_ops;
>>  	} else if (et_type == OCFS2_XATTR_VALUE_EXTENT) {
>> -		struct ocfs2_xattr_value_root *xv =
>> -			(struct ocfs2_xattr_value_root *)obj;
>> -		et->et_root_el = &xv->xr_list;
>>  		et->et_ops = &ocfs2_xattr_et_ops;
>>   
> Since there is only one line, the brackets are unnecessary. I don't  
> recall whether checkpatch or "make sparse" will complain this. ;)

	It's checkpatch, but these go away in a later patch.

Joel

-- 

Life's Little Instruction Book #109

	"Know how to drive a stick shift."

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

[Ocfs2-devel] [PATCH 06/10] ocfs2: Use struct ocfs2_extent_tree in ocfs2_num_free_extents().

[Joel Becker]

ocfs2_num_free_extents() re-implements the logic of
ocfs2_get_extent_tree().  Now that ocfs2_get_extent_tree() does not
allocate, let's use it in ocfs2_num_free_extents() to simplify the code.

The inode validation code in ocfs2_num_free_extents() is not needed.
All callers are passing in pre-validated inodes.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/alloc.c |   30 +++++-------------------------
 1 files changed, 5 insertions(+), 25 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index fb6ae67..0b900f6 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -618,34 +618,13 @@ int ocfs2_num_free_extents(struct ocfs2_super *osb,
 	struct ocfs2_extent_block *eb;
 	struct buffer_head *eb_bh = NULL;
 	u64 last_eb_blk = 0;
+	struct ocfs2_extent_tree et;
 
 	mlog_entry_void();
 
-	if (type == OCFS2_DINODE_EXTENT) {
-		struct ocfs2_dinode *fe =
-				(struct ocfs2_dinode *)root_bh->b_data;
-		if (!OCFS2_IS_VALID_DINODE(fe)) {
-			OCFS2_RO_ON_INVALID_DINODE(inode->i_sb, fe);
-			retval = -EIO;
-			goto bail;
-		}
-
-		if (fe->i_last_eb_blk)
-			last_eb_blk = le64_to_cpu(fe->i_last_eb_blk);
-		el = &fe->id2.i_list;
-	} else if (type == OCFS2_XATTR_VALUE_EXTENT) {
-		struct ocfs2_xattr_value_root *xv =
-			(struct ocfs2_xattr_value_root *) obj;
-
-		last_eb_blk = le64_to_cpu(xv->xr_last_eb_blk);
-		el = &xv->xr_list;
-	} else if (type == OCFS2_XATTR_TREE_EXTENT) {
-		struct ocfs2_xattr_block *xb =
-			(struct ocfs2_xattr_block *)root_bh->b_data;
-
-		last_eb_blk = le64_to_cpu(xb->xb_attrs.xb_root.xt_last_eb_blk);
-		el = &xb->xb_attrs.xb_root.xt_list;
-	}
+	ocfs2_get_extent_tree(&et, inode, root_bh, type, obj);
+	el = et.et_root_el;
+	last_eb_blk = ocfs2_et_get_last_eb_blk(&et);
 
 	if (last_eb_blk) {
 		retval = ocfs2_read_block(osb, last_eb_blk,
@@ -665,6 +644,7 @@ bail:
 	if (eb_bh)
 		brelse(eb_bh);
 
+	ocfs2_put_extent_tree(&et);
 	mlog_exit(retval);
 	return retval;
 }
-- 
1.5.6.3

[Ocfs2-devel] [PATCH 06/10] ocfs2: Use struct ocfs2_extent_tree in ocfs2_num_free_extents().

[TaoMa]

Joel Becker wrote:
> ocfs2_num_free_extents() re-implements the logic of
> ocfs2_get_extent_tree().  Now that ocfs2_get_extent_tree() does not
> allocate, let's use it in ocfs2_num_free_extents() to simplify the code.
>
> The inode validation code in ocfs2_num_free_extents() is not needed.
> All callers are passing in pre-validated inodes.
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
>  fs/ocfs2/alloc.c |   30 +++++-------------------------
>  1 files changed, 5 insertions(+), 25 deletions(-)
>
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index fb6ae67..0b900f6 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -618,34 +618,13 @@ int ocfs2_num_free_extents(struct ocfs2_super *osb,
>  	struct ocfs2_extent_block *eb;
>  	struct buffer_head *eb_bh = NULL;
>  	u64 last_eb_blk = 0;
> +	struct ocfs2_extent_tree et;
>  
>  	mlog_entry_void();
>  
> -	if (type == OCFS2_DINODE_EXTENT) {
> -		struct ocfs2_dinode *fe =
> -				(struct ocfs2_dinode *)root_bh->b_data;
> -		if (!OCFS2_IS_VALID_DINODE(fe)) {
> -			OCFS2_RO_ON_INVALID_DINODE(inode->i_sb, fe);
> -			retval = -EIO;
> -			goto bail;
> -		}
> -
> -		if (fe->i_last_eb_blk)
> -			last_eb_blk = le64_to_cpu(fe->i_last_eb_blk);
> -		el = &fe->id2.i_list;
> -	} else if (type == OCFS2_XATTR_VALUE_EXTENT) {
> -		struct ocfs2_xattr_value_root *xv =
> -			(struct ocfs2_xattr_value_root *) obj;
> -
> -		last_eb_blk = le64_to_cpu(xv->xr_last_eb_blk);
> -		el = &xv->xr_list;
> -	} else if (type == OCFS2_XATTR_TREE_EXTENT) {
> -		struct ocfs2_xattr_block *xb =
> -			(struct ocfs2_xattr_block *)root_bh->b_data;
> -
> -		last_eb_blk = le64_to_cpu(xb->xb_attrs.xb_root.xt_last_eb_blk);
> -		el = &xb->xb_attrs.xb_root.xt_list;
> -	}
> +	ocfs2_get_extent_tree(&et, inode, root_bh, type, obj);
>   
call eo_sanity_check first may be better since there is a check above in 
OCFS2_DINODE_EXTENT.
> +	el = et.et_root_el;
> +	last_eb_blk = ocfs2_et_get_last_eb_blk(&et);
>  
>  	if (last_eb_blk) {
>  		retval = ocfs2_read_block(osb, last_eb_blk,
> @@ -665,6 +644,7 @@ bail:
>  	if (eb_bh)
>  		brelse(eb_bh);
>  
> +	ocfs2_put_extent_tree(&et);
>  	mlog_exit(retval);
>  	return retval;
>  }
>   

Regards,
Tao

[Ocfs2-devel] [PATCH 06/10] ocfs2: Use struct ocfs2_extent_tree in ocfs2_num_free_extents().

[Joel Becker]

On Thu, Aug 21, 2008 at 12:10:21PM +0800, TaoMa wrote:
> Joel Becker wrote:
>> ocfs2_num_free_extents() re-implements the logic of
>> ocfs2_get_extent_tree().  Now that ocfs2_get_extent_tree() does not
>> allocate, let's use it in ocfs2_num_free_extents() to simplify the code.
>>
>> The inode validation code in ocfs2_num_free_extents() is not needed.
>> All callers are passing in pre-validated inodes.
>>
>> Signed-off-by: Joel Becker <joel.becker@oracle.com>
>> ---
>>  fs/ocfs2/alloc.c |   30 +++++-------------------------
>>  1 files changed, 5 insertions(+), 25 deletions(-)
>>
>> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
>> index fb6ae67..0b900f6 100644
>> --- a/fs/ocfs2/alloc.c
>> +++ b/fs/ocfs2/alloc.c
>> @@ -618,34 +618,13 @@ int ocfs2_num_free_extents(struct ocfs2_super *osb,
>>  	struct ocfs2_extent_block *eb;
>>  	struct buffer_head *eb_bh = NULL;
>>  	u64 last_eb_blk = 0;
>> +	struct ocfs2_extent_tree et;
>>   	mlog_entry_void();
>>  -	if (type == OCFS2_DINODE_EXTENT) {
>> -		struct ocfs2_dinode *fe =
>> -				(struct ocfs2_dinode *)root_bh->b_data;
>> -		if (!OCFS2_IS_VALID_DINODE(fe)) {
>> -			OCFS2_RO_ON_INVALID_DINODE(inode->i_sb, fe);
>> -			retval = -EIO;
>> -			goto bail;
>> -		}
>> -
>> -		if (fe->i_last_eb_blk)
>> -			last_eb_blk = le64_to_cpu(fe->i_last_eb_blk);
>> -		el = &fe->id2.i_list;
>> -	} else if (type == OCFS2_XATTR_VALUE_EXTENT) {
>> -		struct ocfs2_xattr_value_root *xv =
>> -			(struct ocfs2_xattr_value_root *) obj;
>> -
>> -		last_eb_blk = le64_to_cpu(xv->xr_last_eb_blk);
>> -		el = &xv->xr_list;
>> -	} else if (type == OCFS2_XATTR_TREE_EXTENT) {
>> -		struct ocfs2_xattr_block *xb =
>> -			(struct ocfs2_xattr_block *)root_bh->b_data;
>> -
>> -		last_eb_blk = le64_to_cpu(xb->xb_attrs.xb_root.xt_last_eb_blk);
>> -		el = &xb->xb_attrs.xb_root.xt_list;
>> -	}
>> +	ocfs2_get_extent_tree(&et, inode, root_bh, type, obj);
>>   
> call eo_sanity_check first may be better since there is a check above in  
> OCFS2_DINODE_EXTENT.

	Every dinode caller has already checked the inode.  I looked at
them before I removed the IS_VALID_INODE().  That's what I meant in the
commit message.

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 06/10] ocfs2: Use struct ocfs2_extent_tree in ocfs2_num_free_extents().

[TaoMa]

Joel Becker wrote:
> On Thu, Aug 21, 2008 at 12:10:21PM +0800, TaoMa wrote:
>   
>> Joel Becker wrote:
>>     
>>> ocfs2_num_free_extents() re-implements the logic of
>>> ocfs2_get_extent_tree().  Now that ocfs2_get_extent_tree() does not
>>> allocate, let's use it in ocfs2_num_free_extents() to simplify the code.
>>>
>>> The inode validation code in ocfs2_num_free_extents() is not needed.
>>> All callers are passing in pre-validated inodes.
>>>
>>> Signed-off-by: Joel Becker <joel.becker@oracle.com>
>>> ---
>>>  fs/ocfs2/alloc.c |   30 +++++-------------------------
>>>  1 files changed, 5 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
>>> index fb6ae67..0b900f6 100644
>>> --- a/fs/ocfs2/alloc.c
>>> +++ b/fs/ocfs2/alloc.c
>>> @@ -618,34 +618,13 @@ int ocfs2_num_free_extents(struct ocfs2_super *osb,
>>>  	struct ocfs2_extent_block *eb;
>>>  	struct buffer_head *eb_bh = NULL;
>>>  	u64 last_eb_blk = 0;
>>> +	struct ocfs2_extent_tree et;
>>>   	mlog_entry_void();
>>>  -	if (type == OCFS2_DINODE_EXTENT) {
>>> -		struct ocfs2_dinode *fe =
>>> -				(struct ocfs2_dinode *)root_bh->b_data;
>>> -		if (!OCFS2_IS_VALID_DINODE(fe)) {
>>> -			OCFS2_RO_ON_INVALID_DINODE(inode->i_sb, fe);
>>> -			retval = -EIO;
>>> -			goto bail;
>>> -		}
>>> -
>>> -		if (fe->i_last_eb_blk)
>>> -			last_eb_blk = le64_to_cpu(fe->i_last_eb_blk);
>>> -		el = &fe->id2.i_list;
>>> -	} else if (type == OCFS2_XATTR_VALUE_EXTENT) {
>>> -		struct ocfs2_xattr_value_root *xv =
>>> -			(struct ocfs2_xattr_value_root *) obj;
>>> -
>>> -		last_eb_blk = le64_to_cpu(xv->xr_last_eb_blk);
>>> -		el = &xv->xr_list;
>>> -	} else if (type == OCFS2_XATTR_TREE_EXTENT) {
>>> -		struct ocfs2_xattr_block *xb =
>>> -			(struct ocfs2_xattr_block *)root_bh->b_data;
>>> -
>>> -		last_eb_blk = le64_to_cpu(xb->xb_attrs.xb_root.xt_last_eb_blk);
>>> -		el = &xb->xb_attrs.xb_root.xt_list;
>>> -	}
>>> +	ocfs2_get_extent_tree(&et, inode, root_bh, type, obj);
>>>   
>>>       
>> call eo_sanity_check first may be better since there is a check above in  
>> OCFS2_DINODE_EXTENT.
>>     
>
> 	Every dinode caller has already checked the inode.  I looked at
> them before I removed the IS_VALID_INODE().  That's what I meant in the
> commit message.
>   
OK.
Signed-off-by: Tao Ma <tao.ma@oracle.com>

[Ocfs2-devel] [PATCH 07/10] ocfs2: Determine an extent tree's max_leaf_clusters in an et_op.

[Joel Becker]

Provide an optional extent_tree_operation to specify the
max_leaf_clusters of an ocfs2_extent_tree.  If not provided, the value
is 0 (unlimited).

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/alloc.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index 0b900f6..7c0721d 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -76,6 +76,8 @@ struct ocfs2_extent_tree_operations {
 	/* These are internal to ocfs2_extent_tree and don't have
 	 * accessor functions */
 	void (*eo_fill_root_el)(struct ocfs2_extent_tree *et);
+	void (*eo_fill_max_leaf_clusters)(struct inode *inode,
+					  struct ocfs2_extent_tree *et);
 };
 
 struct ocfs2_extent_tree {
@@ -205,6 +207,14 @@ static void ocfs2_xattr_tree_fill_root_el(struct ocfs2_extent_tree *et)
 	et->et_root_el = &xb->xb_attrs.xb_root.xt_list;
 }
 
+static void ocfs2_xattr_tree_fill_max_leaf_clusters(struct inode *inode,
+						    struct ocfs2_extent_tree *et)
+{
+	et->et_max_leaf_clusters =
+		ocfs2_clusters_for_bytes(inode->i_sb,
+					 OCFS2_MAX_XATTR_TREE_LEAF_SIZE);
+}
+
 static void ocfs2_xattr_tree_set_last_eb_blk(struct ocfs2_extent_tree *et,
 					     u64 blkno)
 {
@@ -243,6 +253,7 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = {
 	.eo_update_clusters	= ocfs2_xattr_tree_update_clusters,
 	.eo_sanity_check	= ocfs2_xattr_tree_sanity_check,
 	.eo_fill_root_el	= ocfs2_xattr_tree_fill_root_el,
+	.eo_fill_max_leaf_clusters = ocfs2_xattr_tree_fill_max_leaf_clusters,
 };
 
 static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
@@ -254,7 +265,6 @@ static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
 	et->et_type = et_type;
 	get_bh(bh);
 	et->et_root_bh = bh;
-	et->et_max_leaf_clusters = 0;
 	if (!obj)
 		obj = (void *)bh->b_data;
 	et->et_object = obj;
@@ -265,11 +275,13 @@ static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
 		et->et_ops = &ocfs2_xattr_et_ops;
 	} else if (et_type == OCFS2_XATTR_TREE_EXTENT) {
 		et->et_ops = &ocfs2_xattr_tree_et_ops;
-		et->et_max_leaf_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
-						OCFS2_MAX_XATTR_TREE_LEAF_SIZE);
 	}
 
 	et->et_ops->eo_fill_root_el(et);
+	if (!et->et_ops->eo_fill_max_leaf_clusters)
+		et->et_max_leaf_clusters = 0;
+	else
+		et->et_ops->eo_fill_max_leaf_clusters(inode, et);
 }
 
 static void ocfs2_put_extent_tree(struct ocfs2_extent_tree *et)
-- 
1.5.6.3

[Ocfs2-devel] [PATCH 07/10] ocfs2: Determine an extent tree's max_leaf_clusters in an et_op.

[TaoMa]

Joel Becker wrote:
> Provide an optional extent_tree_operation to specify the
> max_leaf_clusters of an ocfs2_extent_tree.  If not provided, the value
> is 0 (unlimited).
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
>  fs/ocfs2/alloc.c |   18 +++++++++++++++---
>  1 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index 0b900f6..7c0721d 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -76,6 +76,8 @@ struct ocfs2_extent_tree_operations {
>  	/* These are internal to ocfs2_extent_tree and don't have
>  	 * accessor functions */
>  	void (*eo_fill_root_el)(struct ocfs2_extent_tree *et);
> +	void (*eo_fill_max_leaf_clusters)(struct inode *inode,
> +					  struct ocfs2_extent_tree *et);
>  };
>  
>  struct ocfs2_extent_tree {
> @@ -205,6 +207,14 @@ static void ocfs2_xattr_tree_fill_root_el(struct ocfs2_extent_tree *et)
>  	et->et_root_el = &xb->xb_attrs.xb_root.xt_list;
>  }
>  
> +static void ocfs2_xattr_tree_fill_max_leaf_clusters(struct inode *inode,
> +						    struct ocfs2_extent_tree *et)
> +{
> +	et->et_max_leaf_clusters =
> +		ocfs2_clusters_for_bytes(inode->i_sb,
> +					 OCFS2_MAX_XATTR_TREE_LEAF_SIZE);
> +}
> +
>  static void ocfs2_xattr_tree_set_last_eb_blk(struct ocfs2_extent_tree *et,
>  					     u64 blkno)
>  {
> @@ -243,6 +253,7 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = {
>  	.eo_update_clusters	= ocfs2_xattr_tree_update_clusters,
>  	.eo_sanity_check	= ocfs2_xattr_tree_sanity_check,
>  	.eo_fill_root_el	= ocfs2_xattr_tree_fill_root_el,
> +	.eo_fill_max_leaf_clusters = ocfs2_xattr_tree_fill_max_leaf_clusters,
>  };
>  
>  static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
> @@ -254,7 +265,6 @@ static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
>  	et->et_type = et_type;
>  	get_bh(bh);
>  	et->et_root_bh = bh;
> -	et->et_max_leaf_clusters = 0;
>  	if (!obj)
>  		obj = (void *)bh->b_data;
>  	et->et_object = obj;
> @@ -265,11 +275,13 @@ static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
>  		et->et_ops = &ocfs2_xattr_et_ops;
>  	} else if (et_type == OCFS2_XATTR_TREE_EXTENT) {
>  		et->et_ops = &ocfs2_xattr_tree_et_ops;
> -		et->et_max_leaf_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
> -						OCFS2_MAX_XATTR_TREE_LEAF_SIZE);
>  	}
>  
>  	et->et_ops->eo_fill_root_el(et);
> +	if (!et->et_ops->eo_fill_max_leaf_clusters)
> +		et->et_max_leaf_clusters = 0;
> +	else
> +		et->et_ops->eo_fill_max_leaf_clusters(inode, et);
>  }
>   
Like what you have done in patch 1/10, maybe we can add a small wrapper 
named ocfs2_et_fill_max_leaf_clusters for this?

Regards,
Tao

[Ocfs2-devel] [PATCH 07/10] ocfs2: Determine an extent tree's max_leaf_clusters in an et_op.

[Joel Becker]

On Thu, Aug 21, 2008 at 12:13:15PM +0800, TaoMa wrote:
> Joel Becker wrote:
>> Provide an optional extent_tree_operation to specify the
>> max_leaf_clusters of an ocfs2_extent_tree.  If not provided, the value
>> is 0 (unlimited).
>>

<snip>

>> @@ -265,11 +275,13 @@ static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
>>  		et->et_ops = &ocfs2_xattr_et_ops;
>>  	} else if (et_type == OCFS2_XATTR_TREE_EXTENT) {
>>  		et->et_ops = &ocfs2_xattr_tree_et_ops;
>> -		et->et_max_leaf_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
>> -						OCFS2_MAX_XATTR_TREE_LEAF_SIZE);
>>  	}
>>   	et->et_ops->eo_fill_root_el(et);
>> +	if (!et->et_ops->eo_fill_max_leaf_clusters)
>> +		et->et_max_leaf_clusters = 0;
>> +	else
>> +		et->et_ops->eo_fill_max_leaf_clusters(inode, et);
>>  }
>>   
> Like what you have done in patch 1/10, maybe we can add a small wrapper  
> named ocfs2_et_fill_max_leaf_clusters for this?

	I did it that way at first, but then I decided to make the
eo_fill_*() functions not have an accessor, because they are internal to
get_extent_tree().  They can only be called from there.
	I'm open to changing it.  It jwas just a thought.

Joel

-- 

Life's Little Instruction Book #396

	"Never give anyone a fruitcake."

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

[Ocfs2-devel] [PATCH 07/10] ocfs2: Determine an extent tree's max_leaf_clusters in an et_op.

[Mark Fasheh]

On Wed, Aug 20, 2008 at 07:48:22PM -0700, Joel Becker wrote:
> Provide an optional extent_tree_operation to specify the
> max_leaf_clusters of an ocfs2_extent_tree.  If not provided, the value
> is 0 (unlimited).
> 
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
>  fs/ocfs2/alloc.c |   18 +++++++++++++++---
>  1 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index 0b900f6..7c0721d 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -76,6 +76,8 @@ struct ocfs2_extent_tree_operations {
>  	/* These are internal to ocfs2_extent_tree and don't have
>  	 * accessor functions */
>  	void (*eo_fill_root_el)(struct ocfs2_extent_tree *et);

A comment here, noting that it's optional and what the expected behavior is
if it isn't set would be nice. Otherwise, this patch looks great.
	--Mark

--
Mark Fasheh

[Ocfs2-devel] [PATCH 07/10] ocfs2: Determine an extent tree's max_leaf_clusters in an et_op.

[Joel Becker]

On Thu, Aug 21, 2008 at 03:25:04PM -0700, Mark Fasheh wrote:
> On Wed, Aug 20, 2008 at 07:48:22PM -0700, Joel Becker wrote:
> > --- a/fs/ocfs2/alloc.c
> > +++ b/fs/ocfs2/alloc.c
> > @@ -76,6 +76,8 @@ struct ocfs2_extent_tree_operations {
> >  	/* These are internal to ocfs2_extent_tree and don't have
> >  	 * accessor functions */
> >  	void (*eo_fill_root_el)(struct ocfs2_extent_tree *et);
> 
> A comment here, noting that it's optional and what the expected behavior is
> if it isn't set would be nice. Otherwise, this patch looks great.

	eo_fill_root_el() is required, but really I think the entire set
of ops should be commented.  I only didn't because the initial set
wasn't.  I was wrong, and probably should have even specified the
comments in the original review of Tao's patches.

Joel

-- 

"I'm drifting and drifting
 Just like a ship out on the sea.
 Cause I ain't got nobody, baby,
 In this world to care for me."

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

[Ocfs2-devel] [PATCH 07/10] ocfs2: Determine an extent tree's max_leaf_clusters in an et_op.

[Joel Becker]

On Thu, Aug 21, 2008 at 04:29:20PM -0700, Joel Becker wrote:
> On Thu, Aug 21, 2008 at 03:25:04PM -0700, Mark Fasheh wrote:
> > On Wed, Aug 20, 2008 at 07:48:22PM -0700, Joel Becker wrote:
> > > --- a/fs/ocfs2/alloc.c
> > > +++ b/fs/ocfs2/alloc.c
> > > @@ -76,6 +76,8 @@ struct ocfs2_extent_tree_operations {
> > >  	/* These are internal to ocfs2_extent_tree and don't have
> > >  	 * accessor functions */
> > >  	void (*eo_fill_root_el)(struct ocfs2_extent_tree *et);
> > 
> > A comment here, noting that it's optional and what the expected behavior is
> > if it isn't set would be nice. Otherwise, this patch looks great.
> 
> 	eo_fill_root_el() is required, but really I think the entire set
> of ops should be commented.  I only didn't because the initial set
> wasn't.  I was wrong, and probably should have even specified the
> comments in the original review of Tao's patches.

	What about this:

From f11a2890332ac830c87d0c9edef8f8f1a337c505 Mon Sep 17 00:00:00 2001
From: Joel Becker <joel.becker@oracle.com>
Date: Thu, 21 Aug 2008 17:11:10 -0700
Subject: [PATCH] ocfs2: Comment struct ocfs2_extent_tree_operations.

struct ocfs2_extent_tree_operations provides methods for the different
on-disk btrees in ocfs2.  Describing what those methods do is probably a
good idea.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/alloc.c |   45 +++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index 3ca6087..bbd7cb7 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -50,21 +50,62 @@
 #include "buffer_head_io.h"
 
 
+/*
+ * Operations for a specific extent tree type.
+ *
+ * To implement an on-disk btree (extent tree) type in ocfs2, add
+ * an ocfs2_extent_tree_operations structure and the matching
+ * ocfs2_get_<thingy>_extent_tree() function.  That's pretty much it
+ * for the allocation portion of the extent tree.
+ */
 struct ocfs2_extent_tree_operations {
+	/*
+	 * last_eb_blk is the block number of the right most leaf extent
+	 * block.  Most on-disk structures containing an extent tree store
+	 * this value for fast access.  The ->eo_set_last_eb_blk() and
+	 * ->eo_get_last_eb_blk() operations access this value.  They are
+	 *  both required.
+	 */
 	void (*eo_set_last_eb_blk)(struct ocfs2_extent_tree *et,
 				   u64 blkno);
 	u64 (*eo_get_last_eb_blk)(struct ocfs2_extent_tree *et);
+
+	/*
+	 * The on-disk structure usually keeps track of how many total
+	 * clusters are stored in this extent tree.  This function updates
+	 * that value.  new_clusters is the delta, and must be
+	 * added to the total.  Required.
+	 */
 	void (*eo_update_clusters)(struct inode *inode,
 				   struct ocfs2_extent_tree *et,
 				   u32 new_clusters);
+
+	/*
+	 * If ->eo_insert_check() exists, it is called before rec is
+	 * inserted into the extent tree.  It is optional.
+	 */
 	int (*eo_insert_check)(struct inode *inode,
 			       struct ocfs2_extent_tree *et,
 			       struct ocfs2_extent_rec *rec);
 	int (*eo_remove_check)(struct inode *inode, struct ocfs2_extent_tree *et);
 
-	/* These are internal to ocfs2_extent_tree and don't have
-	 * accessor functions */
+	/*
+	 * --------------------------------------------------------------
+	 * The remaining are internal to ocfs2_extent_tree and don't have
+	 * accessor functions
+	 */
+
+	/*
+	 * ->eo_fill_root_el() takes et->et_object and sets et->et_root_el.
+	 * It is required.
+	 */
 	void (*eo_fill_root_el)(struct ocfs2_extent_tree *et);
+
+	/*
+	 * ->eo_fill_max_leaf_clusters sets et->et_max_leaf_clusters if
+	 * it exists.  If it does not, et->et_max_leaf_clusters is set
+	 * to 0 (unlimited).  Optional.
+	 */
 	void (*eo_fill_max_leaf_clusters)(struct inode *inode,
 					  struct ocfs2_extent_tree *et);
 };
-- 
1.5.6.3


-- 

"The doctrine of human equality reposes on this: that there is no
 man really clever who has not found that he is stupid."
	- Gilbert K. Chesterson

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

[Ocfs2-devel] [PATCH 08/10] ocfs2: Create specific get_extent_tree functions.

[Joel Becker]

A caller knows what kind of extent tree they have.  There's no reason
they have to call ocfs2_get_extent_tree() with a NULL when they could
just as easily call a specific function to their type of extent tree.

Introduce ocfs2_dinode_get_extent_tree(),
ocfs2_xattr_tree_get_extent_tree(), and
ocfs2_xattr_value_get_extent_tree().  They only take the necessary
arguments, calling into the underlying __ocfs2_get_extent_tree() to do
the real work.

__ocfs2_get_extent_tree() is the old ocfs2_get_extent_tree(), but
without needing any switch-by-type logic.

ocfs2_get_extent_tree() is now a wrapper around the specific calls.  It
exists because a couple alloc.c functions can take et_type.  This will
go later.

Another benefit is that ocfs2_xattr_value_get_extent_tree() can take a
struct ocfs2_xattr_value_root* instead of void*.  This gives us
typechecking where we didn't have it before.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/alloc.c |   76 +++++++++++++++++++++++++++++++++++++++---------------
 fs/ocfs2/alloc.h |    2 +-
 2 files changed, 56 insertions(+), 22 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index 7c0721d..243bacf 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -192,7 +192,7 @@ static int ocfs2_xattr_value_sanity_check(struct inode *inode,
 	return 0;
 }
 
-static struct ocfs2_extent_tree_operations ocfs2_xattr_et_ops = {
+static struct ocfs2_extent_tree_operations ocfs2_xattr_value_et_ops = {
 	.eo_set_last_eb_blk	= ocfs2_xattr_value_set_last_eb_blk,
 	.eo_get_last_eb_blk	= ocfs2_xattr_value_get_last_eb_blk,
 	.eo_update_clusters	= ocfs2_xattr_value_update_clusters,
@@ -256,27 +256,21 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = {
 	.eo_fill_max_leaf_clusters = ocfs2_xattr_tree_fill_max_leaf_clusters,
 };
 
-static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
-				  struct inode *inode,
-				  struct buffer_head *bh,
-				  enum ocfs2_extent_tree_type et_type,
-				  void *obj)
+static void __ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
+				    struct inode *inode,
+				    struct buffer_head *bh,
+				    void *obj,
+				    enum ocfs2_extent_tree_type et_type,
+				    struct ocfs2_extent_tree_operations *ops)
 {
 	et->et_type = et_type;
+	et->et_ops = ops;
 	get_bh(bh);
 	et->et_root_bh = bh;
 	if (!obj)
 		obj = (void *)bh->b_data;
 	et->et_object = obj;
 
-	if (et_type == OCFS2_DINODE_EXTENT) {
-		et->et_ops = &ocfs2_dinode_et_ops;
-	} else if (et_type == OCFS2_XATTR_VALUE_EXTENT) {
-		et->et_ops = &ocfs2_xattr_et_ops;
-	} else if (et_type == OCFS2_XATTR_TREE_EXTENT) {
-		et->et_ops = &ocfs2_xattr_tree_et_ops;
-	}
-
 	et->et_ops->eo_fill_root_el(et);
 	if (!et->et_ops->eo_fill_max_leaf_clusters)
 		et->et_max_leaf_clusters = 0;
@@ -284,6 +278,49 @@ static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
 		et->et_ops->eo_fill_max_leaf_clusters(inode, et);
 }
 
+static void ocfs2_get_dinode_extent_tree(struct ocfs2_extent_tree *et,
+					 struct inode *inode,
+					 struct buffer_head *bh)
+{
+	__ocfs2_get_extent_tree(et, inode, bh, NULL, OCFS2_DINODE_EXTENT,
+				&ocfs2_dinode_et_ops);
+}
+
+static void ocfs2_get_xattr_tree_extent_tree(struct ocfs2_extent_tree *et,
+					     struct inode *inode,
+					     struct buffer_head *bh)
+{
+	__ocfs2_get_extent_tree(et, inode, bh, NULL,
+				OCFS2_XATTR_TREE_EXTENT,
+				&ocfs2_xattr_tree_et_ops);
+}
+
+static void ocfs2_get_xattr_value_extent_tree(struct ocfs2_extent_tree *et,
+					     struct inode *inode,
+					     struct buffer_head *bh,
+					     struct ocfs2_xattr_value_root *xv)
+{
+	__ocfs2_get_extent_tree(et, inode, bh, xv,
+				OCFS2_XATTR_VALUE_EXTENT,
+				&ocfs2_xattr_value_et_ops);
+}
+
+static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
+				  struct inode *inode,
+				  struct buffer_head *bh,
+				  enum ocfs2_extent_tree_type et_type,
+				  void *obj)
+{
+	if (et_type == OCFS2_DINODE_EXTENT)
+		ocfs2_get_dinode_extent_tree(et, inode, bh);
+	else if (et_type == OCFS2_XATTR_VALUE_EXTENT)
+		ocfs2_get_xattr_tree_extent_tree(et, inode, bh);
+	else if (et_type == OCFS2_XATTR_TREE_EXTENT)
+		ocfs2_get_xattr_value_extent_tree(et, inode, bh, obj);
+	else
+		BUG();
+}
+
 static void ocfs2_put_extent_tree(struct ocfs2_extent_tree *et)
 {
 	brelse(et->et_root_bh);
@@ -4441,8 +4478,7 @@ int ocfs2_dinode_insert_extent(struct ocfs2_super *osb,
 	int status;
 	struct ocfs2_extent_tree et;
 
-	ocfs2_get_extent_tree(&et, inode, root_bh, OCFS2_DINODE_EXTENT,
-			      NULL);
+	ocfs2_get_dinode_extent_tree(&et, inode, root_bh);
 	status = ocfs2_insert_extent(osb, handle, inode, root_bh,
 				     cpos, start_blk, new_clusters,
 				     flags, meta_ac, &et);
@@ -4460,13 +4496,12 @@ int ocfs2_xattr_value_insert_extent(struct ocfs2_super *osb,
 				    u32 new_clusters,
 				    u8 flags,
 				    struct ocfs2_alloc_context *meta_ac,
-				    void *obj)
+				    struct ocfs2_xattr_value_root *xv)
 {
 	int status;
 	struct ocfs2_extent_tree et;
 
-	ocfs2_get_extent_tree(&et, inode, root_bh,
-			      OCFS2_XATTR_VALUE_EXTENT, obj);
+	ocfs2_get_xattr_value_extent_tree(&et, inode, root_bh, xv);
 	status = ocfs2_insert_extent(osb, handle, inode, root_bh,
 				     cpos, start_blk, new_clusters,
 				     flags, meta_ac, &et);
@@ -4488,8 +4523,7 @@ int ocfs2_xattr_tree_insert_extent(struct ocfs2_super *osb,
 	int status;
 	struct ocfs2_extent_tree et;
 
-	ocfs2_get_extent_tree(&et, inode, root_bh, OCFS2_XATTR_TREE_EXTENT,
-			      NULL);
+	ocfs2_get_xattr_tree_extent_tree(&et, inode, root_bh);
 	status = ocfs2_insert_extent(osb, handle, inode, root_bh,
 				     cpos, start_blk, new_clusters,
 				     flags, meta_ac, &et);
diff --git a/fs/ocfs2/alloc.h b/fs/ocfs2/alloc.h
index ff40c8f..04a551f 100644
--- a/fs/ocfs2/alloc.h
+++ b/fs/ocfs2/alloc.h
@@ -56,7 +56,7 @@ int ocfs2_xattr_value_insert_extent(struct ocfs2_super *osb,
 				    u32 new_clusters,
 				    u8 flags,
 				    struct ocfs2_alloc_context *meta_ac,
-				    void *private);
+				    struct ocfs2_xattr_value_root *xv);
 int ocfs2_xattr_tree_insert_extent(struct ocfs2_super *osb,
 				   handle_t *handle,
 				   struct inode *inode,
-- 
1.5.6.3

[Ocfs2-devel] [PATCH 08/10] ocfs2: Create specific get_extent_tree functions.

[TaoMa]

Joel Becker wrote:
> A caller knows what kind of extent tree they have.  There's no reason
> they have to call ocfs2_get_extent_tree() with a NULL when they could
> just as easily call a specific function to their type of extent tree.
>
> Introduce ocfs2_dinode_get_extent_tree(),
> ocfs2_xattr_tree_get_extent_tree(), and
> ocfs2_xattr_value_get_extent_tree().  They only take the necessary
> arguments, calling into the underlying __ocfs2_get_extent_tree() to do
> the real work.
>
> __ocfs2_get_extent_tree() is the old ocfs2_get_extent_tree(), but
> without needing any switch-by-type logic.
>
> ocfs2_get_extent_tree() is now a wrapper around the specific calls.  It
> exists because a couple alloc.c functions can take et_type.  This will
> go later.
>
> Another benefit is that ocfs2_xattr_value_get_extent_tree() can take a
> struct ocfs2_xattr_value_root* instead of void*.  This gives us
> typechecking where we didn't have it before.
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
>   
Signed-off-by: Tao Ma <tao.ma@oracle.com>

[Ocfs2-devel] [PATCH 09/10] ocfs2: Add an insertion check to ocfs2_extent_tree_operations.

[Joel Becker]

The op eo_sanity_check() was really a check on remove_right_path().
Let's call it eo_remove_check().  Let's add an eo_insert_check() that
can be called from ocfs2_insert_extent().

Let's have the wrapper calls ocfs2_et_insert_check() and
ocfs2_et_remove_check() ignore NULL ops.  That way we don't have to
provide useless operations for xattr types.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/alloc.c |   88 +++++++++++++++++++++++++++++++++---------------------
 1 files changed, 54 insertions(+), 34 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index 243bacf..38abdf1 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -71,7 +71,10 @@ struct ocfs2_extent_tree_operations {
 	void (*eo_update_clusters)(struct inode *inode,
 				   struct ocfs2_extent_tree *et,
 				   u32 new_clusters);
-	int (*eo_sanity_check)(struct inode *inode, struct ocfs2_extent_tree *et);
+	int (*eo_insert_check)(struct inode *inode,
+			       struct ocfs2_extent_tree *et,
+			       struct ocfs2_extent_rec *rec);
+	int (*eo_remove_check)(struct inode *inode, struct ocfs2_extent_tree *et);
 
 	/* These are internal to ocfs2_extent_tree and don't have
 	 * accessor functions */
@@ -125,7 +128,26 @@ static void ocfs2_dinode_update_clusters(struct inode *inode,
 	spin_unlock(&OCFS2_I(inode)->ip_lock);
 }
 
-static int ocfs2_dinode_sanity_check(struct inode *inode,
+static int ocfs2_dinode_insert_check(struct inode *inode,
+				     struct ocfs2_extent_tree *et,
+				     struct ocfs2_extent_rec *rec)
+{
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+
+	BUG_ON(OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL);
+	mlog_bug_on_msg(!ocfs2_sparse_alloc(osb) &&
+			(OCFS2_I(inode)->ip_clusters != rec->e_cpos),
+			"Device %s, asking for sparse allocation: inode %llu, "
+			"cpos %u, clusters %u\n",
+			osb->dev_str,
+			(unsigned long long)OCFS2_I(inode)->ip_blkno,
+			rec->e_cpos,
+			OCFS2_I(inode)->ip_clusters);
+
+	return 0;
+}
+
+static int ocfs2_dinode_remove_check(struct inode *inode,
 				     struct ocfs2_extent_tree *et)
 {
 	int ret = 0;
@@ -148,7 +170,8 @@ static struct ocfs2_extent_tree_operations ocfs2_dinode_et_ops = {
 	.eo_set_last_eb_blk	= ocfs2_dinode_set_last_eb_blk,
 	.eo_get_last_eb_blk	= ocfs2_dinode_get_last_eb_blk,
 	.eo_update_clusters	= ocfs2_dinode_update_clusters,
-	.eo_sanity_check	= ocfs2_dinode_sanity_check,
+	.eo_insert_check	= ocfs2_dinode_insert_check,
+	.eo_remove_check	= ocfs2_dinode_remove_check,
 	.eo_fill_root_el	= ocfs2_dinode_fill_root_el,
 };
 
@@ -186,17 +209,10 @@ static void ocfs2_xattr_value_update_clusters(struct inode *inode,
 	le32_add_cpu(&xv->xr_clusters, clusters);
 }
 
-static int ocfs2_xattr_value_sanity_check(struct inode *inode,
-					  struct ocfs2_extent_tree *et)
-{
-	return 0;
-}
-
 static struct ocfs2_extent_tree_operations ocfs2_xattr_value_et_ops = {
 	.eo_set_last_eb_blk	= ocfs2_xattr_value_set_last_eb_blk,
 	.eo_get_last_eb_blk	= ocfs2_xattr_value_get_last_eb_blk,
 	.eo_update_clusters	= ocfs2_xattr_value_update_clusters,
-	.eo_sanity_check	= ocfs2_xattr_value_sanity_check,
 	.eo_fill_root_el	= ocfs2_xattr_value_fill_root_el,
 };
 
@@ -241,17 +257,10 @@ static void ocfs2_xattr_tree_update_clusters(struct inode *inode,
 	le32_add_cpu(&xb->xb_attrs.xb_root.xt_clusters, clusters);
 }
 
-static int ocfs2_xattr_tree_sanity_check(struct inode *inode,
-					 struct ocfs2_extent_tree *et)
-{
-	return 0;
-}
-
 static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = {
 	.eo_set_last_eb_blk	= ocfs2_xattr_tree_set_last_eb_blk,
 	.eo_get_last_eb_blk	= ocfs2_xattr_tree_get_last_eb_blk,
 	.eo_update_clusters	= ocfs2_xattr_tree_update_clusters,
-	.eo_sanity_check	= ocfs2_xattr_tree_sanity_check,
 	.eo_fill_root_el	= ocfs2_xattr_tree_fill_root_el,
 	.eo_fill_max_leaf_clusters = ocfs2_xattr_tree_fill_max_leaf_clusters,
 };
@@ -344,10 +353,25 @@ static inline void ocfs2_et_update_clusters(struct inode *inode,
 	et->et_ops->eo_update_clusters(inode, et, clusters);
 }
 
-static inline int ocfs2_et_sanity_check(struct inode *inode,
+static inline int ocfs2_et_insert_check(struct inode *inode,
+					struct ocfs2_extent_tree *et,
+					struct ocfs2_extent_rec *rec)
+{
+	int ret = 0;
+
+	if (et->et_ops->eo_insert_check)
+		ret = et->et_ops->eo_insert_check(inode, et, rec);
+	return ret;
+}
+
+static inline int ocfs2_et_remove_check(struct inode *inode,
 					struct ocfs2_extent_tree *et)
 {
-	return et->et_ops->eo_sanity_check(inode, et);
+	int ret = 0;
+
+	if (et->et_ops->eo_remove_check)
+		ret = et->et_ops->eo_remove_check(inode, et);
+	return ret;
 }
 
 static void ocfs2_free_truncate_context(struct ocfs2_truncate_context *tc);
@@ -2744,7 +2768,7 @@ static int ocfs2_remove_rightmost_path(struct inode *inode, handle_t *handle,
 	struct ocfs2_extent_list *el;
 
 
-	ret = ocfs2_et_sanity_check(inode, et);
+	ret = ocfs2_et_remove_check(inode, et);
 	if (ret)
 		goto out;
 	/*
@@ -4406,26 +4430,22 @@ static int ocfs2_insert_extent(struct ocfs2_super *osb,
 	int uninitialized_var(free_records);
 	struct buffer_head *last_eb_bh = NULL;
 	struct ocfs2_insert_type insert = {0, };
-	struct ocfs2_extent_rec rec;
-
-	BUG_ON(OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL);
+	struct ocfs2_extent_rec rec = {
+		.e_cpos = cpu_to_le32(cpos),
+		.e_blkno = cpu_to_le64(start_blk),
+	};
 
 	mlog(0, "add %u clusters at position %u to inode %llu\n",
 	     new_clusters, cpos, (unsigned long long)OCFS2_I(inode)->ip_blkno);
 
-	mlog_bug_on_msg(!ocfs2_sparse_alloc(osb) &&
-			(OCFS2_I(inode)->ip_clusters != cpos),
-			"Device %s, asking for sparse allocation: inode %llu, "
-			"cpos %u, clusters %u\n",
-			osb->dev_str,
-			(unsigned long long)OCFS2_I(inode)->ip_blkno, cpos,
-			OCFS2_I(inode)->ip_clusters);
-
-	memset(&rec, 0, sizeof(rec));
-	rec.e_cpos = cpu_to_le32(cpos);
-	rec.e_blkno = cpu_to_le64(start_blk);
+	/* gcc doesn't understand anonymous unions in the initializer */
 	rec.e_leaf_clusters = cpu_to_le16(new_clusters);
 	rec.e_flags = flags;
+	status = ocfs2_et_insert_check(inode, et, &rec);
+	if (status) {
+		mlog_errno(status);
+		goto bail;
+	}
 
 	status = ocfs2_figure_insert_type(inode, et, &last_eb_bh, &rec,
 					  &free_records, &insert);
-- 
1.5.6.3

[Ocfs2-devel] [PATCH 09/10] ocfs2: Add an insertion check to ocfs2_extent_tree_operations.

[TaoMa]

Joel Becker wrote:
> The op eo_sanity_check() was really a check on remove_right_path().
> Let's call it eo_remove_check().  Let's add an eo_insert_check() that
> can be called from ocfs2_insert_extent().
>   
eo_sanity_check is used to check inode, and I don't see there is any 
hint of remove_right_path in this function although it is now only used 
in ocfs2_remove_rightmost_path. And I have said in the previous 
e-mail,it can be used in ocfs2_num_free_extents.
So I'd like to reserve the name and your add for insert_check is OK or 
it can be moved to ocfs2_*_insert_extent.

Regards,
Tao

[Ocfs2-devel] [PATCH 09/10] ocfs2: Add an insertion check to ocfs2_extent_tree_operations.

[Joel Becker]

On Thu, Aug 21, 2008 at 12:29:01PM +0800, TaoMa wrote:
> Joel Becker wrote:
>> The op eo_sanity_check() was really a check on remove_right_path().
>> Let's call it eo_remove_check().  Let's add an eo_insert_check() that
>> can be called from ocfs2_insert_extent().
>>   
> eo_sanity_check is used to check inode, and I don't see there is any  
> hint of remove_right_path in this function although it is now only used  
> in ocfs2_remove_rightmost_path. And I have said in the previous  
> e-mail,it can be used in ocfs2_num_free_extents.

	As I pointed out there, ocfs2_num_free_extents() doesn't need
the check - the inode is safe coming in.  I suspect remove_right_path is
the same, though I didn't audit it as much.

> So I'd like to reserve the name and your add for insert_check is OK or  
> it can be moved to ocfs2_*_insert_extent.

	I purposely moved the ocfs2_insert_extent() check to the
_insert_check() helper function because ocfs2_*_insert_extent() go away
in patch 10.
	Check that out, and then let's discuss where sanity/remove_check
is needed.  Maybe we can remove it, or maybe we need to expand its
scope.

Joel

-- 

"Heav'n hath no rage like love to hatred turn'd, nor Hell a fury,
 like a woman scorn'd."
        - William Congreve

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

[Ocfs2-devel] [PATCH 09/10] ocfs2: Add an insertion check to ocfs2_extent_tree_operations.

[Mark Fasheh]

On Wed, Aug 20, 2008 at 07:48:24PM -0700, Joel Becker wrote:
> The op eo_sanity_check() was really a check on remove_right_path().
> Let's call it eo_remove_check().  Let's add an eo_insert_check() that
> can be called from ocfs2_insert_extent().
> 
> Let's have the wrapper calls ocfs2_et_insert_check() and
> ocfs2_et_remove_check() ignore NULL ops.  That way we don't have to
> provide useless operations for xattr types.
> 
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
>  fs/ocfs2/alloc.c |   88 +++++++++++++++++++++++++++++++++---------------------
>  1 files changed, 54 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index 243bacf..38abdf1 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -71,7 +71,10 @@ struct ocfs2_extent_tree_operations {
>  	void (*eo_update_clusters)(struct inode *inode,
>  				   struct ocfs2_extent_tree *et,
>  				   u32 new_clusters);
> -	int (*eo_sanity_check)(struct inode *inode, struct ocfs2_extent_tree *et);
> +	int (*eo_insert_check)(struct inode *inode,
> +			       struct ocfs2_extent_tree *et,
> +			       struct ocfs2_extent_rec *rec);
> +	int (*eo_remove_check)(struct inode *inode, struct ocfs2_extent_tree *et);
>  
>  	/* These are internal to ocfs2_extent_tree and don't have
>  	 * accessor functions */
> @@ -125,7 +128,26 @@ static void ocfs2_dinode_update_clusters(struct inode *inode,
>  	spin_unlock(&OCFS2_I(inode)->ip_lock);
>  }
>  
> -static int ocfs2_dinode_sanity_check(struct inode *inode,
> +static int ocfs2_dinode_insert_check(struct inode *inode,
> +				     struct ocfs2_extent_tree *et,
> +				     struct ocfs2_extent_rec *rec)
> +{
> +	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> +
> +	BUG_ON(OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL);
> +	mlog_bug_on_msg(!ocfs2_sparse_alloc(osb) &&
> +			(OCFS2_I(inode)->ip_clusters != rec->e_cpos),
> +			"Device %s, asking for sparse allocation: inode %llu, "
> +			"cpos %u, clusters %u\n",
> +			osb->dev_str,
> +			(unsigned long long)OCFS2_I(inode)->ip_blkno,
> +			rec->e_cpos,
> +			OCFS2_I(inode)->ip_clusters);
> +
> +	return 0;
> +}
> +
> +static int ocfs2_dinode_remove_check(struct inode *inode,
>  				     struct ocfs2_extent_tree *et)
>  {
>  	int ret = 0;
> @@ -148,7 +170,8 @@ static struct ocfs2_extent_tree_operations ocfs2_dinode_et_ops = {
>  	.eo_set_last_eb_blk	= ocfs2_dinode_set_last_eb_blk,
>  	.eo_get_last_eb_blk	= ocfs2_dinode_get_last_eb_blk,
>  	.eo_update_clusters	= ocfs2_dinode_update_clusters,
> -	.eo_sanity_check	= ocfs2_dinode_sanity_check,
> +	.eo_insert_check	= ocfs2_dinode_insert_check,
> +	.eo_remove_check	= ocfs2_dinode_remove_check,
>  	.eo_fill_root_el	= ocfs2_dinode_fill_root_el,
>  };
>  
> @@ -186,17 +209,10 @@ static void ocfs2_xattr_value_update_clusters(struct inode *inode,
>  	le32_add_cpu(&xv->xr_clusters, clusters);
>  }
>  
> -static int ocfs2_xattr_value_sanity_check(struct inode *inode,
> -					  struct ocfs2_extent_tree *et)
> -{
> -	return 0;
> -}
> -
>  static struct ocfs2_extent_tree_operations ocfs2_xattr_value_et_ops = {
>  	.eo_set_last_eb_blk	= ocfs2_xattr_value_set_last_eb_blk,
>  	.eo_get_last_eb_blk	= ocfs2_xattr_value_get_last_eb_blk,
>  	.eo_update_clusters	= ocfs2_xattr_value_update_clusters,
> -	.eo_sanity_check	= ocfs2_xattr_value_sanity_check,
>  	.eo_fill_root_el	= ocfs2_xattr_value_fill_root_el,
>  };
>  
> @@ -241,17 +257,10 @@ static void ocfs2_xattr_tree_update_clusters(struct inode *inode,
>  	le32_add_cpu(&xb->xb_attrs.xb_root.xt_clusters, clusters);
>  }
>  
> -static int ocfs2_xattr_tree_sanity_check(struct inode *inode,
> -					 struct ocfs2_extent_tree *et)
> -{
> -	return 0;
> -}
> -
>  static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = {
>  	.eo_set_last_eb_blk	= ocfs2_xattr_tree_set_last_eb_blk,
>  	.eo_get_last_eb_blk	= ocfs2_xattr_tree_get_last_eb_blk,
>  	.eo_update_clusters	= ocfs2_xattr_tree_update_clusters,
> -	.eo_sanity_check	= ocfs2_xattr_tree_sanity_check,
>  	.eo_fill_root_el	= ocfs2_xattr_tree_fill_root_el,
>  	.eo_fill_max_leaf_clusters = ocfs2_xattr_tree_fill_max_leaf_clusters,
>  };
> @@ -344,10 +353,25 @@ static inline void ocfs2_et_update_clusters(struct inode *inode,
>  	et->et_ops->eo_update_clusters(inode, et, clusters);
>  }
>  
> -static inline int ocfs2_et_sanity_check(struct inode *inode,
> +static inline int ocfs2_et_insert_check(struct inode *inode,
> +					struct ocfs2_extent_tree *et,
> +					struct ocfs2_extent_rec *rec)
> +{
> +	int ret = 0;
> +
> +	if (et->et_ops->eo_insert_check)
> +		ret = et->et_ops->eo_insert_check(inode, et, rec);
> +	return ret;
> +}
> +
> +static inline int ocfs2_et_remove_check(struct inode *inode,
>  					struct ocfs2_extent_tree *et)
>  {
> -	return et->et_ops->eo_sanity_check(inode, et);
> +	int ret = 0;
> +
> +	if (et->et_ops->eo_remove_check)
> +		ret = et->et_ops->eo_remove_check(inode, et);
> +	return ret;
>  }
>  
>  static void ocfs2_free_truncate_context(struct ocfs2_truncate_context *tc);
> @@ -2744,7 +2768,7 @@ static int ocfs2_remove_rightmost_path(struct inode *inode, handle_t *handle,
>  	struct ocfs2_extent_list *el;
>  
>  
> -	ret = ocfs2_et_sanity_check(inode, et);
> +	ret = ocfs2_et_remove_check(inode, et);
>  	if (ret)
>  		goto out;
>  	/*
> @@ -4406,26 +4430,22 @@ static int ocfs2_insert_extent(struct ocfs2_super *osb,
>  	int uninitialized_var(free_records);
>  	struct buffer_head *last_eb_bh = NULL;
>  	struct ocfs2_insert_type insert = {0, };
> -	struct ocfs2_extent_rec rec;
> -
> -	BUG_ON(OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL);
> +	struct ocfs2_extent_rec rec = {
> +		.e_cpos = cpu_to_le32(cpos),
> +		.e_blkno = cpu_to_le64(start_blk),
> +	};
>  
>  	mlog(0, "add %u clusters at position %u to inode %llu\n",
>  	     new_clusters, cpos, (unsigned long long)OCFS2_I(inode)->ip_blkno);
>  
> -	mlog_bug_on_msg(!ocfs2_sparse_alloc(osb) &&
> -			(OCFS2_I(inode)->ip_clusters != cpos),
> -			"Device %s, asking for sparse allocation: inode %llu, "
> -			"cpos %u, clusters %u\n",
> -			osb->dev_str,
> -			(unsigned long long)OCFS2_I(inode)->ip_blkno, cpos,
> -			OCFS2_I(inode)->ip_clusters);
> -
> -	memset(&rec, 0, sizeof(rec));
> -	rec.e_cpos = cpu_to_le32(cpos);
> -	rec.e_blkno = cpu_to_le64(start_blk);
> +	/* gcc doesn't understand anonymous unions in the initializer */
>  	rec.e_leaf_clusters = cpu_to_le16(new_clusters);
>  	rec.e_flags = flags;

Is there a reason why you changed the way we initialize "rec" for this
patch? I guess the end result is the same - specifically, I want to be 100%
certain that rec.e_reserved1 gets zero'd. Still though, I think the flow we
had before was better.


> +	status = ocfs2_et_insert_check(inode, et, &rec);
> +	if (status) {
> +		mlog_errno(status);
> +		goto bail;
> +	}
>  
>  	status = ocfs2_figure_insert_type(inode, et, &last_eb_bh, &rec,
>  					  &free_records, &insert);
> -- 
> 1.5.6.3
--
Mark Fasheh

[Ocfs2-devel] [PATCH 09/10] ocfs2: Add an insertion check to ocfs2_extent_tree_operations.

[Joel Becker]

On Thu, Aug 21, 2008 at 03:52:39PM -0700, Mark Fasheh wrote:
> On Wed, Aug 20, 2008 at 07:48:24PM -0700, Joel Becker wrote:
> > The op eo_sanity_check() was really a check on remove_right_path().
> > Let's call it eo_remove_check().  Let's add an eo_insert_check() that
> > can be called from ocfs2_insert_extent().
> > 
> > Let's have the wrapper calls ocfs2_et_insert_check() and
> > ocfs2_et_remove_check() ignore NULL ops.  That way we don't have to
> > provide useless operations for xattr types.

<snip>

> > @@ -4406,26 +4430,22 @@ static int ocfs2_insert_extent(struct ocfs2_super *osb,
> >  	int uninitialized_var(free_records);
> >  	struct buffer_head *last_eb_bh = NULL;
> >  	struct ocfs2_insert_type insert = {0, };
> > -	struct ocfs2_extent_rec rec;
> > -
> > -	BUG_ON(OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL);
> > +	struct ocfs2_extent_rec rec = {
> > +		.e_cpos = cpu_to_le32(cpos),
> > +		.e_blkno = cpu_to_le64(start_blk),
> > +	};
> >  
> >  	mlog(0, "add %u clusters at position %u to inode %llu\n",
> >  	     new_clusters, cpos, (unsigned long long)OCFS2_I(inode)->ip_blkno);
> >  
> > -	mlog_bug_on_msg(!ocfs2_sparse_alloc(osb) &&
> > -			(OCFS2_I(inode)->ip_clusters != cpos),
> > -			"Device %s, asking for sparse allocation: inode %llu, "
> > -			"cpos %u, clusters %u\n",
> > -			osb->dev_str,
> > -			(unsigned long long)OCFS2_I(inode)->ip_blkno, cpos,
> > -			OCFS2_I(inode)->ip_clusters);
> > -
> > -	memset(&rec, 0, sizeof(rec));
> > -	rec.e_cpos = cpu_to_le32(cpos);
> > -	rec.e_blkno = cpu_to_le64(start_blk);
> > +	/* gcc doesn't understand anonymous unions in the initializer */
> >  	rec.e_leaf_clusters = cpu_to_le16(new_clusters);
> >  	rec.e_flags = flags;
> 
> Is there a reason why you changed the way we initialize "rec" for this
> patch? I guess the end result is the same - specifically, I want to be 100%
> certain that rec.e_reserved1 gets zero'd. Still though, I think the flow we
> had before was better.

	Well, the naked memset() is ugly (IMHO) when the initializer
works (an initializer ensures that all non-initialized fields are zero).
However, it didn't work quite like I expected, since gcc couldn't handle
the anonymous union in the initializer.  I'll revert it back to the old
way.

Joel

-- 

Life's Little Instruction Book #109

	"Know how to drive a stick shift."

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

[Ocfs2-devel] [PATCH 09/10] ocfs2: Add an insertion check to ocfs2_extent_tree_operations.

[Mark Fasheh]

On Thu, Aug 21, 2008 at 04:25:40PM -0700, Joel Becker wrote:
> > Is there a reason why you changed the way we initialize "rec" for this
> > patch? I guess the end result is the same - specifically, I want to be 100%
> > certain that rec.e_reserved1 gets zero'd. Still though, I think the flow we
> > had before was better.
> 
> 	Well, the naked memset() is ugly (IMHO) when the initializer
> works (an initializer ensures that all non-initialized fields are zero).
> However, it didn't work quite like I expected, since gcc couldn't handle
> the anonymous union in the initializer.  I'll revert it back to the old
> way.

Thanks. Too bad about gcc though :(
	--Mark

--
Mark Fasheh

[Ocfs2-devel] [PATCH 10/10] ocfs2: Make ocfs2_extent_tree the first-class representation of a tree.

[Joel Becker]

We now have three different kinds of extent trees in ocfs2: inode data
(dinode), extended attributes (xattr_tree), and extended attribute
values (xattr_value).  There is a nice abstraction for them,
ocfs2_extent_tree, but it is hidden in alloc.c.  All the calling
functions have to pick amongst a varied API and pass in type bits and
often extraneous pointers.

A better way is to make ocfs2_extent_tree a first-class object.
Everyone converts their object to an ocfs2_extent_tree() via the
ocfs2_get_*_extent_tree() calls, then uses the ocfs2_extent_tree for all
tree calls to alloc.c.

This simplifies a lot of callers, making for readability.  It also
provides an easy way to add additional extent tree types, as they only
need to be defined in alloc.c with a ocfs2_get_<new>_extent_tree()
function.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/alloc.c    |  300 +++++++++++++++-----------------------------------
 fs/ocfs2/alloc.h    |  111 +++++++++++---------
 fs/ocfs2/aops.c     |   16 ++-
 fs/ocfs2/dir.c      |   20 ++--
 fs/ocfs2/file.c     |   34 ++++---
 fs/ocfs2/suballoc.c |   12 +--
 fs/ocfs2/suballoc.h |    6 +-
 fs/ocfs2/xattr.c    |   71 +++++++------
 8 files changed, 238 insertions(+), 332 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index 38abdf1..3ca6087 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -49,20 +49,6 @@
 
 #include "buffer_head_io.h"
 
-/*
- * ocfs2_extent_tree and ocfs2_extent_tree_operations are used to abstract
- * the b-tree operations in ocfs2. Now all the b-tree operations are not
- * limited to ocfs2_dinode only. Any data which need to allocate clusters
- * to store can use b-tree. And it only needs to implement its ocfs2_extent_tree
- * and operation.
- *
- * ocfs2_extent_tree contains info for the root of the b-tree, it must have a
- * root ocfs2_extent_list and a root_bh so that they can be used in the b-tree
- * functions.
- * ocfs2_extent_tree_operations abstract the normal operations we do for
- * the root of extent b-tree.
- */
-struct ocfs2_extent_tree;
 
 struct ocfs2_extent_tree_operations {
 	void (*eo_set_last_eb_blk)(struct ocfs2_extent_tree *et,
@@ -83,28 +69,38 @@ struct ocfs2_extent_tree_operations {
 					  struct ocfs2_extent_tree *et);
 };
 
-struct ocfs2_extent_tree {
-	enum ocfs2_extent_tree_type		et_type;
-	struct ocfs2_extent_tree_operations	*et_ops;
-	struct buffer_head			*et_root_bh;
-	struct ocfs2_extent_list		*et_root_el;
-	void					*et_object;
-	unsigned int				et_max_leaf_clusters;
-};
 
-static void ocfs2_dinode_fill_root_el(struct ocfs2_extent_tree *et)
-{
-	struct ocfs2_dinode *di = et->et_object;
-
-	et->et_root_el = &di->id2.i_list;
-}
+/*
+ * Pre-declare ocfs2_dinode_et_ops so we can use it as a sanity check
+ * in the methods.
+ */
+static u64 ocfs2_dinode_get_last_eb_blk(struct ocfs2_extent_tree *et);
+static void ocfs2_dinode_set_last_eb_blk(struct ocfs2_extent_tree *et,
+					 u64 blkno);
+static void ocfs2_dinode_update_clusters(struct inode *inode,
+					 struct ocfs2_extent_tree *et,
+					 u32 clusters);
+static int ocfs2_dinode_insert_check(struct inode *inode,
+				     struct ocfs2_extent_tree *et,
+				     struct ocfs2_extent_rec *rec);
+static int ocfs2_dinode_remove_check(struct inode *inode,
+				     struct ocfs2_extent_tree *et);
+static void ocfs2_dinode_fill_root_el(struct ocfs2_extent_tree *et);
+static struct ocfs2_extent_tree_operations ocfs2_dinode_et_ops = {
+	.eo_set_last_eb_blk	= ocfs2_dinode_set_last_eb_blk,
+	.eo_get_last_eb_blk	= ocfs2_dinode_get_last_eb_blk,
+	.eo_update_clusters	= ocfs2_dinode_update_clusters,
+	.eo_insert_check	= ocfs2_dinode_insert_check,
+	.eo_remove_check	= ocfs2_dinode_remove_check,
+	.eo_fill_root_el	= ocfs2_dinode_fill_root_el,
+};
 
 static void ocfs2_dinode_set_last_eb_blk(struct ocfs2_extent_tree *et,
 					 u64 blkno)
 {
 	struct ocfs2_dinode *di = et->et_object;
 
-	BUG_ON(et->et_type != OCFS2_DINODE_EXTENT);
+	BUG_ON(et->et_ops != &ocfs2_dinode_et_ops);
 	di->i_last_eb_blk = cpu_to_le64(blkno);
 }
 
@@ -112,7 +108,7 @@ static u64 ocfs2_dinode_get_last_eb_blk(struct ocfs2_extent_tree *et)
 {
 	struct ocfs2_dinode *di = et->et_object;
 
-	BUG_ON(et->et_type != OCFS2_DINODE_EXTENT);
+	BUG_ON(et->et_ops != &ocfs2_dinode_et_ops);
 	return le64_to_cpu(di->i_last_eb_blk);
 }
 
@@ -153,7 +149,7 @@ static int ocfs2_dinode_remove_check(struct inode *inode,
 	int ret = 0;
 	struct ocfs2_dinode *di;
 
-	BUG_ON(et->et_type != OCFS2_DINODE_EXTENT);
+	BUG_ON(et->et_ops != &ocfs2_dinode_et_ops);
 
 	di = et->et_object;
 	if (!OCFS2_IS_VALID_DINODE(di)) {
@@ -166,14 +162,13 @@ static int ocfs2_dinode_remove_check(struct inode *inode,
 	return ret;
 }
 
-static struct ocfs2_extent_tree_operations ocfs2_dinode_et_ops = {
-	.eo_set_last_eb_blk	= ocfs2_dinode_set_last_eb_blk,
-	.eo_get_last_eb_blk	= ocfs2_dinode_get_last_eb_blk,
-	.eo_update_clusters	= ocfs2_dinode_update_clusters,
-	.eo_insert_check	= ocfs2_dinode_insert_check,
-	.eo_remove_check	= ocfs2_dinode_remove_check,
-	.eo_fill_root_el	= ocfs2_dinode_fill_root_el,
-};
+static void ocfs2_dinode_fill_root_el(struct ocfs2_extent_tree *et)
+{
+	struct ocfs2_dinode *di = et->et_object;
+
+	et->et_root_el = &di->id2.i_list;
+}
+
 
 static void ocfs2_xattr_value_fill_root_el(struct ocfs2_extent_tree *et)
 {
@@ -269,10 +264,8 @@ static void __ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
 				    struct inode *inode,
 				    struct buffer_head *bh,
 				    void *obj,
-				    enum ocfs2_extent_tree_type et_type,
 				    struct ocfs2_extent_tree_operations *ops)
 {
-	et->et_type = et_type;
 	et->et_ops = ops;
 	get_bh(bh);
 	et->et_root_bh = bh;
@@ -287,50 +280,31 @@ static void __ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
 		et->et_ops->eo_fill_max_leaf_clusters(inode, et);
 }
 
-static void ocfs2_get_dinode_extent_tree(struct ocfs2_extent_tree *et,
-					 struct inode *inode,
-					 struct buffer_head *bh)
+void ocfs2_get_dinode_extent_tree(struct ocfs2_extent_tree *et,
+				  struct inode *inode,
+				  struct buffer_head *bh)
 {
-	__ocfs2_get_extent_tree(et, inode, bh, NULL, OCFS2_DINODE_EXTENT,
-				&ocfs2_dinode_et_ops);
+	__ocfs2_get_extent_tree(et, inode, bh, NULL, &ocfs2_dinode_et_ops);
 }
 
-static void ocfs2_get_xattr_tree_extent_tree(struct ocfs2_extent_tree *et,
-					     struct inode *inode,
-					     struct buffer_head *bh)
+void ocfs2_get_xattr_tree_extent_tree(struct ocfs2_extent_tree *et,
+				      struct inode *inode,
+				      struct buffer_head *bh)
 {
 	__ocfs2_get_extent_tree(et, inode, bh, NULL,
-				OCFS2_XATTR_TREE_EXTENT,
 				&ocfs2_xattr_tree_et_ops);
 }
 
-static void ocfs2_get_xattr_value_extent_tree(struct ocfs2_extent_tree *et,
-					     struct inode *inode,
-					     struct buffer_head *bh,
-					     struct ocfs2_xattr_value_root *xv)
+void ocfs2_get_xattr_value_extent_tree(struct ocfs2_extent_tree *et,
+				       struct inode *inode,
+				       struct buffer_head *bh,
+				       struct ocfs2_xattr_value_root *xv)
 {
 	__ocfs2_get_extent_tree(et, inode, bh, xv,
-				OCFS2_XATTR_VALUE_EXTENT,
 				&ocfs2_xattr_value_et_ops);
 }
 
-static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et,
-				  struct inode *inode,
-				  struct buffer_head *bh,
-				  enum ocfs2_extent_tree_type et_type,
-				  void *obj)
-{
-	if (et_type == OCFS2_DINODE_EXTENT)
-		ocfs2_get_dinode_extent_tree(et, inode, bh);
-	else if (et_type == OCFS2_XATTR_VALUE_EXTENT)
-		ocfs2_get_xattr_tree_extent_tree(et, inode, bh);
-	else if (et_type == OCFS2_XATTR_TREE_EXTENT)
-		ocfs2_get_xattr_value_extent_tree(et, inode, bh, obj);
-	else
-		BUG();
-}
-
-static void ocfs2_put_extent_tree(struct ocfs2_extent_tree *et)
+void ocfs2_put_extent_tree(struct ocfs2_extent_tree *et)
 {
 	brelse(et->et_root_bh);
 }
@@ -682,22 +656,18 @@ struct ocfs2_merge_ctxt {
  */
 int ocfs2_num_free_extents(struct ocfs2_super *osb,
 			   struct inode *inode,
-			   struct buffer_head *root_bh,
-			   enum ocfs2_extent_tree_type type,
-			   void *obj)
+			   struct ocfs2_extent_tree *et)
 {
 	int retval;
 	struct ocfs2_extent_list *el = NULL;
 	struct ocfs2_extent_block *eb;
 	struct buffer_head *eb_bh = NULL;
 	u64 last_eb_blk = 0;
-	struct ocfs2_extent_tree et;
 
 	mlog_entry_void();
 
-	ocfs2_get_extent_tree(&et, inode, root_bh, type, obj);
-	el = et.et_root_el;
-	last_eb_blk = ocfs2_et_get_last_eb_blk(&et);
+	el = et->et_root_el;
+	last_eb_blk = ocfs2_et_get_last_eb_blk(et);
 
 	if (last_eb_blk) {
 		retval = ocfs2_read_block(osb, last_eb_blk,
@@ -717,7 +687,6 @@ bail:
 	if (eb_bh)
 		brelse(eb_bh);
 
-	ocfs2_put_extent_tree(&et);
 	mlog_exit(retval);
 	return retval;
 }
@@ -4415,16 +4384,15 @@ out:
  *
  * The caller needs to update fe->i_clusters
  */
-static int ocfs2_insert_extent(struct ocfs2_super *osb,
-			       handle_t *handle,
-			       struct inode *inode,
-			       struct buffer_head *root_bh,
-			       u32 cpos,
-			       u64 start_blk,
-			       u32 new_clusters,
-			       u8 flags,
-			       struct ocfs2_alloc_context *meta_ac,
-			       struct ocfs2_extent_tree *et)
+int ocfs2_insert_extent(struct ocfs2_super *osb,
+			handle_t *handle,
+			struct inode *inode,
+			struct ocfs2_extent_tree *et,
+			u32 cpos,
+			u64 start_blk,
+			u32 new_clusters,
+			u8 flags,
+			struct ocfs2_alloc_context *meta_ac)
 {
 	int status;
 	int uninitialized_var(free_records);
@@ -4474,7 +4442,7 @@ static int ocfs2_insert_extent(struct ocfs2_super *osb,
 	status = ocfs2_do_insert_extent(inode, handle, et, &rec, &insert);
 	if (status < 0)
 		mlog_errno(status);
-	else if (et->et_type == OCFS2_DINODE_EXTENT)
+	else if (et->et_ops == &ocfs2_dinode_et_ops)
 		ocfs2_extent_map_insert_rec(inode, &rec);
 
 bail:
@@ -4485,77 +4453,10 @@ bail:
 	return status;
 }
 
-int ocfs2_dinode_insert_extent(struct ocfs2_super *osb,
-			       handle_t *handle,
-			       struct inode *inode,
-			       struct buffer_head *root_bh,
-			       u32 cpos,
-			       u64 start_blk,
-			       u32 new_clusters,
-			       u8 flags,
-			       struct ocfs2_alloc_context *meta_ac)
-{
-	int status;
-	struct ocfs2_extent_tree et;
-
-	ocfs2_get_dinode_extent_tree(&et, inode, root_bh);
-	status = ocfs2_insert_extent(osb, handle, inode, root_bh,
-				     cpos, start_blk, new_clusters,
-				     flags, meta_ac, &et);
-	ocfs2_put_extent_tree(&et);
-
-	return status;
-}
-
-int ocfs2_xattr_value_insert_extent(struct ocfs2_super *osb,
-				    handle_t *handle,
-				    struct inode *inode,
-				    struct buffer_head *root_bh,
-				    u32 cpos,
-				    u64 start_blk,
-				    u32 new_clusters,
-				    u8 flags,
-				    struct ocfs2_alloc_context *meta_ac,
-				    struct ocfs2_xattr_value_root *xv)
-{
-	int status;
-	struct ocfs2_extent_tree et;
-
-	ocfs2_get_xattr_value_extent_tree(&et, inode, root_bh, xv);
-	status = ocfs2_insert_extent(osb, handle, inode, root_bh,
-				     cpos, start_blk, new_clusters,
-				     flags, meta_ac, &et);
-	ocfs2_put_extent_tree(&et);
-
-	return status;
-}
-
-int ocfs2_xattr_tree_insert_extent(struct ocfs2_super *osb,
-				   handle_t *handle,
-				   struct inode *inode,
-				   struct buffer_head *root_bh,
-				   u32 cpos,
-				   u64 start_blk,
-				   u32 new_clusters,
-				   u8 flags,
-				   struct ocfs2_alloc_context *meta_ac)
-{
-	int status;
-	struct ocfs2_extent_tree et;
-
-	ocfs2_get_xattr_tree_extent_tree(&et, inode, root_bh);
-	status = ocfs2_insert_extent(osb, handle, inode, root_bh,
-				     cpos, start_blk, new_clusters,
-				     flags, meta_ac, &et);
-	ocfs2_put_extent_tree(&et);
-
-	return status;
-}
-
 /*
  * Allcate and add clusters into the extent b-tree.
  * The new clusters(clusters_to_add) will be inserted at logical_offset.
- * The extent b-tree's root is root_el and it should be in root_bh, and
+ * The extent b-tree's root is specified by et, and
  * it is not limited to the file storage. Any extent tree can use this
  * function if it implements the proper ocfs2_extent_tree.
  */
@@ -4564,14 +4465,11 @@ int ocfs2_add_clusters_in_btree(struct ocfs2_super *osb,
 				u32 *logical_offset,
 				u32 clusters_to_add,
 				int mark_unwritten,
-				struct buffer_head *root_bh,
-				struct ocfs2_extent_list *root_el,
+				struct ocfs2_extent_tree *et,
 				handle_t *handle,
 				struct ocfs2_alloc_context *data_ac,
 				struct ocfs2_alloc_context *meta_ac,
-				enum ocfs2_alloc_restarted *reason_ret,
-				enum ocfs2_extent_tree_type type,
-				void *obj)
+				enum ocfs2_alloc_restarted *reason_ret)
 {
 	int status = 0;
 	int free_extents;
@@ -4585,8 +4483,7 @@ int ocfs2_add_clusters_in_btree(struct ocfs2_super *osb,
 	if (mark_unwritten)
 		flags = OCFS2_EXT_UNWRITTEN;
 
-	free_extents = ocfs2_num_free_extents(osb, inode, root_bh, type,
-					      obj);
+	free_extents = ocfs2_num_free_extents(osb, inode, et);
 	if (free_extents < 0) {
 		status = free_extents;
 		mlog_errno(status);
@@ -4605,7 +4502,7 @@ int ocfs2_add_clusters_in_btree(struct ocfs2_super *osb,
 		goto leave;
 	} else if ((!free_extents)
 		   && (ocfs2_alloc_context_bits_left(meta_ac)
-		       < ocfs2_extend_meta_needed(root_el))) {
+		       < ocfs2_extend_meta_needed(et->et_root_el))) {
 		mlog(0, "filesystem is really fragmented...\n");
 		status = -EAGAIN;
 		reason = RESTART_META;
@@ -4623,7 +4520,7 @@ int ocfs2_add_clusters_in_btree(struct ocfs2_super *osb,
 	BUG_ON(num_bits > clusters_to_add);
 
 	/* reserve our write early -- insert_extent may update the inode */
-	status = ocfs2_journal_access(handle, inode, root_bh,
+	status = ocfs2_journal_access(handle, inode, et->et_root_bh,
 				      OCFS2_JOURNAL_ACCESS_WRITE);
 	if (status < 0) {
 		mlog_errno(status);
@@ -4633,28 +4530,15 @@ int ocfs2_add_clusters_in_btree(struct ocfs2_super *osb,
 	block = ocfs2_clusters_to_blocks(osb->sb, bit_off);
 	mlog(0, "Allocating %u clusters at block %u for inode %llu\n",
 	     num_bits, bit_off, (unsigned long long)OCFS2_I(inode)->ip_blkno);
-	if (type == OCFS2_DINODE_EXTENT)
-		status = ocfs2_dinode_insert_extent(osb, handle, inode, root_bh,
-						    *logical_offset, block,
-						    num_bits, flags, meta_ac);
-	else if (type == OCFS2_XATTR_TREE_EXTENT)
-		status = ocfs2_xattr_tree_insert_extent(osb, handle,
-							inode, root_bh,
-							*logical_offset,
-							block, num_bits, flags,
-							meta_ac);
-	else
-		status = ocfs2_xattr_value_insert_extent(osb, handle,
-							 inode, root_bh,
-							 *logical_offset,
-							 block, num_bits, flags,
-							 meta_ac, obj);
+	status = ocfs2_insert_extent(osb, handle, inode, et,
+				     *logical_offset, block,
+				     num_bits, flags, meta_ac);
 	if (status < 0) {
 		mlog_errno(status);
 		goto leave;
 	}
 
-	status = ocfs2_journal_dirty(handle, root_bh);
+	status = ocfs2_journal_dirty(handle, et->et_root_bh);
 	if (status < 0) {
 		mlog_errno(status);
 		goto leave;
@@ -4925,25 +4809,21 @@ out:
  *
  * The caller is responsible for passing down meta_ac if we'll need it.
  */
-int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
+int ocfs2_mark_extent_written(struct inode *inode,
+			      struct ocfs2_extent_tree *et,
 			      handle_t *handle, u32 cpos, u32 len, u32 phys,
 			      struct ocfs2_alloc_context *meta_ac,
-			      struct ocfs2_cached_dealloc_ctxt *dealloc,
-			      enum ocfs2_extent_tree_type et_type,
-			      void *obj)
+			      struct ocfs2_cached_dealloc_ctxt *dealloc)
 {
 	int ret, index;
 	u64 start_blkno = ocfs2_clusters_to_blocks(inode->i_sb, phys);
 	struct ocfs2_extent_rec split_rec;
 	struct ocfs2_path *left_path = NULL;
 	struct ocfs2_extent_list *el;
-	struct ocfs2_extent_tree et;
 
 	mlog(0, "Inode %lu cpos %u, len %u, phys %u (%llu)\n",
 	     inode->i_ino, cpos, len, phys, (unsigned long long)start_blkno);
 
-	ocfs2_get_extent_tree(&et, inode, root_bh, et_type, obj);
-
 	if (!ocfs2_writes_unwritten_extents(OCFS2_SB(inode->i_sb))) {
 		ocfs2_error(inode->i_sb, "Inode %llu has unwritten extents "
 			    "that are being written to, but the feature bit "
@@ -4956,11 +4836,14 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
 	/*
 	 * XXX: This should be fixed up so that we just re-insert the
 	 * next extent records.
+	 *
+	 * XXX: This is a hack on the extent tree, maybe it should be
+	 * an op?
 	 */
-	if (et_type == OCFS2_DINODE_EXTENT)
+	if (et->et_ops == &ocfs2_dinode_et_ops)
 		ocfs2_extent_map_trunc(inode, 0);
 
-	left_path = ocfs2_new_path(et.et_root_bh, et.et_root_el);
+	left_path = ocfs2_new_path(et->et_root_bh, et->et_root_el);
 	if (!left_path) {
 		ret = -ENOMEM;
 		mlog_errno(ret);
@@ -4991,7 +4874,7 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
 	split_rec.e_flags = path_leaf_el(left_path)->l_recs[index].e_flags;
 	split_rec.e_flags &= ~OCFS2_EXT_UNWRITTEN;
 
-	ret = __ocfs2_mark_extent_written(inode, &et, handle, left_path,
+	ret = __ocfs2_mark_extent_written(inode, et, handle, left_path,
 					  index, &split_rec, meta_ac,
 					  dealloc);
 	if (ret)
@@ -4999,7 +4882,6 @@ int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
 
 out:
 	ocfs2_free_path(left_path);
-	ocfs2_put_extent_tree(&et);
 	return ret;
 }
 
@@ -5229,25 +5111,21 @@ out:
 	return ret;
 }
 
-int ocfs2_remove_extent(struct inode *inode, struct buffer_head *root_bh,
+int ocfs2_remove_extent(struct inode *inode,
+			struct ocfs2_extent_tree *et,
 			u32 cpos, u32 len, handle_t *handle,
 			struct ocfs2_alloc_context *meta_ac,
-			struct ocfs2_cached_dealloc_ctxt *dealloc,
-			enum ocfs2_extent_tree_type et_type,
-			void *obj)
+			struct ocfs2_cached_dealloc_ctxt *dealloc)
 {
 	int ret, index;
 	u32 rec_range, trunc_range;
 	struct ocfs2_extent_rec *rec;
 	struct ocfs2_extent_list *el;
 	struct ocfs2_path *path = NULL;
-	struct ocfs2_extent_tree et;
-
-	ocfs2_get_extent_tree(&et, inode, root_bh, et_type, obj);
 
 	ocfs2_extent_map_trunc(inode, 0);
 
-	path = ocfs2_new_path(et.et_root_bh, et.et_root_el);
+	path = ocfs2_new_path(et->et_root_bh, et->et_root_el);
 	if (!path) {
 		ret = -ENOMEM;
 		mlog_errno(ret);
@@ -5300,13 +5178,13 @@ int ocfs2_remove_extent(struct inode *inode, struct buffer_head *root_bh,
 
 	if (le32_to_cpu(rec->e_cpos) == cpos || rec_range == trunc_range) {
 		ret = ocfs2_truncate_rec(inode, handle, path, index, dealloc,
-					 cpos, len, &et);
+					 cpos, len, et);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
 		}
 	} else {
-		ret = ocfs2_split_tree(inode, &et, handle, path, index,
+		ret = ocfs2_split_tree(inode, et, handle, path, index,
 				       trunc_range, meta_ac);
 		if (ret) {
 			mlog_errno(ret);
@@ -5355,7 +5233,7 @@ int ocfs2_remove_extent(struct inode *inode, struct buffer_head *root_bh,
 		}
 
 		ret = ocfs2_truncate_rec(inode, handle, path, index, dealloc,
-					 cpos, len, &et);
+					 cpos, len, et);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
@@ -5364,7 +5242,6 @@ int ocfs2_remove_extent(struct inode *inode, struct buffer_head *root_bh,
 
 out:
 	ocfs2_free_path(path);
-	ocfs2_put_extent_tree(&et);
 	return ret;
 }
 
@@ -6783,6 +6660,7 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
 	struct ocfs2_alloc_context *data_ac = NULL;
 	struct page **pages = NULL;
 	loff_t end = osb->s_clustersize;
+	struct ocfs2_extent_tree et;
 
 	has_data = i_size_read(inode) ? 1 : 0;
 
@@ -6882,8 +6760,10 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
 		 * this proves to be false, we could always re-build
 		 * the in-inode data from our pages.
 		 */
-		ret = ocfs2_dinode_insert_extent(osb, handle, inode, di_bh,
-						 0, block, 1, 0, NULL);
+		ocfs2_get_dinode_extent_tree(&et, inode, di_bh);
+		ret = ocfs2_insert_extent(osb, handle, inode, &et,
+					  0, block, 1, 0, NULL);
+		ocfs2_put_extent_tree(&et);
 		if (ret) {
 			mlog_errno(ret);
 			goto out_commit;
diff --git a/fs/ocfs2/alloc.h b/fs/ocfs2/alloc.h
index 04a551f..27311da 100644
--- a/fs/ocfs2/alloc.h
+++ b/fs/ocfs2/alloc.h
@@ -26,46 +26,66 @@
 #ifndef OCFS2_ALLOC_H
 #define OCFS2_ALLOC_H
 
-enum ocfs2_extent_tree_type {
-	OCFS2_DINODE_EXTENT = 0,
-	OCFS2_XATTR_VALUE_EXTENT,
-	OCFS2_XATTR_TREE_EXTENT,
-};
 
 /*
  * For xattr tree leaf, we limit the leaf byte size to be 64K.
  */
 #define OCFS2_MAX_XATTR_TREE_LEAF_SIZE 65536
 
+/*
+ * ocfs2_extent_tree and ocfs2_extent_tree_operations are used to abstract
+ * the b-tree operations in ocfs2. Now all the b-tree operations are not
+ * limited to ocfs2_dinode only. Any data which need to allocate clusters
+ * to store can use b-tree. And it only needs to implement its ocfs2_extent_tree
+ * and operation.
+ *
+ * ocfs2_extent_tree becomes the first-class object for extent tree
+ * manipulation.  Callers of the alloc.c code need to fill it via one of
+ * the ocfs2_get_*_extent_tree() operations below.
+ *
+ * ocfs2_extent_tree contains info for the root of the b-tree, it must have a
+ * root ocfs2_extent_list and a root_bh so that they can be used in the b-tree
+ * functions.
+ * ocfs2_extent_tree_operations abstract the normal operations we do for
+ * the root of extent b-tree.
+ */
+struct ocfs2_extent_tree_operations;
+struct ocfs2_extent_tree {
+	struct ocfs2_extent_tree_operations	*et_ops;
+	struct buffer_head			*et_root_bh;
+	struct ocfs2_extent_list		*et_root_el;
+	void					*et_object;
+	unsigned int				et_max_leaf_clusters;
+};
+
+/*
+ * ocfs2_*_get_extent_tree() will fill an ocfs2_extent_tree from the
+ * specified object buffer.  The bh is referenced until
+ * ocfs2_put_extent_tree().
+ */
+void ocfs2_get_dinode_extent_tree(struct ocfs2_extent_tree *et,
+				  struct inode *inode,
+				  struct buffer_head *bh);
+void ocfs2_get_xattr_tree_extent_tree(struct ocfs2_extent_tree *et,
+				      struct inode *inode,
+				      struct buffer_head *bh);
+void ocfs2_get_xattr_value_extent_tree(struct ocfs2_extent_tree *et,
+				       struct inode *inode,
+				       struct buffer_head *bh,
+				       struct ocfs2_xattr_value_root *xv);
+void ocfs2_put_extent_tree(struct ocfs2_extent_tree *et);
+
 struct ocfs2_alloc_context;
-int ocfs2_dinode_insert_extent(struct ocfs2_super *osb,
-			       handle_t *handle,
-			       struct inode *inode,
-			       struct buffer_head *root_bh,
-			       u32 cpos,
-			       u64 start_blk,
-			       u32 new_clusters,
-			       u8 flags,
-			       struct ocfs2_alloc_context *meta_ac);
-int ocfs2_xattr_value_insert_extent(struct ocfs2_super *osb,
-				    handle_t *handle,
-				    struct inode *inode,
-				    struct buffer_head *root_bh,
-				    u32 cpos,
-				    u64 start_blk,
-				    u32 new_clusters,
-				    u8 flags,
-				    struct ocfs2_alloc_context *meta_ac,
-				    struct ocfs2_xattr_value_root *xv);
-int ocfs2_xattr_tree_insert_extent(struct ocfs2_super *osb,
-				   handle_t *handle,
-				   struct inode *inode,
-				   struct buffer_head *root_bh,
-				   u32 cpos,
-				   u64 start_blk,
-				   u32 new_clusters,
-				   u8 flags,
-				   struct ocfs2_alloc_context *meta_ac);
+int ocfs2_insert_extent(struct ocfs2_super *osb,
+			handle_t *handle,
+			struct inode *inode,
+			struct ocfs2_extent_tree *et,
+			u32 cpos,
+			u64 start_blk,
+			u32 new_clusters,
+			u8 flags,
+			struct ocfs2_alloc_context *meta_ac);
+
 enum ocfs2_alloc_restarted {
 	RESTART_NONE = 0,
 	RESTART_TRANS,
@@ -76,32 +96,25 @@ int ocfs2_add_clusters_in_btree(struct ocfs2_super *osb,
 				u32 *logical_offset,
 				u32 clusters_to_add,
 				int mark_unwritten,
-				struct buffer_head *root_bh,
-				struct ocfs2_extent_list *root_el,
+				struct ocfs2_extent_tree *et,
 				handle_t *handle,
 				struct ocfs2_alloc_context *data_ac,
 				struct ocfs2_alloc_context *meta_ac,
-				enum ocfs2_alloc_restarted *reason_ret,
-				enum ocfs2_extent_tree_type type,
-				void *private);
+				enum ocfs2_alloc_restarted *reason_ret);
 struct ocfs2_cached_dealloc_ctxt;
-int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
+int ocfs2_mark_extent_written(struct inode *inode,
+			      struct ocfs2_extent_tree *et,
 			      handle_t *handle, u32 cpos, u32 len, u32 phys,
 			      struct ocfs2_alloc_context *meta_ac,
-			      struct ocfs2_cached_dealloc_ctxt *dealloc,
-			      enum ocfs2_extent_tree_type et_type,
-			      void *private);
-int ocfs2_remove_extent(struct inode *inode, struct buffer_head *root_bh,
+			      struct ocfs2_cached_dealloc_ctxt *dealloc);
+int ocfs2_remove_extent(struct inode *inode,
+			struct ocfs2_extent_tree *et,
 			u32 cpos, u32 len, handle_t *handle,
 			struct ocfs2_alloc_context *meta_ac,
-			struct ocfs2_cached_dealloc_ctxt *dealloc,
-			enum ocfs2_extent_tree_type et_type,
-			void *private);
+			struct ocfs2_cached_dealloc_ctxt *dealloc);
 int ocfs2_num_free_extents(struct ocfs2_super *osb,
 			   struct inode *inode,
-			   struct buffer_head *root_bh,
-			   enum ocfs2_extent_tree_type et_type,
-			   void *private);
+			   struct ocfs2_extent_tree *et);
 
 /*
  * how many new metadata chunks would an allocation need at maximum?
diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 973550c..d3ffead 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -1242,6 +1242,7 @@ static int ocfs2_write_cluster(struct address_space *mapping,
 	int ret, i, new, should_zero = 0;
 	u64 v_blkno, p_blkno;
 	struct inode *inode = mapping->host;
+	struct ocfs2_extent_tree et;
 
 	new = phys == 0 ? 1 : 0;
 	if (new || unwritten)
@@ -1276,10 +1277,11 @@ static int ocfs2_write_cluster(struct address_space *mapping,
 			goto out;
 		}
 	} else if (unwritten) {
-		ret = ocfs2_mark_extent_written(inode, wc->w_di_bh,
+		ocfs2_get_dinode_extent_tree(&et, inode, wc->w_di_bh);
+		ret = ocfs2_mark_extent_written(inode, &et,
 						wc->w_handle, cpos, 1, phys,
-						meta_ac, &wc->w_dealloc,
-						OCFS2_DINODE_EXTENT, NULL);
+						meta_ac, &wc->w_dealloc);
+		ocfs2_put_extent_tree(&et);
 		if (ret < 0) {
 			mlog_errno(ret);
 			goto out;
@@ -1666,6 +1668,7 @@ int ocfs2_write_begin_nolock(struct address_space *mapping,
 	struct ocfs2_alloc_context *data_ac = NULL;
 	struct ocfs2_alloc_context *meta_ac = NULL;
 	handle_t *handle;
+	struct ocfs2_extent_tree et;
 
 	ret = ocfs2_alloc_write_ctxt(&wc, osb, pos, len, di_bh);
 	if (ret) {
@@ -1719,10 +1722,11 @@ int ocfs2_write_begin_nolock(struct address_space *mapping,
 		     (long long)i_size_read(inode), le32_to_cpu(di->i_clusters),
 		     clusters_to_alloc, extents_to_split);
 
-		ret = ocfs2_lock_allocators(inode, wc->w_di_bh, &di->id2.i_list,
+		ocfs2_get_dinode_extent_tree(&et, inode, wc->w_di_bh);
+		ret = ocfs2_lock_allocators(inode, &et,
 					    clusters_to_alloc, extents_to_split,
-					    &data_ac, &meta_ac,
-					    OCFS2_DINODE_EXTENT, NULL);
+					    &data_ac, &meta_ac);
+		ocfs2_put_extent_tree(&et);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c
index 70988cf..119f9ce 100644
--- a/fs/ocfs2/dir.c
+++ b/fs/ocfs2/dir.c
@@ -1192,6 +1192,9 @@ static int ocfs2_expand_inline_dir(struct inode *dir, struct buffer_head *di_bh,
 	struct buffer_head *dirdata_bh = NULL;
 	struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data;
 	handle_t *handle;
+	struct ocfs2_extent_tree et;
+
+	ocfs2_get_dinode_extent_tree(&et, dir, di_bh);
 
 	alloc = ocfs2_clusters_for_bytes(sb, bytes);
 
@@ -1306,8 +1309,8 @@ static int ocfs2_expand_inline_dir(struct inode *dir, struct buffer_head *di_bh,
 	 * This should never fail as our extent list is empty and all
 	 * related blocks have been journaled already.
 	 */
-	ret = ocfs2_dinode_insert_extent(osb, handle, dir, di_bh, 0, blkno,
-					 len, 0, NULL);
+	ret = ocfs2_insert_extent(osb, handle, dir, &et, 0, blkno, len,
+				  0, NULL);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
@@ -1332,8 +1335,8 @@ static int ocfs2_expand_inline_dir(struct inode *dir, struct buffer_head *di_bh,
 		}
 		blkno = ocfs2_clusters_to_blocks(dir->i_sb, bit_off);
 
-		ret = ocfs2_dinode_insert_extent(osb, handle, dir, di_bh, 1,
-						 blkno, len, 0, NULL);
+		ret = ocfs2_insert_extent(osb, handle, dir, &et, 1,
+					  blkno, len, 0, NULL);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
@@ -1355,6 +1358,7 @@ out:
 
 	brelse(dirdata_bh);
 
+	ocfs2_put_extent_tree(&et);
 	return ret;
 }
 
@@ -1432,6 +1436,7 @@ static int ocfs2_extend_dir(struct ocfs2_super *osb,
 	struct buffer_head *new_bh = NULL;
 	struct ocfs2_dir_entry * de;
 	struct super_block *sb = osb->sb;
+	struct ocfs2_extent_tree et;
 
 	mlog_entry_void();
 
@@ -1475,10 +1480,9 @@ static int ocfs2_extend_dir(struct ocfs2_super *osb,
 	spin_lock(&OCFS2_I(dir)->ip_lock);
 	if (dir_i_size == ocfs2_clusters_to_bytes(sb, OCFS2_I(dir)->ip_clusters)) {
 		spin_unlock(&OCFS2_I(dir)->ip_lock);
-		num_free_extents = ocfs2_num_free_extents(osb, dir,
-							  parent_fe_bh,
-							  OCFS2_DINODE_EXTENT,
-							  NULL);
+		ocfs2_get_dinode_extent_tree(&et, dir, parent_fe_bh);
+		num_free_extents = ocfs2_num_free_extents(osb, dir, &et);
+		ocfs2_put_extent_tree(&et);
 		if (num_free_extents < 0) {
 			status = num_free_extents;
 			mlog_errno(status);
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 65028e2..1f31a99 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -509,14 +509,17 @@ int ocfs2_add_inode_data(struct ocfs2_super *osb,
 			 struct ocfs2_alloc_context *meta_ac,
 			 enum ocfs2_alloc_restarted *reason_ret)
 {
-	struct ocfs2_dinode *fe = (struct ocfs2_dinode *) fe_bh->b_data;
-	struct ocfs2_extent_list *el = &fe->id2.i_list;
+	int ret;
+	struct ocfs2_extent_tree et;
 
-	return ocfs2_add_clusters_in_btree(osb, inode, logical_offset,
+	ocfs2_get_dinode_extent_tree(&et, inode, fe_bh);
+	ret = ocfs2_add_clusters_in_btree(osb, inode, logical_offset,
 					   clusters_to_add, mark_unwritten,
-					   fe_bh, el, handle,
-					   data_ac, meta_ac, reason_ret,
-					   OCFS2_DINODE_EXTENT, NULL);
+					   &et, handle,
+					   data_ac, meta_ac, reason_ret);
+	ocfs2_put_extent_tree(&et);
+
+	return ret;
 }
 
 static int __ocfs2_extend_allocation(struct inode *inode, u32 logical_start,
@@ -533,6 +536,7 @@ static int __ocfs2_extend_allocation(struct inode *inode, u32 logical_start,
 	struct ocfs2_alloc_context *meta_ac = NULL;
 	enum ocfs2_alloc_restarted why;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	struct ocfs2_extent_tree et;
 
 	mlog_entry("(clusters_to_add = %u)\n", clusters_to_add);
 
@@ -564,9 +568,10 @@ restart_all:
 	     (unsigned long long)OCFS2_I(inode)->ip_blkno,
 	     (long long)i_size_read(inode), le32_to_cpu(fe->i_clusters),
 	     clusters_to_add);
-	status = ocfs2_lock_allocators(inode, bh, &fe->id2.i_list,
-				       clusters_to_add, 0, &data_ac,
-				       &meta_ac, OCFS2_DINODE_EXTENT, NULL);
+	ocfs2_get_dinode_extent_tree(&et, inode, bh);
+	status = ocfs2_lock_allocators(inode, &et, clusters_to_add, 0,
+				       &data_ac, &meta_ac);
+	ocfs2_put_extent_tree(&et);
 	if (status) {
 		mlog_errno(status);
 		goto leave;
@@ -1236,10 +1241,11 @@ static int __ocfs2_remove_inode_range(struct inode *inode,
 	handle_t *handle;
 	struct ocfs2_alloc_context *meta_ac = NULL;
 	struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data;
+	struct ocfs2_extent_tree et;
+
+	ocfs2_get_dinode_extent_tree(&et, inode, di_bh);
 
-	ret = ocfs2_lock_allocators(inode, di_bh, &di->id2.i_list,
-				    0, 1, NULL, &meta_ac,
-				    OCFS2_DINODE_EXTENT, NULL);
+	ret = ocfs2_lock_allocators(inode, &et, 0, 1, NULL, &meta_ac);
 	if (ret) {
 		mlog_errno(ret);
 		return ret;
@@ -1269,8 +1275,8 @@ static int __ocfs2_remove_inode_range(struct inode *inode,
 		goto out;
 	}
 
-	ret = ocfs2_remove_extent(inode, di_bh, cpos, len, handle, meta_ac,
-				  dealloc, OCFS2_DINODE_EXTENT, NULL);
+	ret = ocfs2_remove_extent(inode, &et, cpos, len, handle, meta_ac,
+				  dealloc);
 	if (ret) {
 		mlog_errno(ret);
 		goto out_commit;
diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
index d7238a8..a3298f1 100644
--- a/fs/ocfs2/suballoc.c
+++ b/fs/ocfs2/suballoc.c
@@ -1911,12 +1911,11 @@ static inline void ocfs2_debug_suballoc_inode(struct ocfs2_dinode *fe)
  * File systems which don't support holes call this from
  * ocfs2_extend_allocation().
  */
-int ocfs2_lock_allocators(struct inode *inode, struct buffer_head *root_bh,
-			  struct ocfs2_extent_list *root_el,
+int ocfs2_lock_allocators(struct inode *inode,
+			  struct ocfs2_extent_tree *et,
 			  u32 clusters_to_add, u32 extents_to_split,
 			  struct ocfs2_alloc_context **data_ac,
-			  struct ocfs2_alloc_context **meta_ac,
-			  enum ocfs2_extent_tree_type type, void *private)
+			  struct ocfs2_alloc_context **meta_ac)
 {
 	int ret = 0, num_free_extents;
 	unsigned int max_recs_needed = clusters_to_add + 2 * extents_to_split;
@@ -1928,8 +1927,7 @@ int ocfs2_lock_allocators(struct inode *inode, struct buffer_head *root_bh,
 
 	BUG_ON(clusters_to_add != 0 && data_ac == NULL);
 
-	num_free_extents = ocfs2_num_free_extents(osb, inode, root_bh,
-						  type, private);
+	num_free_extents = ocfs2_num_free_extents(osb, inode, et);
 	if (num_free_extents < 0) {
 		ret = num_free_extents;
 		mlog_errno(ret);
@@ -1951,7 +1949,7 @@ int ocfs2_lock_allocators(struct inode *inode, struct buffer_head *root_bh,
 	 */
 	if (!num_free_extents ||
 	    (ocfs2_sparse_alloc(osb) && num_free_extents < max_recs_needed)) {
-		ret = ocfs2_reserve_new_metadata(osb, root_el, meta_ac);
+		ret = ocfs2_reserve_new_metadata(osb, et->et_root_el, meta_ac);
 		if (ret < 0) {
 			if (ret != -ENOSPC)
 				mlog_errno(ret);
diff --git a/fs/ocfs2/suballoc.h b/fs/ocfs2/suballoc.h
index a991819..7f1f966 100644
--- a/fs/ocfs2/suballoc.h
+++ b/fs/ocfs2/suballoc.h
@@ -164,10 +164,8 @@ u64 ocfs2_which_cluster_group(struct inode *inode, u32 cluster);
 int ocfs2_check_group_descriptor(struct super_block *sb,
 				 struct ocfs2_dinode *di,
 				 struct ocfs2_group_desc *gd);
-int ocfs2_lock_allocators(struct inode *inode, struct buffer_head *root_bh,
-			  struct ocfs2_extent_list *root_el,
+int ocfs2_lock_allocators(struct inode *inode, struct ocfs2_extent_tree *et,
 			  u32 clusters_to_add, u32 extents_to_split,
 			  struct ocfs2_alloc_context **data_ac,
-			  struct ocfs2_alloc_context **meta_ac,
-			  enum ocfs2_extent_tree_type type, void *private);
+			  struct ocfs2_alloc_context **meta_ac);
 #endif /* _CHAINALLOC_H_ */
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 6b7685e..1f1e01b 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -222,22 +222,24 @@ static int ocfs2_xattr_extend_allocation(struct inode *inode,
 	struct ocfs2_alloc_context *meta_ac = NULL;
 	enum ocfs2_alloc_restarted why;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
-	struct ocfs2_extent_list *root_el = &xv->xr_list;
 	u32 prev_clusters, logical_start = le32_to_cpu(xv->xr_clusters);
+	struct ocfs2_extent_tree et;
 
 	mlog(0, "(clusters_to_add for xattr= %u)\n", clusters_to_add);
 
+	ocfs2_get_xattr_value_extent_tree(&et, inode, xattr_bh, xv);
+
 restart_all:
 
-	status = ocfs2_lock_allocators(inode, xattr_bh, root_el,
-				       clusters_to_add, 0, &data_ac,
-				       &meta_ac, OCFS2_XATTR_VALUE_EXTENT, xv);
+	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, root_el, clusters_to_add);
+	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);
@@ -260,14 +262,11 @@ restarted_transaction:
 					     &logical_start,
 					     clusters_to_add,
 					     0,
-					     xattr_bh,
-					     root_el,
+					     &et,
 					     handle,
 					     data_ac,
 					     meta_ac,
-					     &why,
-					     OCFS2_XATTR_VALUE_EXTENT,
-					     xv);
+					     &why);
 	if ((status < 0) && (status != -EAGAIN)) {
 		if (status != -ENOSPC)
 			mlog_errno(status);
@@ -292,7 +291,7 @@ restarted_transaction:
 			mlog(0, "restarting transaction.\n");
 			/* TODO: This can be more intelligent. */
 			credits = ocfs2_calc_extend_credits(osb->sb,
-							    root_el,
+							    et.et_root_el,
 							    clusters_to_add);
 			status = ocfs2_extend_trans(handle, credits);
 			if (status < 0) {
@@ -324,6 +323,7 @@ leave:
 		goto restart_all;
 	}
 
+	ocfs2_put_extent_tree(&et);
 	return status;
 }
 
@@ -339,11 +339,13 @@ static int __ocfs2_remove_xattr_range(struct inode *inode,
 	struct inode *tl_inode = osb->osb_tl_inode;
 	handle_t *handle;
 	struct ocfs2_alloc_context *meta_ac = NULL;
+	struct ocfs2_extent_tree et;
+
+	ocfs2_get_xattr_value_extent_tree(&et, inode, root_bh, xv);
 
-	ret = ocfs2_lock_allocators(inode, root_bh, &xv->xr_list,
-				    0, 1, NULL, &meta_ac,
-				    OCFS2_XATTR_VALUE_EXTENT, xv);
+	ret = ocfs2_lock_allocators(inode, &et, 0, 1, NULL, &meta_ac);
 	if (ret) {
+		ocfs2_put_extent_tree(&et);
 		mlog_errno(ret);
 		return ret;
 	}
@@ -372,8 +374,8 @@ static int __ocfs2_remove_xattr_range(struct inode *inode,
 		goto out_commit;
 	}
 
-	ret = ocfs2_remove_extent(inode, root_bh, cpos, len, handle, meta_ac,
-				  dealloc, OCFS2_XATTR_VALUE_EXTENT, xv);
+	ret = ocfs2_remove_extent(inode, &et, cpos, len, handle, meta_ac,
+				  dealloc);
 	if (ret) {
 		mlog_errno(ret);
 		goto out_commit;
@@ -399,6 +401,7 @@ out:
 	if (meta_ac)
 		ocfs2_free_alloc_context(meta_ac);
 
+	ocfs2_put_extent_tree(&et);
 	return ret;
 }
 
@@ -3638,26 +3641,24 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 	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_xattr_block *xb =
-			(struct ocfs2_xattr_block *)root_bh->b_data;
-	struct ocfs2_xattr_tree_root *xb_root = &xb->xb_attrs.xb_root;
-	struct ocfs2_extent_list *root_el = &xb_root->xt_list;
-	enum ocfs2_extent_tree_type type = OCFS2_XATTR_TREE_EXTENT;
+	struct ocfs2_extent_tree et;
 
 	mlog(0, "Add new xattr cluster for %llu, previous xattr hash = %u, "
 	     "previous xattr blkno = %llu\n",
 	     (unsigned long long)OCFS2_I(inode)->ip_blkno,
 	     prev_cpos, prev_blkno);
 
-	ret = ocfs2_lock_allocators(inode, root_bh, root_el,
-				    clusters_to_add, 0, &data_ac,
-				    &meta_ac, type, NULL);
+	ocfs2_get_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, root_el, clusters_to_add);
+	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);
@@ -3721,9 +3722,8 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 
 	mlog(0, "Insert %u clusters at block %llu for xattr at %u\n",
 	     num_bits, block, v_start);
-	ret = ocfs2_xattr_tree_insert_extent(osb, handle, inode, root_bh,
-					     v_start, block, num_bits,
-					     0, meta_ac);
+	ret = ocfs2_insert_extent(osb, handle, inode, &et, v_start, block,
+				  num_bits, 0, meta_ac);
 	if (ret < 0) {
 		mlog_errno(ret);
 		goto leave;
@@ -3743,6 +3743,7 @@ leave:
 	if (meta_ac)
 		ocfs2_free_alloc_context(meta_ac);
 
+	ocfs2_put_extent_tree(&et);
 	return ret;
 }
 
@@ -4347,9 +4348,11 @@ static int ocfs2_rm_xattr_cluster(struct inode *inode,
 	handle_t *handle;
 	struct ocfs2_xattr_block *xb =
 			(struct ocfs2_xattr_block *)root_bh->b_data;
-	struct ocfs2_extent_list *root_el = &xb->xb_attrs.xb_root.xt_list;
 	struct ocfs2_alloc_context *meta_ac = NULL;
 	struct ocfs2_cached_dealloc_ctxt dealloc;
+	struct ocfs2_extent_tree et;
+
+	ocfs2_get_xattr_tree_extent_tree(&et, inode, root_bh);
 
 	ocfs2_init_dealloc_ctxt(&dealloc);
 
@@ -4358,10 +4361,9 @@ static int ocfs2_rm_xattr_cluster(struct inode *inode,
 
 	ocfs2_remove_xattr_clusters_from_cache(inode, blkno, len);
 
-	ret = ocfs2_lock_allocators(inode, root_bh, root_el,
-				    0, 1, NULL, &meta_ac,
-				    OCFS2_XATTR_TREE_EXTENT, NULL);
+	ret = ocfs2_lock_allocators(inode, &et, 0, 1, NULL, &meta_ac);
 	if (ret) {
+		ocfs2_put_extent_tree(&et);
 		mlog_errno(ret);
 		return ret;
 	}
@@ -4390,8 +4392,8 @@ static int ocfs2_rm_xattr_cluster(struct inode *inode,
 		goto out_commit;
 	}
 
-	ret = ocfs2_remove_extent(inode, root_bh, cpos, len, handle, meta_ac,
-				  &dealloc, OCFS2_XATTR_TREE_EXTENT, NULL);
+	ret = ocfs2_remove_extent(inode, &et, cpos, len, handle, meta_ac,
+				  &dealloc);
 	if (ret) {
 		mlog_errno(ret);
 		goto out_commit;
@@ -4421,6 +4423,7 @@ out:
 
 	ocfs2_run_deallocs(osb, &dealloc);
 
+	ocfs2_put_extent_tree(&et);
 	return ret;
 }
 
-- 
1.5.6.3

[Ocfs2-devel] [PATCH 10/10] ocfs2: Make ocfs2_extent_tree the first-class representation of a tree.

[TaoMa]

Joel Becker wrote:
> We now have three different kinds of extent trees in ocfs2: inode data
> (dinode), extended attributes (xattr_tree), and extended attribute
> values (xattr_value).  There is a nice abstraction for them,
> ocfs2_extent_tree, but it is hidden in alloc.c.  All the calling
> functions have to pick amongst a varied API and pass in type bits and
> often extraneous pointers.
>
> A better way is to make ocfs2_extent_tree a first-class object.
> Everyone converts their object to an ocfs2_extent_tree() via the
> ocfs2_get_*_extent_tree() calls, then uses the ocfs2_extent_tree for all
> tree calls to alloc.c.
>
> This simplifies a lot of callers, making for readability.  It also
> provides an easy way to add additional extent tree types, as they only
> need to be defined in alloc.c with a ocfs2_get_<new>_extent_tree()
> function.
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
>  fs/ocfs2/alloc.c    |  300 +++++++++++++++-----------------------------------
>  fs/ocfs2/alloc.h    |  111 +++++++++++---------
>  fs/ocfs2/aops.c     |   16 ++-
>  fs/ocfs2/dir.c      |   20 ++--
>  fs/ocfs2/file.c     |   34 ++++---
>  fs/ocfs2/suballoc.c |   12 +--
>  fs/ocfs2/suballoc.h |    6 +-
>  fs/ocfs2/xattr.c    |   71 +++++++------
>  8 files changed, 238 insertions(+), 332 deletions(-)
>
>   
>  
> @@ -1236,10 +1241,11 @@ static int __ocfs2_remove_inode_range(struct inode *inode,
>  	handle_t *handle;
>  	struct ocfs2_alloc_context *meta_ac = NULL;
>  	struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data;
> +	struct ocfs2_extent_tree et;
> +
> +	ocfs2_get_dinode_extent_tree(&et, inode, di_bh);
>  
> -	ret = ocfs2_lock_allocators(inode, di_bh, &di->id2.i_list,
> -				    0, 1, NULL, &meta_ac,
> -				    OCFS2_DINODE_EXTENT, NULL);
> +	ret = ocfs2_lock_allocators(inode, &et, 0, 1, NULL, &meta_ac);
>  	if (ret) {
>  		mlog_errno(ret);
>  		return ret;
>   
here you need to ocfs2_put_dinoe_extent_tree and I see no put in the end 
of the function.
> @@ -1269,8 +1275,8 @@ static int __ocfs2_remove_inode_range(struct inode *inode,
>  		goto out;
>  	}
>  
> -	ret = ocfs2_remove_extent(inode, di_bh, cpos, len, handle, meta_ac,
> -				  dealloc, OCFS2_DINODE_EXTENT, NULL);
> +	ret = ocfs2_remove_extent(inode, &et, cpos, len, handle, meta_ac,
> +				  dealloc);
>  	if (ret) {
>  		mlog_errno(ret);
>  		goto out_commit;
>   
btw, I like your abstraction.

Regards,
Tao

[Ocfs2-devel] [PATCH 10/10] ocfs2: Make ocfs2_extent_tree the first-class representation of a tree.

[Joel Becker]

On Thu, Aug 21, 2008 at 03:46:36PM +0800, TaoMa wrote:
>>    @@ -1236,10 +1241,11 @@ static int __ocfs2_remove_inode_range(struct 
>> inode *inode,
>>  	handle_t *handle;
>>  	struct ocfs2_alloc_context *meta_ac = NULL;
>>  	struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data;
>> +	struct ocfs2_extent_tree et;
>> +
>> +	ocfs2_get_dinode_extent_tree(&et, inode, di_bh);
>>  -	ret = ocfs2_lock_allocators(inode, di_bh, &di->id2.i_list,
>> -				    0, 1, NULL, &meta_ac,
>> -				    OCFS2_DINODE_EXTENT, NULL);
>> +	ret = ocfs2_lock_allocators(inode, &et, 0, 1, NULL, &meta_ac);
>>  	if (ret) {
>>  		mlog_errno(ret);
>>  		return ret;
>>   
> here you need to ocfs2_put_dinoe_extent_tree and I see no put in the end  
> of the function.

	Good catch.  Here's the on-top diff:

diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 1f31a99..c49d2f3 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1247,6 +1247,7 @@ static int __ocfs2_remove_inode_range(struct inode *inode,
 
 	ret = ocfs2_lock_allocators(inode, &et, 0, 1, NULL, &meta_ac);
 	if (ret) {
+		ocfs2_put_extent_tree(&et);
 		mlog_errno(ret);
 		return ret;
 	}
@@ -1303,6 +1304,7 @@ out:
 	if (meta_ac)
 		ocfs2_free_alloc_context(meta_ac);
 
+	ocfs2_put_extent_tree(&et);
 	return ret;
 }
 
>> @@ -1269,8 +1275,8 @@ static int __ocfs2_remove_inode_range(struct inode *inode,
>>  		goto out;
>>  	}
>>  -	ret = ocfs2_remove_extent(inode, di_bh, cpos, len, handle, meta_ac,
>> -				  dealloc, OCFS2_DINODE_EXTENT, NULL);
>> +	ret = ocfs2_remove_extent(inode, &et, cpos, len, handle, meta_ac,
>> +				  dealloc);
>>  	if (ret) {
>>  		mlog_errno(ret);
>>  		goto out_commit;
>>   
> btw, I like your abstraction.

	It's your abstraction in the first place.  I just liked it so
much I promoted it to public view.  But thank you :-)

Joel

-- 

"What no boss of a programmer can ever understand is that a programmer
 is working when he's staring out of the window"
	- With apologies to Burton Rascoe

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

[Ocfs2-devel] [PATCH 0/10] Make ocfs2_extent_tree a first-class object

[TaoMa]

Joel Becker wrote:
> The new xattr code introduced a great abstraction, ocfs2_extent_tree.
> However, it's hidden in alloc.c.  Tao said he wanted to keep it there,
> but I think it works great as a first-class object.
>
> Callers into alloc.c can just fill in an ocfs2_extent_tree with the
> appropriate ocfs2_get_*_extent_tree() function, then pass it to all
> alloc.c functions.  Those functions no longer need individual root_bh,
> root_el, et_type, or void*private arguments.  People filling in dinode
> extent trees don't even need to pass NULL for that private.
>
> The series first adds structure prefixes to ocfs2_extent_tree and
> ocfs2_extent_tree_operations.  Next, it turns 'private' into 'object',
> calculating object as bh->b_data if it was NULL.  Third, we calculate
> the other fields (root_el, max_leaf_extents) in operations rather than
> if blocks.  Finally, we remove the et_type, the if block, and start
> using extent trees as first class objects.
>
> Check out the last patch, which is really the payoff, to see what I'm
> going for.
>   
wow, you did such a quick job. ;)
I just put my modification(which is generated from your review) in my 
local branch and wait for a final send, but you have already sent them 
out. Awesome!
Then I will just need to review them.;)

Regards,
Tao

[Ocfs2-devel] [PATCH 0/10] Make ocfs2_extent_tree a first-class object

[Joel Becker]

On Thu, Aug 21, 2008 at 11:45:56AM +0800, TaoMa wrote:
> Joel Becker wrote:
>> Check out the last patch, which is really the payoff, to see what I'm
>> going for.
>>   
> wow, you did such a quick job. ;)
> I just put my modification(which is generated from your review) in my  
> local branch and wait for a final send, but you have already sent them  
> out. Awesome!
> Then I will just need to review them.;)

	Note that this is very Request For Comment.  I want to know if
you and Mark like what I did here.

Joel

-- 

"Gone to plant a weeping willow
 On the bank's green edge it will roll, roll, roll.
 Sing a lulaby beside the waters.
 Lovers come and go, the river roll, roll, rolls."

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

[Ocfs2-devel] [PATCH 0/10] Make ocfs2_extent_tree a first-class object

[Mark Fasheh]

On Wed, Aug 20, 2008 at 09:28:51PM -0700, Joel Becker wrote:
> 	Note that this is very Request For Comment.  I want to know if
> you and Mark like what I did here.

I agree with Tao - the conceptual changes look great. Detailed review to
follow.
	--Mark

--
Mark Fasheh

[Ocfs2-devel] [PATCH 0/10] Make ocfs2_extent_tree a first-class object

[Joel Becker]

On Thu, Aug 21, 2008 at 11:45:56AM +0800, TaoMa wrote:
> Then I will just need to review them.;)

	Thanks for the fast review, btw!

Joel

-- 

Life's Little Instruction Book #197

	"Don't forget, a person's greatest emotional need is to 
	 feel appreciated."

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