Ocfs2: Change ocfs2_remove_btree_range() a bit to consider refcount case.

As ocfs2_prepare_refcount_change_for_del() only called from ocfs2_commit_truncate for now(punching holes are going to use that soon I guess), we're safe to defer metadata blocks reservation unitl ocfs2_remove_btree_range(), who is also reserving blocks for extent btree adjusting, so it will be nice to do these in one go.

Instead, it will return the blocks calculated to caller, to let him know how many extra blocks for refcount tree should be reserved next.

Signed-off-by Tristan Ye <[email protected]>

---
 fs/ocfs2/refcounttree.c |   21 +++++++--------------
 fs/ocfs2/refcounttree.h |    2 +-
 2 files changed, 8 insertions(+), 15 deletions(-)

fs/ocfs2/refcounttree.c +7 -14

index 60287fc..42a8446 100644
@@ -2432,8 +2432,8 @@ out:
  *
  * Normally the refcount blocks store these refcount should be
  * continguous also, so that we can get the number easily.
- * As for meta_ac, we will at most add split 2 refcount record and
- * 2 more refcount block, so just check it in a rough way.
+ * We will at most add split 2 refcount records and 2 more
+ * refcount block, so just check it in a rough way.
  *
  * Caller must hold refcount tree lock.
  */

@@ -2442,9 +2442,9 @@ int ocfs2_prepare_refcount_change_for_del(struct inode *inode,
 					  u64 phys_blkno,
 					  u32 clusters,
 					  int *credits,
-					  struct ocfs2_alloc_context **meta_ac)
+					  int *ref_blocks)
 {
-	int ret, ref_blocks = 0;
+	int ret;
 	struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data;
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
 	struct buffer_head *ref_root_bh = NULL;

@@ -2480,21 +2480,14 @@ int ocfs2_prepare_refcount_change_for_del(struct inode *inode,
 					       &tree->rf_ci,
 					       ref_root_bh,
 					       start_cpos, clusters,
-					       &ref_blocks, credits);
+					       ref_blocks, credits);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
 
-	mlog(0, "reserve new metadata %d, credits = %d\n",
-	     ref_blocks, *credits);
-
-	if (ref_blocks) {
-		ret = ocfs2_reserve_new_metadata_blocks(OCFS2_SB(inode->i_sb),
-							ref_blocks, meta_ac);
-		if (ret)
-			mlog_errno(ret);
-	}
+	mlog(0, "reserve new metadata %d blocks, credits = %d\n",
+	     *ref_blocks, *credits);
 
 out:
 	brelse(ref_root_bh);

fs/ocfs2/refcounttree.h +1 -1

index c1d19b1..f4bf110 100644
@@ -51,7 +51,7 @@ int ocfs2_prepare_refcount_change_for_del(struct inode *inode,
 					  u64 phys_blkno,
 					  u32 clusters,
 					  int *credits,
-					  struct ocfs2_alloc_context **meta_ac);
+					  int *ref_blocks);
 int ocfs2_refcount_cow(struct inode *inode, struct buffer_head *di_bh,
 		       u32 cpos, u32 write_len, u32 max_cpos);
 

Hi Tristan,
you'd better integrate this patch with patch 3/3. otherwise we
can't build successfully if we apply this one and don't apply the next one(in some git bisect case).

Regards,
Tao

Quoted 87 lines · Tristan Ye

As ocfs2_prepare_refcount_change_for_del() only called from ocfs2_commit_truncate for
now(punching holes are going to use that soon I guess), we're safe to defer metadata
blocks reservation unitl ocfs2_remove_btree_range(), who is also reserving blocks for
extent btree adjusting, so it will be nice to do these in one go.

Instead, it will return the blocks calculated to caller, to let him know how many extra
blocks for refcount tree should be reserved next.

Signed-off-by Tristan Ye <[email protected]>

---
 fs/ocfs2/refcounttree.c |   21 +++++++--------------
 fs/ocfs2/refcounttree.h |    2 +-
 2 files changed, 8 insertions(+), 15 deletions(-)

fs/ocfs2/refcounttree.c +4 -4

index 60287fc..42a8446 100644
@@ -2432,8 +2432,8 @@ out:
  *
  * Normally the refcount blocks store these refcount should be
  * continguous also, so that we can get the number easily.
- * As for meta_ac, we will at most add split 2 refcount record and
- * 2 more refcount block, so just check it in a rough way.
+ * We will at most add split 2 refcount records and 2 more
+ * refcount block, so just check it in a rough way.
  *
  * Caller must hold refcount tree lock.
  */

@@ -2442,9 +2442,9 @@ int 
 ocfs2_prepare_refcount_change_for_del(struct inode *inode,
                       u64 phys_blkno,
                       u32 clusters,
                       int *credits,
-                      struct ocfs2_alloc_context **meta_ac)
+                      int *ref_blocks)
 {
-    int ret, ref_blocks = 0;
+    int ret;
     struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data;
     struct ocfs2_inode_info *oi = OCFS2_I(inode);

     struct buffer_head *ref_root_bh = NULL;

fs/ocfs2/refcounttree.c +3 -10

@@ -2480,21 +2480,14 @@ int 
 ocfs2_prepare_refcount_change_for_del(struct inode *inode,
                            &tree->rf_ci,
                            ref_root_bh,
                            start_cpos, clusters,
-                           &ref_blocks, credits);
+                           ref_blocks, credits);
     if (ret) {
         mlog_errno(ret);
         goto out;
     }
 
