ocfs2: check the metadate alloc before marking extent written

We need to check the free number of the records in each loop to mark extent written, because the last extent block may be changed through many times marking extent written and the 'num_free_extents' also be changed. In the worst case, the 'num_free_extents' may become less than the beginning of the loop. So we should not estimate the free number of the records at the beginning of the loop to mark extent written.

Reported-by John Lightsey <[email protected]>

Signed-off-by Alex Chen <[email protected]>

Reviewed-by Jun Piao <[email protected]>

---
 fs/ocfs2/aops.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 12 deletions(-)

fs/ocfs2/aops.c +31 -3

index d151632..702aebc 100644
@@ -2272,6 +2272,35 @@ static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock,
 	return ret;
 }
 
+static int ocfs2_dio_should_restart(struct ocfs2_extent_tree *et,
+		struct ocfs2_alloc_context *meta_ac, int max_rec_needed)
+{
+	int status = 0, free_extents;
+
+	free_extents = ocfs2_num_free_extents(et);
+	if (free_extents < 0) {
+		status = free_extents;
+		mlog_errno(status);
+		return status;
+	}
+
+	/*
+	 * there are two cases which could cause us to EAGAIN in the
+	 * we-need-more-metadata case:
+	 * 1) we haven't reserved *any*
+	 * 2) we are so fragmented, we've needed to add metadata too
+	 *    many times.
+	 */
+	if (free_extents < max_rec_needed) {
+		if (!meta_ac || (ocfs2_alloc_context_bits_left(meta_ac)
+				< ocfs2_extend_meta_needed(et->et_root_el)))
+			status = 1;
+	}
+
+	return status;
+}
+
+#define OCFS2_MAX_REC_NEEDED_SPLIT 2
 static int ocfs2_dio_end_io_write(struct inode *inode,
 				  struct ocfs2_dio_write_ctxt *dwc,
 				  loff_t offset,

@@ -2281,14 +2310,13 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 	struct ocfs2_extent_tree et;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
-	struct ocfs2_unwritten_extent *ue = NULL;
+	struct ocfs2_unwritten_extent *ue = NULL, *tmp_ue;
 	struct buffer_head *di_bh = NULL;
 	struct ocfs2_dinode *di;
-	struct ocfs2_alloc_context *data_ac = NULL;
 	struct ocfs2_alloc_context *meta_ac = NULL;
 	handle_t *handle = NULL;
 	loff_t end = offset + bytes;
-	int ret = 0, credits = 0, locked = 0;
+	int ret = 0, credits = 0, locked = 0, restart = 0;
 
 	ocfs2_init_dealloc_ctxt(&dealloc);
 

fs/ocfs2/aops.c +3 -2

 	ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh);
 
-	ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2,
-				    &data_ac, &meta_ac);
+restart_all:
+	ret = ocfs2_lock_allocators(inode, &et, 0, OCFS2_MAX_REC_NEEDED_SPLIT,
+				    NULL, &meta_ac);
 	if (ret) {
 		mlog_errno(ret);
 		goto unlock;

fs/ocfs2/aops.c +26 -2

 	}
 	ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh,
 				      OCFS2_JOURNAL_ACCESS_WRITE);

@@ -2352,7 +2381,17 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 		goto commit;
 	}
 
