[Ocfs2-tools-devel] [PATCH 2/2] libocfs2: extend xattr and xattr value tree

[Tao Ma]

Hi Tiger,
	I just played around with your patches to implement refcount support in 
ocfs2-tools and found some strange things.

Tiger Yang wrote:
> Add ocfs2_tree_extend_allocation, ocfs2_tree_insert_extent
> and ocfs2_tree_find_leaf for extending xattr record and
> xattr value tree.
> On the other hand, remain ocfs2_extend_allocation,
> ocfs2_inode_insert_extent and ocfs2_find_leaf for
> extending file data tree in inode.
> 
> Signed-off-by: Tiger Yang <tiger.yang at oracle.com>
<snip>
> +errcode_t ocfs2_tree_insert_extent(ocfs2_cached_inode *ci,
> +				   uint32_t cpos, uint64_t c_blkno,
> +				   uint32_t clusters, uint16_t flag,
> +				   struct ocfs2_extent_tree *et)
> +{
> +	errcode_t ret = 0;
> +	struct insert_ctxt ctxt;
> +	struct ocfs2_insert_type insert = {0, };
> +	char *last_eb = NULL;
> +	char *di_buf = NULL;
> +	ocfs2_filesys *fs = ci->ci_fs;
> +	struct ocfs2_extent_list *el = et->et_root_el;
> +	struct ocfs2_path *path = NULL;
> +
> +	ctxt.fs = fs;
> +	ctxt.di = ci->ci_inode;
> +	di_buf = (char *)ctxt.di;
As you said above, now tree insert is used to insert something into a 
generic extent tree. So why we still need to pass in ci and di? I knew 
that the old insert_ctxt has a member named "di", and you probably want 
ot init it here. But since we have already changed it to 
ocfs2_extent_tree, I guess we should change "di" to extent_tree also.
> +
> +	memset(&ctxt.rec, 0, sizeof(struct ocfs2_extent_rec));
> +	ctxt.rec.e_cpos = cpos;
> +	ctxt.rec.e_blkno = c_blkno;
> +	ctxt.rec.e_leaf_clusters = clusters;
> +	ctxt.rec.e_flags = flag;
> +
> +	insert.ins_split = SPLIT_NONE;
> +	insert.ins_tree_depth = el->l_tree_depth;
> +
> +	if (!el->l_tree_depth) {
> +		ocfs2_figure_contig_type(fs, &insert, el , &ctxt.rec);
> +		ocfs2_figure_appending_type(&insert, el, &ctxt.rec);
> +	} else {
> +		/*
> +		 * If we have tree depth, we read in the
> +		 * rightmost extent block ahead of time as
> +		 * ocfs2_figure_insert_type() and ocfs2_add_branch()
> +		 * may want it later.
> +		 */
> +		uint64_t last_eb_blk = et->et_ops->eo_get_last_eb_blk(et);
> +
> +		ret = ocfs2_malloc_block(fs->fs_io, &last_eb);
> +		if (ret)
> +			return ret;
> +		ret = ocfs2_read_extent_block(fs, last_eb_blk, last_eb);
> +		if (ret)
> +			goto bail;
> +		el = &((struct ocfs2_extent_block *)last_eb)->h_list;
> +
> +		path = ocfs2_new_tree_path(el, last_eb_blk, last_eb);
> +		if (!path) {
> +			ret = OCFS2_ET_NO_MEMORY;
> +			goto bail;
> +		}
> +		ret = ocfs2_figure_insert_type(&ctxt, path, last_eb_blk,
> +						&insert);
> +		if (ret)
> +			goto free_path;
> +	}
> +
> +	if (insert.ins_contig == CONTIG_NONE &&
> +	    el->l_count == el->l_next_free_rec) {
> +		ret = ocfs2_grow_tree(fs, et, &insert.ins_tree_depth,
> +				      &last_eb);
> +		if (ret)
> +			goto free_path;
> +	}
> +
> +	if (el->l_tree_depth == 0) {
> +		ocfs2_insert_at_leaf(fs, &ctxt.rec, el, &insert);
> +		goto out_update_clusters;
> +	}
> +
> +	/* Finally, we can add clusters. This might rotate the tree for us. */
> +	ret = __ocfs2_do_insert_extent(&ctxt, &insert, el, path);
hey, I just see this. You didn't pass the et into the most important 
b-tree function. So how could we use et? As I said above, we need to 
pass et in ctxt so that the internal b-tree fucntions can use it.

btw, I also checked other functions, and it looks that some functions in 
b-tree operations didn't use extent_tree. See 
ocfs2_remove_rightmost_path for example, it just treat the root of a 
path to be a inode, but it isn't actually. So please double check 
whether all the b-tree functions have been changed to work for a 
extent_tree not the inode.

Regards,
Tao