-    mlog(0, "reserve new metadata %d, credits = %d\n",
-         ref_blocks, *credits);
-
-    if (ref_blocks) {
-        ret = ocfs2_reserve_new_metadata_blocks(OCFS2_SB(inode->i_sb),
-                            ref_blocks, meta_ac);
-        if (ret)
-            mlog_errno(ret);
-    }
+    mlog(0, "reserve new metadata %d blocks, credits = %d\n",
+         *ref_blocks, *credits);
 
 out:

     brelse(ref_root_bh);

fs/ocfs2/refcounttree.h +1 -1

index c1d19b1..f4bf110 100644
@@ -51,7 +51,7 @@ int ocfs2_prepare_refcount_change_for_del(struct 
 inode *inode,
                       u64 phys_blkno,
                       u32 clusters,
                       int *credits,
-                      struct ocfs2_alloc_context **meta_ac);
+                      int *ref_blocks);
 int ocfs2_refcount_cow(struct inode *inode, struct buffer_head *di_bh,
                u32 cpos, u32 write_len, u32 max_cpos);

Quoted 4 lines · TaoMa

Hi Tristan,
you'd better integrate this patch with patch 3/3. otherwise we
can't build successfully if we apply this one and don't apply the next one(in some git bisect case).

Yes I see, here I just split the patches as small as possible to be logically separate, for the convenience of your reviewing:)

Quoted 10 lines · tristan

Quoted 5 lines · TaoMa

Hi Tristan,
you'd better integrate this patch with patch 3/3. otherwise we
can't build successfully if we apply this one and don't apply the next one(in some git bisect case).

Yes I see, here I just split the patches as small as possible to be logically separate, for the convenience of your reviewing:)

Tristan,
Your instinct is good, but Tao is right, every commit has to
compile and work. The next patches you work on, keep trying to break it up into logical things, but have working code. One of the best tricks is doing the isomorphic changes (changes that don't actually affect the behavior yet) first. I use this trick a lot. I can add an argument to a function or change what is passed through without actually changing the functionality. Once I've used this methodology to restructure the code, then I can change the behavior.

For this particular change, I'm OK with it being one patch as I
stated in my other email.

Joel

As we known, truncate is just a special case of punching holes(from new i_size to end), we therefore could take advantage of existing ocfs2_remove_btree_range() codes to reduce the comlexity and redundancy in alloc.c, the goal here is to make truncate codes more generic and straightforward.

Several former functions only used by ocfs2_commit_truncate() will be simply wiped off. New logic for truncating will remove extents from truncate_size to file end one by one, just like punching holes did.

v2 patch uses former sequence of records truncating(from tail to new_i_size) which keeps a high efficiency in performance, also, it has the refcount support as well.

Signed-off-by Tristan Ye <[email protected]>

---
 fs/ocfs2/alloc.c |  556 +++++------------------------------------------------
 fs/ocfs2/alloc.h |    3 +-
 fs/ocfs2/file.c  |    9 +-
 fs/ocfs2/inode.c |    9 +-
 4 files changed, 56 insertions(+), 521 deletions(-)

fs/ocfs2/alloc.c +11 -7

index 6c5f1de..0c42003 100644
@@ -7422,21 +6999,25 @@ out:
  */
 int ocfs2_commit_truncate(struct ocfs2_super *osb,
 			  struct inode *inode,
-			  struct buffer_head *fe_bh,
-			  struct ocfs2_truncate_context *tc)
+			  struct buffer_head *fe_bh)
 {
-	int status, i, credits, tl_sem = 0;
-	u32 clusters_to_del, new_highest_cpos, range;
+	int status = 0, i, credits = 0, ref_blocks = 0, flags = 0;
+	u32 new_highest_cpos, range, trunc_cpos, trunc_len, phys_cpos, coff;
 	u64 blkno = 0;
 	struct ocfs2_extent_list *el;
-	handle_t *handle = NULL;
-	struct inode *tl_inode = osb->osb_tl_inode;
+	struct ocfs2_extent_rec *rec;
 	struct ocfs2_path *path = NULL;
 	struct ocfs2_dinode *di = (struct ocfs2_dinode *)fe_bh->b_data;
-	struct ocfs2_alloc_context *meta_ac = NULL;
+
+	struct ocfs2_extent_tree et;
+	struct ocfs2_cached_dealloc_ctxt dealloc;
+
 	struct ocfs2_refcount_tree *ref_tree = NULL;
 
 	mlog_entry_void();
+	
+	ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), fe_bh);
+	ocfs2_init_dealloc_ctxt(&dealloc);
 
 	new_highest_cpos = ocfs2_clusters_for_bytes(osb->sb,
 						     i_size_read(inode));