-	list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) {
+	list_for_each_entry_safe(ue, tmp_ue, &dwc->dw_zero_list, ue_node) {
+		ret = ocfs2_dio_should_restart(&et, meta_ac,
+				OCFS2_MAX_REC_NEEDED_SPLIT * 2);
+		if (ret < 0) {
+			mlog_errno(ret);
+			break;
+		} else if (ret == 1) {
+			restart = 1;
+			break;
+		}
+
 		ret = ocfs2_mark_extent_written(inode, &et, handle,
 						ue->ue_cpos, 1,
 						ue->ue_phys,

@@ -2361,24 +2400,37 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 			mlog_errno(ret);
 			break;
 		}
+
+		dwc->dw_zero_count--;
+		list_del_init(&ue->ue_node);
+		spin_lock(&oi->ip_lock);
+		list_del_init(&ue->ue_ip_node);
+		spin_unlock(&oi->ip_lock);
+		kfree(ue);
 	}
 
-	if (end > i_size_read(inode)) {
+	if (!restart && end > i_size_read(inode)) {
 		ret = ocfs2_set_inode_size(handle, inode, di_bh, end);
 		if (ret < 0)
 			mlog_errno(ret);
 	}
+
 commit:
 	ocfs2_commit_trans(osb, handle);
+free_ac:
+	if (meta_ac) {
+		ocfs2_free_alloc_context(meta_ac);
+		meta_ac = NULL;
+	}
+	if (restart) {

fs/ocfs2/aops.c +0 -0

 	ocfs2_run_deallocs(osb, &dealloc);
 	if (locked)
 		inode_unlock(inode);

Quoted 70 lines · alex chen

We need to check the free number of the records in each loop to mark extent written, because the last extent block may be changed through many times marking extent written and the 'num_free_extents' also be changed. In the worst case, the 'num_free_extents' may become less than the beginning of the loop. So we should not estimate the free number of the records at the beginning of the loop to mark extent written.

Reported-by John Lightsey <[email protected]>

Signed-off-by Alex Chen <[email protected]>

Reviewed-by Jun Piao <[email protected]>

---
 fs/ocfs2/aops.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 12 deletions(-)

fs/ocfs2/aops.c +31 -3

index d151632..702aebc 100644
@@ -2272,6 +2272,35 @@ static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock,
 	return ret;
 }
 
+static int ocfs2_dio_should_restart(struct ocfs2_extent_tree *et,
+		struct ocfs2_alloc_context *meta_ac, int max_rec_needed)
+{
+	int status = 0, free_extents;
+
+	free_extents = ocfs2_num_free_extents(et);
+	if (free_extents < 0) {
+		status = free_extents;
+		mlog_errno(status);
+		return status;
+	}
+
+	/*
+	 * there are two cases which could cause us to EAGAIN in the
+	 * we-need-more-metadata case:
+	 * 1) we haven't reserved *any*
+	 * 2) we are so fragmented, we've needed to add metadata too
+	 *    many times.
+	 */
+	if (free_extents < max_rec_needed) {
+		if (!meta_ac || (ocfs2_alloc_context_bits_left(meta_ac)
+				< ocfs2_extend_meta_needed(et->et_root_el)))
+			status = 1;
+	}
+
+	return status;
+}
+
+#define OCFS2_MAX_REC_NEEDED_SPLIT 2
 static int ocfs2_dio_end_io_write(struct inode *inode,
 				  struct ocfs2_dio_write_ctxt *dwc,
 				  loff_t offset,

@@ -2281,14 +2310,13 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 	struct ocfs2_extent_tree et;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
-	struct ocfs2_unwritten_extent *ue = NULL;
+	struct ocfs2_unwritten_extent *ue = NULL, *tmp_ue;
 	struct buffer_head *di_bh = NULL;
 	struct ocfs2_dinode *di;
-	struct ocfs2_alloc_context *data_ac = NULL;
 	struct ocfs2_alloc_context *meta_ac = NULL;
 	handle_t *handle = NULL;
 	loff_t end = offset + bytes;
-	int ret = 0, credits = 0, locked = 0;
+	int ret = 0, credits = 0, locked = 0, restart = 0;
 

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2358

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

 	ocfs2_init_dealloc_ctxt(&dealloc);
 

@@ -2330,8 +2358,9 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 

Should we restart here?

Quoted 12 lines

fs/ocfs2/aops.c +3 -2

 	ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh);
 
-	ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2,
-				    &data_ac, &meta_ac);
+restart_all:
+	ret = ocfs2_lock_allocators(inode, &et, 0, OCFS2_MAX_REC_NEEDED_SPLIT,
+				    NULL, &meta_ac);
 	if (ret) {
 		mlog_errno(ret);
 		goto unlock;

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2375

Quoted 5 lines

fs/ocfs2/aops.c +1 -1

@@ -2343,7 +2372,7 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 	if (IS_ERR(handle)) {
 		ret = PTR_ERR(handle);
 		mlog_errno(ret);
-		goto unlock;
+		goto free_ac;

I don't think it is a necessary change here.

Quoted 51 lines

fs/ocfs2/aops.c +26 -2

 	}
 	ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh,
 				      OCFS2_JOURNAL_ACCESS_WRITE);

@@ -2352,7 +2381,17 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 		goto commit;
 	}
 
