Make ocfs2_extent_tree a first-class object
cover Make ocfs2_extent_tree a first-class object
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
PATCH 1/10 ocfs2: Prefix the extent tree operations structure.
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 <[email protected]>
--- fs/ocfs2/alloc.c | 85 +++++++++++++++++++++++++++++------------------------ 1 files changed, 46 insertions(+), 39 deletions(-)
fs/ocfs2/alloc.c +46 -39
@@ -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);
Signature · 1 line
1.5.6.3
PATCH 1/10 ocfs2: Prefix the extent tree operations structure.
Quoted 7 lines · Joel Becker
The
ocfs2_extent_tree_operationsstructure 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 <[email protected]>
Signed-off-by Tao Ma <[email protected]>
PATCH 2/10 ocfs2: Prefix the ocfs2_extent_tree structure.
In reply to Joel BeckerThe members of the ocfs2_extent_tree structure gain a prefix of 'et_'. All users are updated.
Signed-off-by Joel Becker <[email protected]>
--- fs/ocfs2/alloc.c | 118 ++++++++++++++++++++++++++++-------------------------- 1 files changed, 61 insertions(+), 57 deletions(-)
fs/ocfs2/alloc.c +61 -57
@@ -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);
Signature · 1 line
1.5.6.3
PATCH 2/10 ocfs2: Prefix the ocfs2_extent_tree structure.
Quoted 5 lines · Joel Becker
The members of the
ocfs2_extent_treestructure gain a prefix of 'et_'. All users are updated.Signed-off-by Joel Becker <[email protected]>
Signed-off-by Tao Ma <[email protected]>
PATCH 3/10 ocfs2: Make ocfs2_extent_tree get/put instead of alloc.
In reply to Joel BeckerRather 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 <[email protected]>
--- fs/ocfs2/alloc.c | 117 ++++++++++++++++------------------------------------- 1 files changed, 36 insertions(+), 81 deletions(-)
fs/ocfs2/alloc.c +36 -81
@@ -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; }
Signature · 1 line
1.5.6.3
PATCH 3/10 ocfs2: Make ocfs2_extent_tree get/put instead of alloc.
Quoted 20 lines · Joel Becker
Rather than allocating a struct
ocfs2_extent_tree, just put it on the stack. Fill it withocfs2_get_extent_tree()and drop it withocfs2_put_extent_tree(). Now the callers don't have to ENOMEM, yet still safely ref theroot_bh.Signed-off-by Joel Becker <[email protected]>--- fs/ocfs2/alloc.c | 117 ++++++++++++++++------------------------------------- 1 files changed, 36 insertions(+), 81 deletions(-)fs/ocfs2/alloc.c +0
-0@@ -223,22 +223,17 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = { .eo_sanity_check = ocfs2_xattr_tree_sanity_check, };
fs/ocfs2/alloc.c static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = { · line 232
Quoted 10 lines
fs/ocfs2/alloc.c +2
-5-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?
Quoted 105 lines
fs/ocfs2/alloc.c +18
-39static 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;
fs/ocfs2/alloc.c int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh, · line 4892
Quoted 10 lines
fs/ocfs2/alloc.c +0
-7} - 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
PATCH 3/10 ocfs2: Make ocfs2_extent_tree get/put instead of alloc.
Quoted 15 lines · TaoMa
Quoted 14 lines · Joel Becker
Rather than allocating a struct
ocfs2_extent_tree, just put it on the stack. Fill it withocfs2_get_extent_tree()and drop it withocfs2_put_extent_tree(). Now the callers don't have to ENOMEM, yet still safely ref theroot_bh.Signed-off-by Joel Becker <[email protected]>fs/ocfs2/alloc.c +0
-0@@ -223,22 +223,17 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = { .eo_sanity_check = ocfs2_xattr_tree_sanity_check, };
Quoted 11 lines
fs/ocfs2/alloc.c static struct ocfs2_extent_tree* · line 261
Quoted 10 lines
fs/ocfs2/alloc.c +2
-4-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.
Quoted 20 lines
Quoted 20 lines
fs/ocfs2/alloc.c +2
-3@@ -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 ;
Quoted 8 lines
fs/ocfs2/alloc.c int ocfs2_mark_extent_written(struct inode *inode, struct buffer_head *root_bh,
Quoted 5 lines
fs/ocfs2/alloc.c +0
-4-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
Signature · 10 lines
"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
PATCH 3/10 ocfs2: Make ocfs2_extent_tree get/put instead of alloc.
In reply to Joel BeckerQuoted 18 lines · Joel Becker
Rather than allocating a struct
ocfs2_extent_tree, just put it on the stack. Fill it withocfs2_get_extent_tree()and drop it withocfs2_put_extent_tree(). Now the callers don't have to ENOMEM, yet still safely ref theroot_bh.Signed-off-by Joel Becker <[email protected]>--- fs/ocfs2/alloc.c | 117 ++++++++++++++++------------------------------------- 1 files changed, 36 insertions(+), 81 deletions(-)fs/ocfs2/alloc.c +0
-0@@ -223,22 +223,17 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = { .eo_sanity_check = ocfs2_xattr_tree_sanity_check, };
fs/ocfs2/alloc.c static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = { · line 230
Quoted 10 lines
fs/ocfs2/alloc.c +5
-5-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.
Quoted 17 lines
fs/ocfs2/alloc.c +1
-6{ -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 ) {fs/ocfs2/alloc.c +0
-0et->et_root_el =
fs/ocfs2/alloc.c static struct ocfs2_extent_tree* · line 254
Quoted 3 lines
fs/ocfs2/alloc.c +0
-0@@ -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
PATCH 3/10 ocfs2: Make ocfs2_extent_tree get/put instead of alloc.
Quoted 19 lines · Mark Fasheh
Quoted 18 lines · Joel Becker
Rather than allocating a struct
ocfs2_extent_tree, just put it on the stack. Fill it withocfs2_get_extent_tree()and drop it withocfs2_put_extent_tree(). Now the callers don't have to ENOMEM, yet still safely ref theroot_bh.Signed-off-by Joel Becker <[email protected]>--- fs/ocfs2/alloc.c | 117 ++++++++++++++++------------------------------------- 1 files changed, 36 insertions(+), 81 deletions(-)fs/ocfs2/alloc.c +0
-0@@ -223,22 +223,17 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = { .eo_sanity_check = ocfs2_xattr_tree_sanity_check, };
Quoted 13 lines
fs/ocfs2/alloc.c static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = { · line 230
Quoted 10 lines
fs/ocfs2/alloc.c +5
-5-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.
Quoted 9 lines
fs/ocfs2/alloc.c static struct ocfs2_extent_tree* · line 254
Quoted 4 lines
fs/ocfs2/alloc.c +0
-0@@ -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 ofet_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
Signature · 9 lines
"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-8127PATCH 3/10 ocfs2: Make ocfs2_extent_tree get/put instead of alloc.
Quoted 34 lines · Joel Becker
Quoted 19 lines · Mark Fasheh
Quoted 18 lines · Joel Becker
Rather than allocating a struct
ocfs2_extent_tree, just put it on the stack. Fill it withocfs2_get_extent_tree()and drop it withocfs2_put_extent_tree(). Now the callers don't have to ENOMEM, yet still safely ref theroot_bh.Signed-off-by Joel Becker <[email protected]>--- fs/ocfs2/alloc.c | 117 ++++++++++++++++------------------------------------- 1 files changed, 36 insertions(+), 81 deletions(-)fs/ocfs2/alloc.c +0
-0@@ -223,22 +223,17 @@ static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = { .eo_sanity_check = ocfs2_xattr_tree_sanity_check, };Quoted 13 lines
fs/ocfs2/alloc.c static struct ocfs2_extent_tree_operations ocfs2_xattr_tree_et_ops = { · line 230
Quoted 10 lines
fs/ocfs2/alloc.c +5
-5-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.
Quoted 3 lines
I originally had "fill", but I chose "get" because we
get_bh()
and thus require a "put" tobrelse(). I'm not tied to it, so if you like "init" I'll change it.
Nah, this is ok.
Quoted 10 lines
Quoted 9 lines
fs/ocfs2/alloc.c static struct ocfs2_extent_tree* · line 254
Quoted 4 lines
fs/ocfs2/alloc.c +0
-0@@ -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 ofet_root_el, etc. This won't fail as nice as the kzalloc would have before.
Quoted 3 lines
The final patch removes the
et_typecompletely. If you want, I
can have the intermediate patches do stricteret_typechecking (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
PATCH 3/10 ocfs2: Make ocfs2_extent_tree get/put instead of alloc.
Quoted 6 lines · Mark Fasheh
Quoted 5 lines · Joel Becker
Quoted 4 lines
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 ofet_root_el, etc. This won't fail as nice as the kzalloc would have before.
Quoted 6 lines
Quoted 3 lines
The final patch removes the
et_typecompletely. If you want, I
can have the intermediate patches do stricteret_typechecking (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 toeo_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
Signature · 8 lines
"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-8127PATCH 4/10 ocfs2: Make 'private' into 'object' on ocfs2_extent_tree.
In reply to Joel BeckerThe '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 <[email protected]>
--- fs/ocfs2/alloc.c | 62 +++++++++++++++++++++++++---------------------------- 1 files changed, 29 insertions(+), 33 deletions(-)
fs/ocfs2/alloc.c +29 -33
@@ -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 );
Signature · 1 line
1.5.6.3
PATCH 4/10 ocfs2: Make 'private' into 'object' on ocfs2_extent_tree.
Quoted 9 lines · 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 toocfs2_get_extent_tree(), 'object' is set tobh->b_data;Signed-off-by Joel Becker <[email protected]>
Signed-off-by Tao Ma <[email protected]>
PATCH 5/10 ocfs2: Provide the get_root_el() method to ocfs2_extent_tree_operations.
In reply to Joel BeckerThe 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 <[email protected]>
--- fs/ocfs2/alloc.c | 38 ++++++++++++++++++++++++++++++-------- 1 files changed, 30 insertions(+), 8 deletions(-)
fs/ocfs2/alloc.c +30 -8
@@ -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)
Signature · 1 line
1.5.6.3
PATCH 5/10 ocfs2: Provide the get_root_el() method to ocfs2_extent_tree_operations.
Quoted 10 lines · Joel Becker
The
root_elof anocfs2_extent_treeneeds to be calculated fromet->et_object. Make it an operation onet->et_ops.Signed-off-by Joel Becker <[email protected]>static void ocfs2_get_extent_tree(struct
ocfs2_extent_tree*et,fs/ocfs2/alloc.c +0
-0@@ -232,22 +260,16 @@ static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et, et->et_object = obj;
fs/ocfs2/alloc.c static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et, · line 266
Quoted 10 lines
fs/ocfs2/alloc.c +0
-5if (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
PATCH 5/10 ocfs2: Provide the get_root_el() method to ocfs2_extent_tree_operations.
Quoted 6 lines · TaoMa
Quoted 5 lines · Joel Becker
The
root_elof anocfs2_extent_treeneeds to be calculated fromet->et_object. Make it an operation onet->et_ops.Signed-off-by Joel Becker <[email protected]>
Quoted 15 lines
fs/ocfs2/alloc.c static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et, · line 265
Quoted 13 lines
static void ocfs2_get_extent_tree(struct
ocfs2_extent_tree*et,fs/ocfs2/alloc.c +0
-5@@ -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
Signature · 9 lines
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
PATCH 6/10 ocfs2: Use struct ocfs2_extent_tree in ocfs2_num_free_extents().
In reply to Joel Beckerocfs2_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 <[email protected]>
--- fs/ocfs2/alloc.c | 30 +++++------------------------- 1 files changed, 5 insertions(+), 25 deletions(-)
fs/ocfs2/alloc.c +5 -25
@@ -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; }
Signature · 1 line
1.5.6.3
PATCH 6/10 ocfs2: Use struct ocfs2_extent_tree in ocfs2_num_free_extents().
Quoted 41 lines · Joel Becker
ocfs2_num_free_extents()re-implements the logic ofocfs2_get_extent_tree(). Now thatocfs2_get_extent_tree()does not allocate, let's use it inocfs2_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 <[email protected]>--- fs/ocfs2/alloc.c | 30 +++++------------------------- 1 files changed, 5 insertions(+), 25 deletions(-)fs/ocfs2/alloc.c +1
-17@@ -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);
fs/ocfs2/alloc.c int ocfs2_num_free_extents(struct ocfs2_super *osb, · line 626
Quoted 10 lines
fs/ocfs2/alloc.c +1
-8- 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.
Quoted 7 lines
fs/ocfs2/alloc.c +2
-0+ el = et.et_root_el; + last_eb_blk =ocfs2_et_get_last_eb_blk (&et);if (last_eb_blk) {fs/ocfs2/alloc.c +0
-0retval = ocfs2_read_block(osb, last_eb_blk,
fs/ocfs2/alloc.c bail: · line 650
Quoted 7 lines
fs/ocfs2/alloc.c +1
-0@@ -665,6 +644,7 @@ bail:if (eb_bh)brelse (eb_bh); +ocfs2_put_extent_tree (&et);mlog_exit (retval);return retval; }
Regards,
Tao
PATCH 6/10 ocfs2: Use struct ocfs2_extent_tree in ocfs2_num_free_extents().
Quoted 44 lines · TaoMa
Quoted 43 lines · Joel Becker
ocfs2_num_free_extents()re-implements the logic ofocfs2_get_extent_tree(). Now thatocfs2_get_extent_tree()does not allocate, let's use it inocfs2_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 <[email protected]>--- fs/ocfs2/alloc.c | 30 +++++------------------------- 1 files changed, 5 insertions(+), 25 deletions(-)fs/ocfs2/alloc.c +1
-20@@ -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;
Quoted 8 lines
fs/ocfs2/alloc.c int ocfs2_num_free_extents(struct ocfs2_super *osb, · line 628
Quoted 6 lines
fs/ocfs2/alloc.c +1
-4- - 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_checkfirst may be better since there is a check above inOCFS2_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
Signature · 10 lines
"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
PATCH 6/10 ocfs2: Use struct ocfs2_extent_tree in ocfs2_num_free_extents().
Quoted 58 lines · Joel Becker
Quoted 46 lines · TaoMa
Quoted 44 lines · Joel Becker
ocfs2_num_free_extents()re-implements the logic ofocfs2_get_extent_tree(). Now thatocfs2_get_extent_tree()does not allocate, let's use it inocfs2_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 <[email protected]>--- fs/ocfs2/alloc.c | 30 +++++------------------------- 1 files changed, 5 insertions(+), 25 deletions(-)fs/ocfs2/alloc.c +1
-21@@ -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; -Quoted 9 lines
fs/ocfs2/alloc.c int ocfs2_num_free_extents(struct ocfs2_super *osb,
Quoted 6 lines
fs/ocfs2/alloc.c +1
-3- 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_checkfirst may be better since there is a check above inOCFS2_DINODE_EXTENT.
Quoted 4 lines
Every dinode caller has already checked the inode. I looked at
them before I removed theIS_VALID_INODE(). That's what I meant in the commit message.
OK.
Signed-off-by Tao Ma <[email protected]>
PATCH 7/10 ocfs2: Determine an extent tree's max_leaf_clusters in an et_op.
In reply to Joel BeckerProvide 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 <[email protected]>
--- fs/ocfs2/alloc.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
fs/ocfs2/alloc.c +15 -3
@@ -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)
Signature · 1 line
1.5.6.3
PATCH 7/10 ocfs2: Determine an extent tree's max_leaf_clusters in an et_op.
Quoted 58 lines · Joel Becker
Provide an optional
extent_tree_operationto specify themax_leaf_clustersof anocfs2_extent_tree. If not provided, the value is 0 (unlimited).Signed-off-by Joel Becker <[email protected]>--- fs/ocfs2/alloc.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)fs/ocfs2/alloc.c +11
-2@@ -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,
fs/ocfs2/alloc.c static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et, · line 286
Quoted 10 lines
fs/ocfs2/alloc.c +4
-1- 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
PATCH 7/10 ocfs2: Determine an extent tree's max_leaf_clusters in an et_op.
Quoted 5 lines · TaoMa
Quoted 4 lines · Joel Becker
Provide an optional
extent_tree_operationto specify themax_leaf_clustersof anocfs2_extent_tree. If not provided, the value is 0 (unlimited).
<snip>
Quoted 8 lines
Quoted 8 lines
fs/ocfs2/alloc.c +0
-2@@ -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);
Quoted 8 lines
fs/ocfs2/alloc.c static void ocfs2_get_extent_tree(struct ocfs2_extent_tree *et, · line 286
Quoted 6 lines
fs/ocfs2/alloc.c +4
-0+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_clustersfor 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
Signature · 9 lines
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
PATCH 7/10 ocfs2: Determine an extent tree's max_leaf_clusters in an et_op.
In reply to Joel BeckerQuoted 14 lines · Joel Becker
Provide an optional
extent_tree_operationto specify themax_leaf_clustersof anocfs2_extent_tree. If not provided, the value is 0 (unlimited).Signed-off-by Joel Becker <[email protected]>--- fs/ocfs2/alloc.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)fs/ocfs2/alloc.c +0
-0
fs/ocfs2/alloc.c struct ocfs2_extent_tree_operations { · line 78
Quoted 3 lines
fs/ocfs2/alloc.c +0
-0@@ -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
PATCH 7/10 ocfs2: Determine an extent tree's max_leaf_clusters in an et_op.
Quoted 10 lines · Mark Fasheh
fs/ocfs2/alloc.c struct ocfs2_extent_tree_operations { · line 78
Quoted 6 lines · Joel Becker
fs/ocfs2/alloc.c +0
-0@@ -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
Signature · 10 lines
"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
PATCH 7/10 ocfs2: Determine an extent tree's max_leaf_clusters in an et_op.
Quoted 12 lines · Joel Becker
Quoted 10 lines · Mark Fasheh
fs/ocfs2/alloc.c struct ocfs2_extent_tree_operations { · line 78
Quoted 6 lines · Joel Becker
fs/ocfs2/alloc.c +0
-0@@ -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.
Quoted 4 lines
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 <[email protected]>
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 <[email protected]>
--- fs/ocfs2/alloc.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 43 insertions(+), 2 deletions(-)
fs/ocfs2/alloc.c +43 -2
@@ -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); };
Signature · 14 lines
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
PATCH 8/10 ocfs2: Create specific get_extent_tree functions.
In reply to Joel BeckerA 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 <[email protected]>
--- fs/ocfs2/alloc.c | 76 +++++++++++++++++++++++++++++++++++++++--------------- fs/ocfs2/alloc.h | 2 +- 2 files changed, 56 insertions(+), 22 deletions(-)
fs/ocfs2/alloc.c +55 -21
@@ -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);
fs/ocfs2/alloc.h +1 -1
@@ -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,
Signature · 1 line
1.5.6.3
PATCH 8/10 ocfs2: Create specific get_extent_tree functions.
Quoted 17 lines · 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(), andocfs2_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 oldocfs2_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 couplealloc.cfunctions can takeet_type. This will go later.
Quoted 6 lines
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 <[email protected]>
Signed-off-by Tao Ma <[email protected]>
PATCH 9/10 ocfs2: Add an insertion check to ocfs2_extent_tree_operations.
In reply to Joel BeckerThe 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 <[email protected]>
--- fs/ocfs2/alloc.c | 88 +++++++++++++++++++++++++++++++++--------------------- 1 files changed, 54 insertions(+), 34 deletions(-)
fs/ocfs2/alloc.c +54 -34
@@ -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);
Signature · 1 line
1.5.6.3
PATCH 9/10 ocfs2: Add an insertion check to ocfs2_extent_tree_operations.
Quoted 4 lines · Joel Becker
The op
eo_sanity_check()was really a check onremove_right_path(). Let's call iteo_remove_check(). Let's add aneo_insert_check()that can be called fromocfs2_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
PATCH 9/10 ocfs2: Add an insertion check to ocfs2_extent_tree_operations.
Quoted 9 lines · TaoMa
Quoted 4 lines · Joel Becker
The op
eo_sanity_check()was really a check onremove_right_path(). Let's call iteo_remove_check(). Let's add aneo_insert_check()that can be called fromocfs2_insert_extent().
eo_sanity_checkis used to check inode, and I don't see there is any hint ofremove_right_pathin this function although it is now only used inocfs2_remove_rightmost_path. And I have said in the previous e-mail,it can be used inocfs2_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.
Quoted 2 lines
So I'd like to reserve the name and your add for
insert_checkis 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
Signature · 9 lines
"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-8127PATCH 9/10 ocfs2: Add an insertion check to ocfs2_extent_tree_operations.
In reply to Joel BeckerQuoted 159 lines · Joel Becker
The op
eo_sanity_check()was really a check onremove_right_path(). Let's call iteo_remove_check(). Let's add aneo_insert_check()that can be called fromocfs2_insert_extent().Let's have the wrapper calls
ocfs2_et_insert_check()andocfs2_et_remove_check()ignore NULL ops. That way we don't have to provide useless operations for xattr types.Signed-off-by Joel Becker <[email protected]>--- fs/ocfs2/alloc.c | 88 +++++++++++++++++++++++++++++++++--------------------- 1 files changed, 54 insertions(+), 34 deletions(-)fs/ocfs2/alloc.c +48
-27@@ -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",
fs/ocfs2/alloc.c static int ocfs2_insert_extent(struct ocfs2_super *osb, · line 4443
Quoted 10 lines
fs/ocfs2/alloc.c +1
-7- 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.
Quoted 10 lines
fs/ocfs2/alloc.c +5
-0+ 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);Signature · 1 line
1.5.6.3
--
Mark Fasheh
PATCH 9/10 ocfs2: Add an insertion check to ocfs2_extent_tree_operations.
Quoted 8 lines · Mark Fasheh
Quoted 7 lines · Joel Becker
The op
eo_sanity_check()was really a check onremove_right_path(). Let's call iteo_remove_check(). Let's add aneo_insert_check()that can be called fromocfs2_insert_extent().Let's have the wrapper calls
ocfs2_et_insert_check()andocfs2_et_remove_check()ignore NULL ops. That way we don't have to provide useless operations for xattr types.
<snip>
Quoted 26 lines
Quoted 26 lines
fs/ocfs2/alloc.c +4
-14@@ -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);
Quoted 8 lines
fs/ocfs2/alloc.c static int ocfs2_insert_extent(struct ocfs2_super *osb, · line 4443
Quoted 3 lines
fs/ocfs2/alloc.c +1
-0+/* 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
Signature · 9 lines
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
PATCH 9/10 ocfs2: Add an insertion check to ocfs2_extent_tree_operations.
Quoted 10 lines · Joel Becker
Quoted 4 lines
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
PATCH 10/10 ocfs2: Make ocfs2_extent_tree the first-class representation of a tree.
In reply to Joel BeckerWe 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 <[email protected]>
--- 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(-)
fs/ocfs2/alloc.c +90 -210
@@ -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;
fs/ocfs2/alloc.h +62 -49
140 lines not on this page · raw message
fs/ocfs2/aops.c +10 -6
@@ -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;
fs/ocfs2/dir.c +12 -8
@@ -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);
fs/ocfs2/file.c +20 -14
@@ -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;
fs/ocfs2/suballoc.c +5 -7
@@ -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);
fs/ocfs2/suballoc.h +2 -4
@@ -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_ */
fs/ocfs2/xattr.c +37 -34
185 lines not on this page · raw message
Signature · 1 line
1.5.6.3
PATCH 10/10 ocfs2: Make ocfs2_extent_tree the first-class representation of a tree.
Quoted 37 lines · 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 inalloc.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_treea first-class object. Everyone converts their object to anocfs2_extent_tree()via the ocfs2_get_*_extent_tree() calls, then uses theocfs2_extent_treefor all tree calls toalloc.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.cwith a ocfs2_get_<new>_extent_tree() function.Signed-off-by Joel Becker <[email protected]>--- 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(-)fs/ocfs2/file.c +1
-0@@ -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;
fs/ocfs2/file.c static int __ocfs2_remove_inode_range(struct inode *inode, · line 1251
Quoted 10 lines
fs/ocfs2/file.c +3
-3+ +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.
fs/ocfs2/file.c static int __ocfs2_remove_inode_range(struct inode *inode, · line 1282
Quoted 12 lines
fs/ocfs2/file.c +2
-2@@ -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
PATCH 10/10 ocfs2: Make ocfs2_extent_tree the first-class representation of a tree.
Quoted 10 lines · TaoMa
Quoted 10 lines
@@ -1236,10 +1241,11 @@ static int __ocfs2_remove_inode_range(structinode *inode,
(quoted diff) +3
-1handle_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,
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +1
-1- 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_treeand I see no put in the end of the function.
Good catch. Here's the on-top diff:
fs/ocfs2/file.c +2 -0
@@ -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; }
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0
-0@@ -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,
Quoted 8 lines
Review static int __ocfs2_remove_inode_range(struct inode *inode, · line 1282
Quoted 7 lines
(quoted diff) +2
-1- 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
Signature · 9 lines
"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
cover Make ocfs2_extent_tree a first-class object
In reply to Joel BeckerQuoted 17 lines · Joel Becker
The new xattr code introduced a great abstraction,
ocfs2_extent_tree. However, it's hidden inalloc.c. Tao said he wanted to keep it there, but I think it works great as a first-class object.Callers into
alloc.ccan just fill in anocfs2_extent_treewith the appropriate ocfs2_get_*_extent_tree() function, then pass it to allalloc.cfunctions. Those functions no longer need individualroot_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_treeandocfs2_extent_tree_operations. Next, it turns 'private' into 'object', calculating object asbh->b_dataif it was NULL. Third, we calculate the other fields (root_el,max_leaf_extents) in operations rather than if blocks. Finally, we remove theet_type, the if block, and start using extent trees as first class objects.
Quoted 3 lines
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
cover Make ocfs2_extent_tree a first-class object
Quoted 9 lines · TaoMa
Quoted 3 lines · Joel Becker
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
Signature · 10 lines
"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
cover Make ocfs2_extent_tree a first-class object
Quoted 2 lines · Joel Becker
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
cover Make ocfs2_extent_tree a first-class object
In reply to TaoMaQuoted 1 line · TaoMa
Then I will just need to review them.;)
Thanks for the fast review, btw!
Joel
Signature · 10 lines
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