fs/ocfs2/alloc.c +0 -0

 start:
 	/*

fs/ocfs2/alloc.c +21 -14

 	 * Check that we still have allocation to delete.

@@ -7461,6 +7040,7 @@ start:
 	}
 
 	credits = 0;
+	ref_blocks = 0;
 
 	/*
 	 * Truncate always works against the rightmost tree branch.

@@ -7496,30 +7076,47 @@ start:
 	}
 
 	i = le16_to_cpu(el->l_next_free_rec) - 1;
-	range = le32_to_cpu(el->l_recs[i].e_cpos) +
-		ocfs2_rec_clusters(el, &el->l_recs[i]);
-	if (i == 0 && ocfs2_is_empty_extent(&el->l_recs[i])) {
-		clusters_to_del = 0;
-	} else if (le32_to_cpu(el->l_recs[i].e_cpos) >= new_highest_cpos) {
-		clusters_to_del = ocfs2_rec_clusters(el, &el->l_recs[i]);
-		blkno = le64_to_cpu(el->l_recs[i].e_blkno);
+	rec = &el->l_recs[i];
+	flags = rec->e_flags;
+	range = le32_to_cpu(rec->e_cpos) + ocfs2_rec_clusters(el, rec);
+
+	if (i == 0 && ocfs2_is_empty_extent(rec)) {
+		/*
+		 * Should remove this extent block.
+		 */
+		trunc_cpos = le32_to_cpu(rec->e_cpos);
+		trunc_len = 0;
+		coff = 0;
+		blkno = 0;
+	} else if (le32_to_cpu(rec->e_cpos) >= new_highest_cpos) {
+		/* 
+		 * Truncate entire record.
+		 */
+		trunc_cpos = le32_to_cpu(rec->e_cpos);
+		trunc_len = ocfs2_rec_clusters(el, rec);
+		coff = 0;
+		blkno = le64_to_cpu(rec->e_blkno);
 	} else if (range > new_highest_cpos) {
-		clusters_to_del = (ocfs2_rec_clusters(el, &el->l_recs[i]) +
-				   le32_to_cpu(el->l_recs[i].e_cpos)) -
-				  new_highest_cpos;
-		blkno = le64_to_cpu(el->l_recs[i].e_blkno) +
-			ocfs2_clusters_to_blocks(inode->i_sb,
-				ocfs2_rec_clusters(el, &el->l_recs[i]) -
-				clusters_to_del);

fs/ocfs2/alloc.c +5 -3

 	} else {
+		/*
+		 * Truncate completed, leave happily.
+		 */
 		status = 0;
 		goto bail;
 	}
 
-	mlog(0, "clusters_to_del = %u in this pass, tail blk=%llu\n",
-	     clusters_to_del, (unsigned long long)path_leaf_bh(path)->b_blocknr);
+	phys_cpos = ocfs2_blocks_to_clusters(inode->i_sb, blkno);
 
-	if (el->l_recs[i].e_flags & OCFS2_EXT_REFCOUNTED && clusters_to_del) {
+	if (el->l_recs[i].e_flags & OCFS2_EXT_REFCOUNTED && trunc_len) {
 		BUG_ON(!(OCFS2_I(inode)->ip_dyn_features &
 			 OCFS2_HAS_REFCOUNT_FL));

fs/ocfs2/alloc.c +5 -38

-		if (status < 0) {
-			mlog_errno(status);
-			goto bail;
-		}
-	}
-
-	mutex_lock(&tl_inode->i_mutex);
-	tl_sem = 1;
-	/* ocfs2_truncate_log_needs_flush guarantees us at least one
-	 * record is free for use. If there isn't any, we flush to get
-	 * an empty truncate log.  */
-	if (ocfs2_truncate_log_needs_flush(osb)) {
-		status = __ocfs2_flush_truncate_log(osb);
+							       &ref_blocks);
 		if (status < 0) {
 			mlog_errno(status);
 			goto bail;
 		}
 	}
 
-	credits += ocfs2_calc_tree_trunc_credits(osb->sb, clusters_to_del,
-						(struct ocfs2_dinode *)fe_bh->b_data,
-						el);
-	handle = ocfs2_start_trans(osb, credits);
-	if (IS_ERR(handle)) {
-		status = PTR_ERR(handle);
-		handle = NULL;
-		mlog_errno(status);
-		goto bail;
-	}
-
-	status = ocfs2_do_truncate(osb, clusters_to_del, inode, fe_bh, handle,
-				   tc, path, meta_ac);
+	status = ocfs2_remove_btree_range(inode, &et, trunc_cpos, phys_cpos,
+					  trunc_len, &dealloc, credits,
+					  ref_blocks, flags);
 	if (status < 0) {
 		mlog_errno(status);
 		goto bail;
 	}
-
-	mutex_unlock(&tl_inode->i_mutex);
-	tl_sem = 0;
-
-	ocfs2_commit_trans(osb, handle);
-	handle = NULL;
-
+	
 	ocfs2_reinit_path(path, 1);
 