-	list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) {
+	list_for_each_entry_safe(ue, tmp_ue, &dwc->dw_zero_list, ue_node) {
+		ret = ocfs2_dio_should_restart(&et, meta_ac,
+				OCFS2_MAX_REC_NEEDED_SPLIT * 2);
+		if (ret < 0) {
+			mlog_errno(ret);
+			break;
+		} else if (ret == 1) {
+			restart = 1;
+			break;
+		}
+
 		ret = ocfs2_mark_extent_written(inode, &et, handle,
 						ue->ue_cpos, 1,
 						ue->ue_phys,

@@ -2361,24 +2400,37 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 			mlog_errno(ret);
 			break;
 		}
+
+		dwc->dw_zero_count--;
+		list_del_init(&ue->ue_node);
+		spin_lock(&oi->ip_lock);
+		list_del_init(&ue->ue_ip_node);
+		spin_unlock(&oi->ip_lock);
+		kfree(ue);
 	}
 
-	if (end > i_size_read(inode)) {
+	if (!restart && end > i_size_read(inode)) {
 		ret = ocfs2_set_inode_size(handle, inode, di_bh, end);
 		if (ret < 0)
 			mlog_errno(ret);
 	}
+
 commit:
 	ocfs2_commit_trans(osb, handle);
+free_ac:
+	if (meta_ac) {
+		ocfs2_free_alloc_context(meta_ac);
+		meta_ac = NULL;
+	}
+	if (restart) {

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2379

Quoted 10 lines

fs/ocfs2/aops.c +3 -2

+		restart = 0;
+		goto restart_all;
+	}
 unlock:
 	up_write(&oi->ip_alloc_sem);
 	ocfs2_inode_unlock(inode, 1);
 	brelse(di_bh);
 out:
-	if (data_ac)
-		ocfs2_free_alloc_context(data_ac);

Agree to cleanup the unused data_ac.

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2381

Quoted 2 lines

fs/ocfs2/aops.c +0 -2

-	if (meta_ac)
-		ocfs2_free_alloc_context(meta_ac);Just move the restart logic down is enough.

Thanks,
Joseph

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2436

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

 	ocfs2_run_deallocs(osb, &dealloc);
 	if (locked)
 		inode_unlock(inode);

.

Hi Joseph,

Thanks for your reviews.

Quoted 73 lines · Joseph Qi

Quoted 70 lines · alex chen

We need to check the free number of the records in each loop to mark extent written, because the last extent block may be changed through many times marking extent written and the 'num_free_extents' also be changed. In the worst case, the 'num_free_extents' may become less than the beginning of the loop. So we should not estimate the free number of the records at the beginning of the loop to mark extent written.

Reported-by John Lightsey <[email protected]>

Signed-off-by Alex Chen <[email protected]>

Reviewed-by Jun Piao <[email protected]>

---
 fs/ocfs2/aops.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 12 deletions(-)

fs/ocfs2/aops.c +31 -3

index d151632..702aebc 100644
@@ -2272,6 +2272,35 @@ static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock,
 	return ret;
 }
 
+static int ocfs2_dio_should_restart(struct ocfs2_extent_tree *et,
+		struct ocfs2_alloc_context *meta_ac, int max_rec_needed)
+{
+	int status = 0, free_extents;
+
+	free_extents = ocfs2_num_free_extents(et);
+	if (free_extents < 0) {
+		status = free_extents;
+		mlog_errno(status);
+		return status;
+	}
+
+	/*
+	 * there are two cases which could cause us to EAGAIN in the
+	 * we-need-more-metadata case:
+	 * 1) we haven't reserved *any*
+	 * 2) we are so fragmented, we've needed to add metadata too
+	 *    many times.
+	 */
+	if (free_extents < max_rec_needed) {
+		if (!meta_ac || (ocfs2_alloc_context_bits_left(meta_ac)
+				< ocfs2_extend_meta_needed(et->et_root_el)))
+			status = 1;
+	}
+
+	return status;
+}
+
+#define OCFS2_MAX_REC_NEEDED_SPLIT 2
 static int ocfs2_dio_end_io_write(struct inode *inode,
 				  struct ocfs2_dio_write_ctxt *dwc,
 				  loff_t offset,

@@ -2281,14 +2310,13 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 	struct ocfs2_extent_tree et;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
-	struct ocfs2_unwritten_extent *ue = NULL;
+	struct ocfs2_unwritten_extent *ue = NULL, *tmp_ue;
 	struct buffer_head *di_bh = NULL;
 	struct ocfs2_dinode *di;
-	struct ocfs2_alloc_context *data_ac = NULL;
 	struct ocfs2_alloc_context *meta_ac = NULL;
 	handle_t *handle = NULL;
 	loff_t end = offset + bytes;
-	int ret = 0, credits = 0, locked = 0;
+	int ret = 0, credits = 0, locked = 0, restart = 0;
 

Quoted 5 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2358

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

 	ocfs2_init_dealloc_ctxt(&dealloc);
 

@@ -2330,8 +2358,9 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 

Should we restart here?

OK. we should restart before initialized the inode extent tree.

Quoted 18 lines

Quoted 12 lines

fs/ocfs2/aops.c +3 -2

 	ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh);
 
-	ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2,
-				    &data_ac, &meta_ac);
+restart_all:
+	ret = ocfs2_lock_allocators(inode, &et, 0, OCFS2_MAX_REC_NEEDED_SPLIT,
+				    NULL, &meta_ac);
 	if (ret) {
 		mlog_errno(ret);
 		goto unlock;

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2375

Quoted 5 lines

fs/ocfs2/aops.c +1 -1

@@ -2343,7 +2372,7 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 	if (IS_ERR(handle)) {
 		ret = PTR_ERR(handle);
 		mlog_errno(ret);
-		goto unlock;
+		goto free_ac;

I don't think it is a necessary change here.

we need to free the 'meta_ac' which allocated in ocfs2_lock_allocators() when we start transaction failed.

Thanks,
Alex

Quoted 63 lines

Quoted 51 lines

fs/ocfs2/aops.c +26 -2

 	}
 	ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh,
 				      OCFS2_JOURNAL_ACCESS_WRITE);

