On Thu, Aug 07, 2008 at 06:31:29AM +0800, Tao Ma wrote:
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -78,6 +78,7 @@ struct ocfs2_extent_tree {
> struct ocfs2_extent_tree_operations *eops;
> struct buffer_head *root_bh;
> struct ocfs2_extent_list *root_el;
> + void *private;
> };
Why 'private'? Why not 'object'? I don't like this if block:
> static struct ocfs2_extent_tree*
> ocfs2_new_extent_tree(struct buffer_head *bh,
> - enum ocfs2_extent_tree_type et_type)
> + enum ocfs2_extent_tree_type et_type,
> + void *private)
> {
> struct ocfs2_extent_tree *et;
>
> @@ -149,12 +191,16 @@ static struct ocfs2_extent_tree*
> et->type = et_type;
> get_bh(bh);
> et->root_bh = bh;
> + et->private = private;
>
> - /* current we only support dinode extent. */
> - BUG_ON(et->type != OCFS2_DINODE_EXTENT);
> if (et_type == OCFS2_DINODE_EXTENT) {
> et->root_el = &((struct ocfs2_dinode *)bh->b_data)->id2.i_list;
> et->eops = &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;
> }
Why don't you have an operation for getting the el? Really, the
way I see it, you should have:
void ocfs2_fill_extent_tree(*et, *bh, void *object, type)
{
et->et_type = type;
et->et_ops = extent_tree_ops[type];
get_bh(bh);
et->et_root_bh = bh;
et->et_object = object ? object : (void *)bh->b_data;
et->et_root_el = et->et_ops->et_get_root_el(et);
}
If you pass NULL for *object, it knows to use the start of the bh. This
works for dinode, etc. But you can pass a non-NULL object for the
xattr_value_root. The get_root_el operations know how to take et_object
and return the el:
ocfs2_dinode_get_root_el(*et)
{
struct ocfs2_dinode *di = et->et_object;
return &di->id2.i_list;
}
ocfs2_xattr_value_root_get_el(*et)
{
struct ocfs2_xattr_value_root *xv = et->et_object;
return &xv->xr_list;
}
> @@ -495,7 +541,8 @@ 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)
> + enum ocfs2_extent_tree_type type,
> + void *private)
> {
> int retval;
> struct ocfs2_extent_list *el = NULL;
> @@ -517,6 +564,12 @@ int ocfs2_num_free_extents(struct ocfs2_super *osb,
> 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 *) private;
> +
> + last_eb_blk = le64_to_cpu(xv->xr_last_eb_blk);
> + el = &xv->xr_list;
Again, just to avoid the extent_tree object, you are
hand-accessing the last_eb_block and el entries. I really think you
should be passing an *et into this function.
> -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,
> - enum ocfs2_extent_tree_type et_type)
> +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)
Here you are passing in the et, so you don't need root_bh
anymore, right
> @@ -4554,7 +4658,8 @@ int ocfs2_mark_extent_written(struct inode *inode,
> struct buffer_head *root_bh,
> 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)
> + enum ocfs2_extent_tree_type et_type,
> + void *private)
Here again you're passing root_bh, et_type, and private. Why
not just pass in the et?
> +int ocfs2_xattr_get_clusters(struct inode *inode, u32 v_cluster,
> + u32 *p_cluster, u32 *num_clusters,
> + struct ocfs2_extent_list *el)
> +{
> + int ret = 0, i;
> + struct buffer_head *eb_bh = NULL;
> + struct ocfs2_extent_block *eb;
> + struct ocfs2_extent_rec *rec;
> + u32 coff;
> +
> + if (el->l_tree_depth) {
> + ret = ocfs2_find_leaf(inode, el, v_cluster, &eb_bh);
> + if (ret) {
> + mlog_errno(ret);
> + goto out;
> + }
> +
> + eb = (struct ocfs2_extent_block *) eb_bh->b_data;
> + el = &eb->h_list;
> +
> + if (el->l_tree_depth) {
> + ocfs2_error(inode->i_sb,
> + "Inode %lu has non zero tree depth in "
> + "xattr leaf block %llu\n", inode->i_ino,
> + (unsigned long long)eb_bh->b_blocknr);
> + ret = -EROFS;
> + goto out;
> + }
> + }
> +
> + i = ocfs2_search_extent_list(el, v_cluster);
> + if (i == -1) {
> + ret = -EROFS;
> + mlog_errno(ret);
> + goto out;
> + } else {
> + rec = &el->l_recs[i];
> + BUG_ON(v_cluster < le32_to_cpu(rec->e_cpos));
> +
> + if (!rec->e_blkno) {
> + ocfs2_error(inode->i_sb, "Inode %lu has bad extent "
> + "record (%u, %u, 0) in xattr", inode->i_ino,
> + le32_to_cpu(rec->e_cpos),
> + ocfs2_rec_clusters(el, rec));
> + ret = -EROFS;
> + goto out;
> + }
> + coff = v_cluster - le32_to_cpu(rec->e_cpos);
> + *p_cluster = ocfs2_blocks_to_clusters(inode->i_sb,
> + le64_to_cpu(rec->e_blkno));
> + *p_cluster = *p_cluster + coff;
> + if (num_clusters)
> + *num_clusters = ocfs2_rec_clusters(el, rec) - coff;
> + }
> +out:
> + if (eb_bh)
> + brelse(eb_bh);
> + return ret;
> +}
Part of me wonders if you could have refactored
ocfs2_get_clusters to share most of this duplicated code. But I do see
the difference, and we can always come back to it.
> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> index d82a9a0..00d0fff 100644
> --- a/fs/ocfs2/suballoc.c
> +++ b/fs/ocfs2/suballoc.c
> @@ -1906,7 +1906,8 @@ int ocfs2_lock_allocators(struct inode *inode, struct
> buffer_head *root_bh,
> struct ocfs2_extent_list *root_el,
> u32 clusters_to_add, u32 extents_to_split,
> struct ocfs2_alloc_context **data_ac,
> - struct ocfs2_alloc_context **meta_ac)
> + struct ocfs2_alloc_context **meta_ac,
> + enum ocfs2_extent_tree_type type, void *private)
Pass in a struct ocfs2_extent_list and you don't have to pass
root_bh. root_el, type, or private.
> @@ -1992,7 +1993,8 @@ int ocfs2_add_clusters_in_btree(struct ocfs2_super *osb,
> struct ocfs2_alloc_context *data_ac,
> struct ocfs2_alloc_context *meta_ac,
> enum ocfs2_alloc_restarted *reason_ret,
> - enum ocfs2_extent_tree_type type)
> + enum ocfs2_extent_tree_type type,
> + void *private)
Same here. I'll stop repeating this.
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> new file mode 100644
> index 0000000..9604a4c
> --- /dev/null
> +++ b/fs/ocfs2/xattr.c
This file I like :-) Pretty straightforward and clean.
Joel
--
"Too much walking shoes worn thin.
Too much trippin' and my soul's worn thin.
Time to catch a ride it leaves today
Her name is what it means.
Too much walking shoes worn thin."
Joel Becker
Principal Software Developer
Oracle
E-mail: [EMAIL PROTECTED]
Phone: (650) 506-8127
_______________________________________________
Ocfs2-devel mailing list
[email protected]
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