-	if (meta_ac) {
-		ocfs2_free_alloc_context(meta_ac);
-		meta_ac = NULL;
-	}
-
 	if (ref_tree) {
 		ocfs2_unlock_refcount_tree(osb, ref_tree, 1);

fs/ocfs2/alloc.c +0 -10

 		ref_tree = NULL;

@@ -7598,28 +7161,15 @@ start:
 	goto start;
 
 bail:
-
 	ocfs2_schedule_truncate_log_flush(osb, 1);
 
-	if (tl_sem)
-		mutex_unlock(&tl_inode->i_mutex);
-
-	if (handle)
-		ocfs2_commit_trans(osb, handle);
-
-	if (meta_ac)
-		ocfs2_free_alloc_context(meta_ac);
-
 	if (ref_tree)
 		ocfs2_unlock_refcount_tree(osb, ref_tree, 1);

Quoted 70 lines · Tristan Ye

As we known, truncate is just a special case of punching holes(from new i_size
to end), we therefore could take advantage of existing ocfs2_remove_btree_range()
codes to reduce the comlexity and redundancy in alloc.c, the goal here is to make
truncate codes more generic and straightforward.

Several former functions only used by ocfs2_commit_truncate() will be simply wiped off.
New logic for truncating will remove extents from truncate_size to file end one by one,
just like punching holes did.

v2 patch uses former sequence of records truncating(from tail to new_i_size) which keeps
a high efficiency in performance, also, it has the refcount support as well.

Signed-off-by Tristan Ye <[email protected]>

---
fs/ocfs2/alloc.c | 556
+++++------------------------------------------------
fs/ocfs2/alloc.h | 3 +-
fs/ocfs2/file.c | 9 +-
fs/ocfs2/inode.c | 9 +-

4 files changed, 56 insertions(+), 521 deletions(-)

fs/ocfs2/alloc.c +11 -7

index 6c5f1de..0c42003 100644
@@ -7422,21 +6999,25 @@ out:
 */
 int ocfs2_commit_truncate(struct ocfs2_super *osb,
 struct inode *inode,
- struct buffer_head *fe_bh,
- struct ocfs2_truncate_context *tc)
+ struct buffer_head *fe_bh)
 {
- int status, i, credits, tl_sem = 0;
- u32 clusters_to_del, new_highest_cpos, range;
+ int status = 0, i, credits = 0, ref_blocks = 0, flags = 0;
+ u32 new_highest_cpos, range, trunc_cpos, trunc_len, phys_cpos, coff;
 u64 blkno = 0;
 struct ocfs2_extent_list *el;
- handle_t *handle = NULL;
- struct inode *tl_inode = osb->osb_tl_inode;
+ struct ocfs2_extent_rec *rec;
 struct ocfs2_path *path = NULL;
 struct ocfs2_dinode *di = (struct ocfs2_dinode *)fe_bh->b_data;
- struct ocfs2_alloc_context *meta_ac = NULL;
+
+ struct ocfs2_extent_tree et;
+ struct ocfs2_cached_dealloc_ctxt dealloc;
+
 struct ocfs2_refcount_tree *ref_tree = NULL;
 
 mlog_entry_void();
+
+ ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), fe_bh);
+ ocfs2_init_dealloc_ctxt(&dealloc);
 
 new_highest_cpos = ocfs2_clusters_for_bytes(osb->sb,
 i_size_read(inode));

@@ -7449,8 +7030,6 @@ int ocfs2_commit_truncate(struct ocfs2_super *osb,
 goto bail;
 }
 

fs/ocfs2/alloc.c int ocfs2_commit_truncate(struct ocfs2_super *osb, · line 7453

Quoted 2 lines

fs/ocfs2/alloc.c +0 -2

- ocfs2_extent_map_trunc(inode, new_highest_cpos);
-

Please keep this line. It will truncate the extent map.

Quoted 52 lines

start:
/*
* Check that we still have allocation to delete.

fs/ocfs2/alloc.c +23 -14

@@ -7461,6 +7040,7 @@ start:
 }
 
 credits = 0;
+ ref_blocks = 0;
 
 /*
 * Truncate always works against the rightmost tree branch.

@@ -7496,30 +7076,47 @@ start:
 }
 
 i = le16_to_cpu(el->l_next_free_rec) - 1;
- range = le32_to_cpu(el->l_recs[i].e_cpos) +
- ocfs2_rec_clusters(el, &el->l_recs[i]);
- if (i == 0 && ocfs2_is_empty_extent(&el->l_recs[i])) {
- clusters_to_del = 0;
- } else if (le32_to_cpu(el->l_recs[i].e_cpos) >= new_highest_cpos) {
- clusters_to_del = ocfs2_rec_clusters(el, &el->l_recs[i]);
- blkno = le64_to_cpu(el->l_recs[i].e_blkno);
+ rec = &el->l_recs[i];
+ flags = rec->e_flags;
+ range = le32_to_cpu(rec->e_cpos) + ocfs2_rec_clusters(el, rec);
+
+ if (i == 0 && ocfs2_is_empty_extent(rec)) {
+ /*
+ * Should remove this extent block.
+ */
+ trunc_cpos = le32_to_cpu(rec->e_cpos);
+ trunc_len = 0;
+ coff = 0;
+ blkno = 0;
+ } else if (le32_to_cpu(rec->e_cpos) >= new_highest_cpos) {
+ /* + * Truncate entire record.
+ */
+ trunc_cpos = le32_to_cpu(rec->e_cpos);
+ trunc_len = ocfs2_rec_clusters(el, rec);
+ coff = 0;
+ blkno = le64_to_cpu(rec->e_blkno);
 } else if (range > new_highest_cpos) {
- clusters_to_del = (ocfs2_rec_clusters(el, &el->l_recs[i]) +
- le32_to_cpu(el->l_recs[i].e_cpos)) -
- new_highest_cpos;
- blkno = le64_to_cpu(el->l_recs[i].e_blkno) +
- ocfs2_clusters_to_blocks(inode->i_sb,
- ocfs2_rec_clusters(el, &el->l_recs[i]) -
- clusters_to_del);
+ /*
+ * Partial truncate. it also should be
+ * the last truncate we're doing.

fs/ocfs2/alloc.c start: · line 7108

Quoted 6 lines

fs/ocfs2/alloc.c +6 -0

+ */
+ trunc_cpos = new_highest_cpos;
+ trunc_len = range - new_highest_cpos;
+ coff = new_highest_cpos - le32_to_cpu(rec->e_cpos);
+ blkno = le64_to_cpu(rec->e_blkno) +
+ ocfs2_clusters_to_blocks(inode->i_sb, coff);

another reason you want to change clusters_to_del to trunc_len? The calcuation is the same.

Quoted 19 lines

fs/ocfs2/alloc.c +5 -3

 } else {
+ /*
+ * Truncate completed, leave happily.
+ */
 status = 0;
 goto bail;
 }
 