@@ -2352,7 +2381,17 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 		goto commit;
 	}
 
-	list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) {
+	list_for_each_entry_safe(ue, tmp_ue, &dwc->dw_zero_list, ue_node) {
+		ret = ocfs2_dio_should_restart(&et, meta_ac,
+				OCFS2_MAX_REC_NEEDED_SPLIT * 2);
+		if (ret < 0) {
+			mlog_errno(ret);
+			break;
+		} else if (ret == 1) {
+			restart = 1;
+			break;
+		}
+
 		ret = ocfs2_mark_extent_written(inode, &et, handle,
 						ue->ue_cpos, 1,
 						ue->ue_phys,

@@ -2361,24 +2400,37 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 			mlog_errno(ret);
 			break;
 		}
+
+		dwc->dw_zero_count--;
+		list_del_init(&ue->ue_node);
+		spin_lock(&oi->ip_lock);
+		list_del_init(&ue->ue_ip_node);
+		spin_unlock(&oi->ip_lock);
+		kfree(ue);
 	}
 
-	if (end > i_size_read(inode)) {
+	if (!restart && end > i_size_read(inode)) {
 		ret = ocfs2_set_inode_size(handle, inode, di_bh, end);
 		if (ret < 0)
 			mlog_errno(ret);
 	}
+
 commit:
 	ocfs2_commit_trans(osb, handle);
+free_ac:
+	if (meta_ac) {
+		ocfs2_free_alloc_context(meta_ac);
+		meta_ac = NULL;
+	}
+	if (restart) {

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2379

Quoted 10 lines

fs/ocfs2/aops.c +3 -2

+		restart = 0;
+		goto restart_all;
+	}
 unlock:
 	up_write(&oi->ip_alloc_sem);
 	ocfs2_inode_unlock(inode, 1);
 	brelse(di_bh);
 out:
-	if (data_ac)
-		ocfs2_free_alloc_context(data_ac);

Agree to cleanup the unused data_ac.

Quoted 2 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2381

Quoted 2 lines

fs/ocfs2/aops.c +0 -2

-	if (meta_ac)
-		ocfs2_free_alloc_context(meta_ac);

Quoted 11 lines

Thanks,
Joseph

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2436

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

 	ocfs2_run_deallocs(osb, &dealloc);
 	if (locked)
 		inode_unlock(inode);

.

Quoted 105 lines · alex chen

Hi Joseph,

Thanks for your reviews.

Quoted 73 lines · Joseph Qi

Quoted 70 lines · alex chen

We need to check the free number of the records in each loop to mark extent written, because the last extent block may be changed through many times marking extent written and the 'num_free_extents' also be changed. In the worst case, the 'num_free_extents' may become less than the beginning of the loop. So we should not estimate the free number of the records at the beginning of the loop to mark extent written.

Reported-by John Lightsey <[email protected]>

Signed-off-by Alex Chen <[email protected]>

Reviewed-by Jun Piao <[email protected]>

---
 fs/ocfs2/aops.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 12 deletions(-)

fs/ocfs2/aops.c +31 -3

index d151632..702aebc 100644
@@ -2272,6 +2272,35 @@ static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock,
 	return ret;
 }
 
+static int ocfs2_dio_should_restart(struct ocfs2_extent_tree *et,
+		struct ocfs2_alloc_context *meta_ac, int max_rec_needed)
+{
+	int status = 0, free_extents;
+
+	free_extents = ocfs2_num_free_extents(et);
+	if (free_extents < 0) {
+		status = free_extents;
+		mlog_errno(status);
+		return status;
+	}
+
+	/*
+	 * there are two cases which could cause us to EAGAIN in the
+	 * we-need-more-metadata case:
+	 * 1) we haven't reserved *any*
+	 * 2) we are so fragmented, we've needed to add metadata too
+	 *    many times.
+	 */
+	if (free_extents < max_rec_needed) {
+		if (!meta_ac || (ocfs2_alloc_context_bits_left(meta_ac)
+				< ocfs2_extend_meta_needed(et->et_root_el)))
+			status = 1;
+	}
+
+	return status;
+}
+
+#define OCFS2_MAX_REC_NEEDED_SPLIT 2
 static int ocfs2_dio_end_io_write(struct inode *inode,
 				  struct ocfs2_dio_write_ctxt *dwc,
 				  loff_t offset,

@@ -2281,14 +2310,13 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 	struct ocfs2_extent_tree et;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
-	struct ocfs2_unwritten_extent *ue = NULL;
+	struct ocfs2_unwritten_extent *ue = NULL, *tmp_ue;
 	struct buffer_head *di_bh = NULL;
 	struct ocfs2_dinode *di;
-	struct ocfs2_alloc_context *data_ac = NULL;
 	struct ocfs2_alloc_context *meta_ac = NULL;
 	handle_t *handle = NULL;
 	loff_t end = offset + bytes;
-	int ret = 0, credits = 0, locked = 0;
+	int ret = 0, credits = 0, locked = 0, restart = 0;
 

Quoted 5 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2358

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

 	ocfs2_init_dealloc_ctxt(&dealloc);
 

@@ -2330,8 +2358,9 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 

Should we restart here?

OK. we should restart before initialized the inode extent tree.

Quoted 18 lines

Quoted 12 lines

fs/ocfs2/aops.c +3 -2

 	ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh);
 
-	ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2,
-				    &data_ac, &meta_ac);
+restart_all:
+	ret = ocfs2_lock_allocators(inode, &et, 0, OCFS2_MAX_REC_NEEDED_SPLIT,
+				    NULL, &meta_ac);
 	if (ret) {
 		mlog_errno(ret);
 		goto unlock;

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2375

Quoted 5 lines

fs/ocfs2/aops.c +1 -1

@@ -2343,7 +2372,7 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 	if (IS_ERR(handle)) {
 		ret = PTR_ERR(handle);
 		mlog_errno(ret);
-		goto unlock;
+		goto free_ac;

I don't think it is a necessary change here.

Quoted 3 lines

we need to free the 'meta_ac' which allocated in ocfs2_lock_allocators() when we start transaction failed.

As I described in the last mail, just move the restart check after the original free meta_ac check.

Quoted 65 lines

Thanks,
Alex

Quoted 63 lines

Quoted 51 lines

fs/ocfs2/aops.c +26 -2

 	}
 	ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh,
 				      OCFS2_JOURNAL_ACCESS_WRITE);

