[Ocfs2-devel] [PATCH 07/15] Add extent tree operation for xattr value.v3

[TaoMa]

Joel Becker wrote:
> On Fri, Aug 15, 2008 at 07:18:54PM -0700, Joel Becker wrote:
>   
>> 	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:
>>     
>
> <snip>
>
>   
>>> -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
>>     
>
> 	In fact, you don't need
> xattr_insert_extent/dinode_insert_extent.  Just have the callers of
> ocfs2_insert_extent create an extent tree (they're almost doing so
> already) and then pass it to ocfs2_insert_extent.
>   
Actually this is the consensus that Mark and I have made. See
http://oss.oracle.com/pipermail/ocfs2-devel/2008-June/002332.html
> 	Conversely, if you really like those wrappers, have them turn
> cpos/start_blk/new_clusters into an extent_rec before calling
> ocfs2_insert_extent.  It can be passed straight down into
> do_insert_extent.
>   
yes, this sounds reasonable since ocfs2_extent_rec will be created in 
ocfs2_insert_extent also. So we just move it to the 3 different callers.
Mark, agree?

Regards,
Tao