- mlog(0, "clusters_to_del = %u in this pass, tail blk=%llu\n",
- clusters_to_del, (unsigned long long)path_leaf_bh(path)->b_blocknr);
+ phys_cpos = ocfs2_blocks_to_clusters(inode->i_sb, blkno);
 
- if (el->l_recs[i].e_flags & OCFS2_EXT_REFCOUNTED && clusters_to_del) {
+ if (el->l_recs[i].e_flags & OCFS2_EXT_REFCOUNTED && trunc_len) {

BUG_ON(!(OCFS2_I(inode)->ip_dyn_features &
OCFS2_HAS_REFCOUNT_FL));

fs/ocfs2/alloc.c +0 -0

@@ -7533,59 +7130,25 @@ start:
 

fs/ocfs2/alloc.c start: · line 7538

Quoted 6 lines

fs/ocfs2/alloc.c +1 -2

 status = ocfs2_prepare_refcount_change_for_del(inode, fe_bh,
 blkno,
- clusters_to_del,
+ trunc_len,
 &credits,
- &meta_ac);

As I said in the previous e-mail, we may add this function to the ocfs2_remove_btree_range.
And what's more, we don't need to fixing punching hole any more since this function can handle refcount tree by itself.

Quoted 80 lines

fs/ocfs2/alloc.c +5 -38

- if (status < 0) {
- mlog_errno(status);
- goto bail;
- }
- }
-
- mutex_lock(&tl_inode->i_mutex);
- tl_sem = 1;
- /* ocfs2_truncate_log_needs_flush guarantees us at least one
- * record is free for use. If there isn't any, we flush to get
- * an empty truncate log. */
- if (ocfs2_truncate_log_needs_flush(osb)) {
- status = __ocfs2_flush_truncate_log(osb);
+ &ref_blocks);
 if (status < 0) {
 mlog_errno(status);
 goto bail;
 }
 }
 
- credits += ocfs2_calc_tree_trunc_credits(osb->sb, clusters_to_del,
- (struct ocfs2_dinode *)fe_bh->b_data,
- el);
- handle = ocfs2_start_trans(osb, credits);
- if (IS_ERR(handle)) {
- status = PTR_ERR(handle);
- handle = NULL;
- mlog_errno(status);
- goto bail;
- }
-
- status = ocfs2_do_truncate(osb, clusters_to_del, inode, fe_bh, handle,
- tc, path, meta_ac);
+ status = ocfs2_remove_btree_range(inode, &et, trunc_cpos, phys_cpos,
+ trunc_len, &dealloc, credits,
+ ref_blocks, flags);
 if (status < 0) {
 mlog_errno(status);
 goto bail;
 }
-
- mutex_unlock(&tl_inode->i_mutex);
- tl_sem = 0;
-
- ocfs2_commit_trans(osb, handle);
- handle = NULL;
-
+
 ocfs2_reinit_path(path, 1);
 