@@ -2352,7 +2381,17 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 		goto commit;
 	}
 
-	list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) {
+	list_for_each_entry_safe(ue, tmp_ue, &dwc->dw_zero_list, ue_node) {
+		ret = ocfs2_dio_should_restart(&et, meta_ac,
+				OCFS2_MAX_REC_NEEDED_SPLIT * 2);
+		if (ret < 0) {
+			mlog_errno(ret);
+			break;
+		} else if (ret == 1) {
+			restart = 1;
+			break;
+		}
+
 		ret = ocfs2_mark_extent_written(inode, &et, handle,
 						ue->ue_cpos, 1,
 						ue->ue_phys,

@@ -2361,24 +2400,37 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 			mlog_errno(ret);
 			break;
 		}
+
+		dwc->dw_zero_count--;
+		list_del_init(&ue->ue_node);
+		spin_lock(&oi->ip_lock);
+		list_del_init(&ue->ue_ip_node);
+		spin_unlock(&oi->ip_lock);
+		kfree(ue);
 	}
 
-	if (end > i_size_read(inode)) {
+	if (!restart && end > i_size_read(inode)) {
 		ret = ocfs2_set_inode_size(handle, inode, di_bh, end);
 		if (ret < 0)
 			mlog_errno(ret);
 	}
+
 commit:
 	ocfs2_commit_trans(osb, handle);
+free_ac:
+	if (meta_ac) {
+		ocfs2_free_alloc_context(meta_ac);
+		meta_ac = NULL;
+	}
+	if (restart) {

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2379

Quoted 10 lines

fs/ocfs2/aops.c +3 -2

+		restart = 0;
+		goto restart_all;
+	}
 unlock:
 	up_write(&oi->ip_alloc_sem);
 	ocfs2_inode_unlock(inode, 1);
 	brelse(di_bh);
 out:
-	if (data_ac)
-		ocfs2_free_alloc_context(data_ac);

Agree to cleanup the unused data_ac.

Quoted 2 lines

Quoted 2 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2381

Quoted 2 lines

fs/ocfs2/aops.c +0 -2

-	if (meta_ac)
-		ocfs2_free_alloc_context(meta_ac);

Just move the restart logic down is enough.

Quoted 4 lines

Quoted 4 lines

Thanks,
Joseph

Quoted 8 lines

Quoted 7 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2436

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

 	ocfs2_run_deallocs(osb, &dealloc);
 	if (locked)
 		inode_unlock(inode);

.

.

Quoted 179 lines · Joseph Qi

Quoted 105 lines · alex chen

Hi Joseph,

Thanks for your reviews.

Quoted 73 lines · Joseph Qi

Quoted 70 lines · alex chen

We need to check the free number of the records in each loop to mark extent written, because the last extent block may be changed through many times marking extent written and the 'num_free_extents' also be changed. In the worst case, the 'num_free_extents' may become less than the beginning of the loop. So we should not estimate the free number of the records at the beginning of the loop to mark extent written.

Reported-by John Lightsey <[email protected]>

Signed-off-by Alex Chen <[email protected]>

Reviewed-by Jun Piao <[email protected]>

---
 fs/ocfs2/aops.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 12 deletions(-)

fs/ocfs2/aops.c +31 -3

index d151632..702aebc 100644
@@ -2272,6 +2272,35 @@ static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock,
 	return ret;
 }
 
+static int ocfs2_dio_should_restart(struct ocfs2_extent_tree *et,
+		struct ocfs2_alloc_context *meta_ac, int max_rec_needed)
+{
+	int status = 0, free_extents;
+
+	free_extents = ocfs2_num_free_extents(et);
+	if (free_extents < 0) {
+		status = free_extents;
+		mlog_errno(status);
+		return status;
+	}
+
+	/*
+	 * there are two cases which could cause us to EAGAIN in the
+	 * we-need-more-metadata case:
+	 * 1) we haven't reserved *any*
+	 * 2) we are so fragmented, we've needed to add metadata too
+	 *    many times.
+	 */
+	if (free_extents < max_rec_needed) {
+		if (!meta_ac || (ocfs2_alloc_context_bits_left(meta_ac)
+				< ocfs2_extend_meta_needed(et->et_root_el)))
+			status = 1;
+	}
+
+	return status;
+}
+
+#define OCFS2_MAX_REC_NEEDED_SPLIT 2
 static int ocfs2_dio_end_io_write(struct inode *inode,
 				  struct ocfs2_dio_write_ctxt *dwc,
 				  loff_t offset,

@@ -2281,14 +2310,13 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 	struct ocfs2_extent_tree et;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
-	struct ocfs2_unwritten_extent *ue = NULL;
+	struct ocfs2_unwritten_extent *ue = NULL, *tmp_ue;
 	struct buffer_head *di_bh = NULL;
 	struct ocfs2_dinode *di;
-	struct ocfs2_alloc_context *data_ac = NULL;
 	struct ocfs2_alloc_context *meta_ac = NULL;
 	handle_t *handle = NULL;
 	loff_t end = offset + bytes;
-	int ret = 0, credits = 0, locked = 0;
+	int ret = 0, credits = 0, locked = 0, restart = 0;
 

Quoted 5 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2358

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

 	ocfs2_init_dealloc_ctxt(&dealloc);
 

@@ -2330,8 +2358,9 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 

Should we restart here?

OK. we should restart before initialized the inode extent tree.

Quoted 18 lines

Quoted 12 lines

fs/ocfs2/aops.c +3 -2

 	ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh);
 
-	ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2,
-				    &data_ac, &meta_ac);
+restart_all:
+	ret = ocfs2_lock_allocators(inode, &et, 0, OCFS2_MAX_REC_NEEDED_SPLIT,
+				    NULL, &meta_ac);
 	if (ret) {
 		mlog_errno(ret);
 		goto unlock;

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2375

Quoted 5 lines

fs/ocfs2/aops.c +1 -1

@@ -2343,7 +2372,7 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 	if (IS_ERR(handle)) {
 		ret = PTR_ERR(handle);
 		mlog_errno(ret);
-		goto unlock;
+		goto free_ac;

I don't think it is a necessary change here.

Quoted 3 lines

we need to free the 'meta_ac' which allocated in ocfs2_lock_allocators() when we start transaction failed.

As I described in the last mail, just move the restart check after the original free meta_ac check.

Quoted 65 lines

Thanks,
Alex

Quoted 63 lines

Quoted 51 lines

fs/ocfs2/aops.c +26 -2

 	}
 	ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh,
 				      OCFS2_JOURNAL_ACCESS_WRITE);