- if (meta_ac) {
- ocfs2_free_alloc_context(meta_ac);
- meta_ac = NULL;
- }
-
 if (ref_tree) {
 ocfs2_unlock_refcount_tree(osb, ref_tree, 1);
 ref_tree = NULL;

@@ -7598,28 +7161,15 @@ start:
 goto start;
 

bail:
-
ocfs2_schedule_truncate_log_flush(osb, 1);

- if (tl_sem)
- mutex_unlock(&tl_inode->i_mutex);

fs/ocfs2/alloc.c +0 -7

-
- if (handle)
- ocfs2_commit_trans(osb, handle);
-
- if (meta_ac)
- ocfs2_free_alloc_context(meta_ac);
-

if (ref_tree)
ocfs2_unlock_refcount_tree(osb, ref_tree, 1);

- ocfs2_run_deallocs(osb, &tc->tc_dealloc);
+ ocfs2_run_deallocs(osb, &dealloc);

Quoted 5 lines

ocfs2_free_path(path);

- /* This will drop the ext_alloc cluster lock for us */
- ocfs2_free_truncate_context(tc);
-

I would suggest you to separate the removal of tc and ocfs2_prepare_truncate to another function since it has nothing to do with this patch.

Regards,
Tao

Truncating and punching holes codes need to take refcount into account when removing a extent record, it has to decrease refcount on refcount tree.

As in, credits and blocks reserved from allocator for refcount should be calculated beforehand in caller who revokes ocfs2_remove_btree_range(), it will not hurt anyone who're using this function, if you want an extra credits and reserved blocks be taken into account, just pass in, otherwise, leave it as 0.

Originally it's made for truncating codes who is being optimized to use ocfs2_remove_btree_range for straightfoward purpose, our punching holes codes however, will also benenfit from this patch as well.

The patch also add a new func ocfs2_lock_allocator_with_extra_blocks() in suballoc.c, which was almost the same as ocfs2_lock_allocators(), except it accepts some blocks for extra use(e.g. we may need more blocks to reserve when truncating a file to consider refcount case), and and it only handles meta data allocations, for now, it's only called by ocfs2_remove_btree_range() to handle truncating and punching holes.

Signed-off-by Tristan Ye <[email protected]>

---
 fs/ocfs2/alloc.c    |   28 ++++++++++++++++++++++------
 fs/ocfs2/alloc.h    |    3 ++-
 fs/ocfs2/dir.c      |    2 +-
 fs/ocfs2/file.c     |    2 +-
 fs/ocfs2/suballoc.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/suballoc.h |    6 ++++++
 6 files changed, 83 insertions(+), 9 deletions(-)

fs/ocfs2/alloc.c +0 -0

index 38a42f5..6c5f1de 100644

fs/ocfs2/alloc.c +20 -5

@@ -5682,7 +5683,8 @@ int ocfs2_remove_btree_range(struct inode *inode,
 	handle_t *handle;
 	struct ocfs2_alloc_context *meta_ac = NULL;
 
-	ret = ocfs2_lock_allocators(inode, et, 0, 1, NULL, &meta_ac);
+	ret = ocfs2_lock_allocator_with_extra_blocks(inode, et, 1,
+						     &meta_ac, extra_blocks);
 	if (ret) {
 		mlog_errno(ret);
 		return ret;

@@ -5698,7 +5700,8 @@ int ocfs2_remove_btree_range(struct inode *inode,
 		}
 	}
 
-	handle = ocfs2_start_trans(osb, ocfs2_remove_extent_credits(osb->sb));
+	handle = ocfs2_start_trans(osb,
+			ocfs2_remove_extent_credits(osb->sb) + credits);
 	if (IS_ERR(handle)) {
 		ret = PTR_ERR(handle);
 		mlog_errno(ret);

@@ -5729,9 +5732,22 @@ int ocfs2_remove_btree_range(struct inode *inode,
 		goto out_commit;
 	}
 
-	ret = ocfs2_truncate_log_append(osb, handle, phys_blkno, len);
-	if (ret)
-		mlog_errno(ret);
+	if (phys_blkno) {
+		if (flags & OCFS2_EXT_REFCOUNTED) {
+			ret = ocfs2_decrease_refcount(inode, handle,
+                                        ocfs2_blocks_to_clusters(osb->sb,
+                                                                 phys_blkno),
+                                        len, meta_ac,
+                                        dealloc, 1);
+		}
+
+		else
+			ret = ocfs2_truncate_log_append(osb, handle,
+							phys_blkno, len);
+		if (ret)
+                        mlog_errno(ret );
+
+	}
 
 out_commit:
 	ocfs2_commit_trans(osb, handle);

fs/ocfs2/alloc.h +2 -1

index 9c122d5..483cdf1 100644
@@ -141,7 +141,8 @@ int ocfs2_remove_extent(handle_t *handle, struct ocfs2_extent_tree *et,
 int ocfs2_remove_btree_range(struct inode *inode,
 			     struct ocfs2_extent_tree *et,
 			     u32 cpos, u32 phys_cpos, u32 len,
-			     struct ocfs2_cached_dealloc_ctxt *dealloc);
+			     struct ocfs2_cached_dealloc_ctxt *dealloc,
+			     int credits, int ref_blocks, int flags);
 
 int ocfs2_num_free_extents(struct ocfs2_super *osb,
 			   struct ocfs2_extent_tree *et);

fs/ocfs2/dir.c +1 -1

index 28c3ec2..3aff1e1 100644
@@ -4557,7 +4557,7 @@ int ocfs2_dx_dir_truncate(struct inode *dir, struct buffer_head *di_bh)
 		p_cpos = ocfs2_blocks_to_clusters(dir->i_sb, blkno);
 
 		ret = ocfs2_remove_btree_range(dir, &et, cpos, p_cpos, clen,
-					       &dealloc);
+					       &dealloc, 0, 0, 0);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;

fs/ocfs2/file.c +1 -1

index 89fc8ee..3bebb3b 100644
@@ -1499,7 +1499,7 @@ static int ocfs2_remove_inode_range(struct inode *inode,
 		if (phys_cpos != 0) {
 			ret = ocfs2_remove_btree_range(inode, &et, cpos,
 						       phys_cpos, alloc_size,
-						       &dealloc);
+						       &dealloc, 0, 0, 0);
 			if (ret) {
 				mlog_errno(ret);
 				goto out;

fs/ocfs2/suballoc.c +4 -0

index c30b644..8bf4523 100644
@@ -2208,6 +2208,57 @@ out:
 }
 
 /*
+ * ocfs2_lock_allocator_with_extra_blocks() would look almost the
+ * same as ocfs2_lock_alloctors(), except for it accepts a blocks
+ * number to reserve some extra blocks, and it only handles meta
+ * data allocations.

fs/ocfs2/suballoc.c +16 -0

+{
+	int ret = 0, num_free_extents, blocks;
+	unsigned int max_recs_needed = 2 * extents_to_split;
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+
+	*meta_ac = NULL;
+
+	num_free_extents = ocfs2_num_free_extents(osb, et);
+	if (num_free_extents < 0) {
+		ret = num_free_extents;
+		mlog_errno(ret);
+		goto out;
+	}
+
+	if (!num_free_extents ||
+	    (ocfs2_sparse_alloc(osb) && num_free_extents < max_recs_needed)) {

Hi Tristan,
Thanks for the quick coding. Here are some comments.

Quoted 51 lines · Tristan Ye

Truncating and punching holes codes need to take refcount into account when
removing a extent record, it has to decrease refcount on refcount tree.

As in, credits and blocks reserved from allocator for refcount should be calculated
beforehand in caller who revokes ocfs2_remove_btree_range(), it will not hurt anyone
who're using this function, if you want an extra credits and reserved blocks be taken
into account, just pass in, otherwise, leave it as 0.

Originally it's made for truncating codes who is being optimized to use ocfs2_remove_btree_range
for straightfoward purpose, our punching holes codes however, will also benenfit from this patch
as well.

The patch also add a new func ocfs2_lock_allocator_with_extra_blocks() in suballoc.c, which was almost the same as ocfs2_lock_allocators(), except it accepts some blocks
for extra use(e.g. we may need more blocks to reserve when truncating a file to consider
refcount case), and and it only handles meta data allocations, for now, it's only called
by ocfs2_remove_btree_range() to handle truncating and punching holes.

Signed-off-by Tristan Ye <[email protected]>

---
 fs/ocfs2/alloc.c    |   28 ++++++++++++++++++++++------
 fs/ocfs2/alloc.h    |    3 ++-
 fs/ocfs2/dir.c      |    2 +-
 fs/ocfs2/file.c     |    2 +-
 fs/ocfs2/suballoc.c |   51 
+++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/suballoc.h |    6 ++++++
 6 files changed, 83 insertions(+), 9 deletions(-)

fs/ocfs2/alloc.c +2 -1

index 38a42f5..6c5f1de 100644
@@ -5673,7 +5673,8 @@ out:
 int ocfs2_remove_btree_range(struct inode *inode,
                  struct ocfs2_extent_tree *et,
                  u32 cpos, u32 phys_cpos, u32 len,
-                 struct ocfs2_cached_dealloc_ctxt *dealloc)
+                 struct ocfs2_cached_dealloc_ctxt *dealloc,
+                 int credits, int extra_blocks, int flags)
 {

fs/ocfs2/alloc.c out: · line 5680

Quoted 3 lines

fs/ocfs2/alloc.c +0 -0

     int ret;
     u64 phys_blkno = ocfs2_clusters_to_blocks(inode->i_sb, phys_cpos);

I just think that we should let this function handle the work of lock/unlock refcount tree.
It is a necessary part of ocfs2_remove_btree_range. The caller should knows nothing about
whether the refcount tree need locked or not.

Quoted 112 lines

fs/ocfs2/alloc.c +4 -2

@@ -5682,7 +5683,8 @@ int ocfs2_remove_btree_range(struct inode *inode,
     handle_t *handle;
     struct ocfs2_alloc_context *meta_ac = NULL;
 
-    ret = ocfs2_lock_allocators(inode, et, 0, 1, NULL, &meta_ac);
+    ret = ocfs2_lock_allocator_with_extra_blocks(inode, et, 1,
+                             &meta_ac, extra_blocks);
     if (ret) {
         mlog_errno(ret);
         return ret;

@@ -5698,7 +5700,8 @@ int ocfs2_remove_btree_range(struct inode *inode,
         }
     }
 
-    handle = ocfs2_start_trans(osb, 
 ocfs2_remove_extent_credits(osb->sb));
+    handle = ocfs2_start_trans(osb,
+            ocfs2_remove_extent_credits(osb->sb) + credits);
     if (IS_ERR(handle)) {
         ret = PTR_ERR(handle);

         mlog_errno(ret);

fs/ocfs2/alloc.c +16 -3

@@ -5729,9 +5732,22 @@ int ocfs2_remove_btree_range(struct inode *inode,
         goto out_commit;
     }
 
-    ret = ocfs2_truncate_log_append(osb, handle, phys_blkno, len);
-    if (ret)
-        mlog_errno(ret);
+    if (phys_blkno) {
+        if (flags & OCFS2_EXT_REFCOUNTED) {
+            ret = ocfs2_decrease_refcount(inode, handle,
+                                        
 ocfs2_blocks_to_clusters(osb->sb,
+                                                                 
 phys_blkno),
+                                        len, meta_ac,
+                                        dealloc, 1);
+        }
+
+        else
+            ret = ocfs2_truncate_log_append(osb, handle,
+                            phys_blkno, len);
+        if (ret)
+                        mlog_errno(ret );
+
+    }
 

out_commit:

     ocfs2_commit_trans(osb, handle);

fs/ocfs2/alloc.h +2 -1

index 9c122d5..483cdf1 100644
@@ -141,7 +141,8 @@ int ocfs2_remove_extent(handle_t *handle, struct 
 ocfs2_extent_tree *et,
 int ocfs2_remove_btree_range(struct inode *inode,
                  struct ocfs2_extent_tree *et,
                  u32 cpos, u32 phys_cpos, u32 len,
-                 struct ocfs2_cached_dealloc_ctxt *dealloc);
+                 struct ocfs2_cached_dealloc_ctxt *dealloc,
+                 int credits, int ref_blocks, int flags);
 
 int ocfs2_num_free_extents(struct ocfs2_super *osb,

                struct ocfs2_extent_tree *et);

fs/ocfs2/dir.c +1 -1

index 28c3ec2..3aff1e1 100644
@@ -4557,7 +4557,7 @@ int ocfs2_dx_dir_truncate(struct inode *dir, 
 struct buffer_head *di_bh)
         p_cpos = ocfs2_blocks_to_clusters(dir->i_sb, blkno);
 
         ret = ocfs2_remove_btree_range(dir, &et, cpos, p_cpos, clen,
-                           &dealloc);
+                           &dealloc, 0, 0, 0);
         if (ret) {
             mlog_errno(ret);

             goto out;

fs/ocfs2/file.c +1 -1

index 89fc8ee..3bebb3b 100644
@@ -1499,7 +1499,7 @@ static int ocfs2_remove_inode_range(struct 
 inode *inode,
         if (phys_cpos != 0) {
             ret = ocfs2_remove_btree_range(inode, &et, cpos,
                                phys_cpos, alloc_size,
-                               &dealloc);
+                               &dealloc, 0, 0, 0);
             if (ret) {
                 mlog_errno(ret);

                 goto out;

fs/ocfs2/suballoc.c +12 -0

index c30b644..8bf4523 100644
@@ -2208,6 +2208,57 @@ out:
 }
 
 /*
+ * ocfs2_lock_allocator_with_extra_blocks() would look almost the
+ * same as ocfs2_lock_alloctors(), except for it accepts a blocks
+ * number to reserve some extra blocks, and it only handles meta
+ * data allocations.
+ *
+ * Currently, only ocfs2_remove_btree_range() uses it for truncating
+ * and punching holes.
+ */
+int ocfs2_lock_allocator_with_extra_blocks(struct inode *inode,
+                       struct ocfs2_extent_tree *et,
+                       u32 extents_to_split,
+                       struct ocfs2_alloc_context **meta_ac,

fs/ocfs2/suballoc.c out: · line 2224

Quoted 2 lines

fs/ocfs2/suballoc.c +1 -0

+                       int extra_blocks)
 

I am just curious about this function.
So if you want to make it as a generic function, why you remove another parameter clusters_to_add?
On the other hand, if you only want it to be used in ocfs2_remove_btree_range, why not remove extents_to_split also since we know that function only works for one extent record?

Quoted 23 lines

fs/ocfs2/suballoc.c +22 -0

+{
+    int ret = 0, num_free_extents, blocks;
+    unsigned int max_recs_needed = 2 * extents_to_split;
+    struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+
+    *meta_ac = NULL;
+
+    num_free_extents = ocfs2_num_free_extents(osb, et);
+    if (num_free_extents < 0) {
+        ret = num_free_extents;
+        mlog_errno(ret);
+        goto out;
+    }
+
+    if (!num_free_extents ||
+        (ocfs2_sparse_alloc(osb) && num_free_extents < 
 max_recs_needed)) {
+        blocks = ocfs2_extend_meta_needed(et->et_root_el) +
+                extra_blocks;
+        ret = ocfs2_reserve_new_metadata_blocks(osb, blocks, meta_ac);
+        if (ret < 0) {
+            if (ret != -ENOSPC)
+                mlog_errno(ret);

fs/ocfs2/suballoc.c out:

Quoted 4 lines

fs/ocfs2/suballoc.c +3 -0

+            goto out;
+        }
+    }
 

here is a bug. You should calculate the 'blocks' in the 'if' while call ocfs2_reserve_new_metadata_blocks after if. consider the case num_free_extents > 0 and extra_blocks > 0. In your code, we won't reserve new metadatas which is false.

Regards,
Tao