@@ -2352,7 +2381,17 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 		goto commit;
 	}
 
-	list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) {
+	list_for_each_entry_safe(ue, tmp_ue, &dwc->dw_zero_list, ue_node) {
+		ret = ocfs2_dio_should_restart(&et, meta_ac,
+				OCFS2_MAX_REC_NEEDED_SPLIT * 2);
+		if (ret < 0) {
+			mlog_errno(ret);
+			break;
+		} else if (ret == 1) {
+			restart = 1;
+			break;
+		}
+
 		ret = ocfs2_mark_extent_written(inode, &et, handle,
 						ue->ue_cpos, 1,
 						ue->ue_phys,

@@ -2361,24 +2400,37 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 			mlog_errno(ret);
 			break;
 		}
+
+		dwc->dw_zero_count--;
+		list_del_init(&ue->ue_node);
+		spin_lock(&oi->ip_lock);
+		list_del_init(&ue->ue_ip_node);
+		spin_unlock(&oi->ip_lock);
+		kfree(ue);
 	}
 
-	if (end > i_size_read(inode)) {
+	if (!restart && end > i_size_read(inode)) {
 		ret = ocfs2_set_inode_size(handle, inode, di_bh, end);
 		if (ret < 0)
 			mlog_errno(ret);
 	}
+
 commit:
 	ocfs2_commit_trans(osb, handle);
+free_ac:
+	if (meta_ac) {
+		ocfs2_free_alloc_context(meta_ac);
+		meta_ac = NULL;
+	}
+	if (restart) {

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2379

Quoted 10 lines

fs/ocfs2/aops.c +3 -2

+		restart = 0;
+		goto restart_all;
+	}
 unlock:
 	up_write(&oi->ip_alloc_sem);
 	ocfs2_inode_unlock(inode, 1);
 	brelse(di_bh);
 out:
-	if (data_ac)
-		ocfs2_free_alloc_context(data_ac);

Agree to cleanup the unused data_ac.

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2381

Quoted 2 lines

fs/ocfs2/aops.c +0 -2

-	if (meta_ac)
-		ocfs2_free_alloc_context(meta_ac);

Just move the restart logic down is enough.

If we check the restart here, we will unlock the 'ip_alloc_sem' and release the inode buffer, which we don't need to do.

Thanks,
Alex

Quoted 15 lines

Quoted 4 lines

Quoted 4 lines

Thanks,
Joseph

Quoted 8 lines

Quoted 7 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2436

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

 	ocfs2_run_deallocs(osb, &dealloc);
 	if (locked)
 		inode_unlock(inode);

.

.