Ocfs2 devel - Mar 2014 - [PATCH] Ocfs2: Rollback alloc_dinode counts when ocfs2_block_group_set_bits() failed

After updating alloc_dinode counts in ocfs2_alloc_dinode_update_counts(),
if ocfs2_alloc_dinode_update_bitmap() failed, there is a rare case that some
space may be lost.
  So, we rollback alloc_dinode counts when ocfs2_block_group_set_bits() failed.

Signed-off-by: Younger Liu <younger.liucn at gmail.com>
Cc: Mark Fasheh <mfasheh at suse.de>
---
 fs/ocfs2/move_extents.c |    5 ++++-
 fs/ocfs2/suballoc.c     |   24 +++++++++++++++++++++++-
 fs/ocfs2/suballoc.h     |    4 ++++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c
index 64c304d..65483d0 100644
--- a/fs/ocfs2/move_extents.c
+++ b/fs/ocfs2/move_extents.c
@@ -690,8 +690,11 @@ static int ocfs2_move_extent(struct
ocfs2_move_extents_context *context,
 
 	ret = ocfs2_block_group_set_bits(handle, gb_inode, gd, gd_bh,
 					 goal_bit, len);
-	if (ret)
+	if (ret) {
+		ocfs2_rollback_alloc_dinode_counts(gb_inode, gb_bh, len,
+					       le16_to_cpu(gd->bg_chain));
 		mlog_errno(ret);
+	}
 
 	/*
 	 * Here we should write the new page out first if we are
diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
index 47ae266..150d94f 100644
--- a/fs/ocfs2/suballoc.c
+++ b/fs/ocfs2/suballoc.c
@@ -1607,6 +1607,20 @@ out:
 	return ret;
 }
 
+void ocfs2_rollback_alloc_dinode_counts(struct inode *inode,
+				       struct buffer_head *di_bh,
+				       u32 num_bits,
+				       u16 chain)
+{
+	u32 tmp_used;
+	struct ocfs2_dinode *di = (struct ocfs2_dinode *) di_bh->b_data;
+	struct ocfs2_chain_list *cl = (struct ocfs2_chain_list *)
&di->id2.i_chain;
+
+	tmp_used = le32_to_cpu(di->id1.bitmap1.i_used);
+	di->id1.bitmap1.i_used = cpu_to_le32(tmp_used - num_bits);
+	le32_add_cpu(&cl->cl_recs[chain].c_free, num_bits);
+}
+
 static int ocfs2_bg_discontig_fix_by_rec(struct ocfs2_suballoc_result *res,
 					 struct ocfs2_extent_rec *rec,
 					 struct ocfs2_chain_list *cl)
@@ -1707,8 +1721,12 @@ static int ocfs2_search_one_group(struct
ocfs2_alloc_context *ac,
 
 	ret = ocfs2_block_group_set_bits(handle, alloc_inode, gd, group_bh,
 					 res->sr_bit_offset, res->sr_bits);
-	if (ret < 0)
+	if (ret < 0) {
+		ocfs2_rollback_alloc_dinode_counts(alloc_inode, ac->ac_bh,
+					       res->sr_bits,
+					       le16_to_cpu(gd->bg_chain));
 		mlog_errno(ret);
+	}
 
 out_loc_only:
 	*bits_left = le16_to_cpu(gd->bg_free_bits_count);
@@ -1838,6 +1856,8 @@ static int ocfs2_search_chain(struct ocfs2_alloc_context
*ac,
 					    res->sr_bit_offset,
 					    res->sr_bits);
 	if (status < 0) {
+		ocfs2_rollback_alloc_dinode_counts(alloc_inode,
+					ac->ac_bh, res->sr_bits, chain);
 		mlog_errno(status);
 		goto bail;
 	}
@@ -2149,6 +2169,8 @@ int ocfs2_claim_new_inode_at_loc(handle_t *handle,
 					 res->sr_bit_offset,
 					 res->sr_bits);
 	if (ret < 0) {
+		ocfs2_rollback_alloc_dinode_counts(ac->ac_inode,
+					       ac->ac_bh, res->sr_bits, chain);
 		mlog_errno(ret);
 		goto out;
 	}
diff --git a/fs/ocfs2/suballoc.h b/fs/ocfs2/suballoc.h
index 218d803..2d25017 100644
--- a/fs/ocfs2/suballoc.h
+++ b/fs/ocfs2/suballoc.h
@@ -91,6 +91,10 @@ int ocfs2_alloc_dinode_update_counts(struct inode *inode,
 			 struct buffer_head *di_bh,
 			 u32 num_bits,
 			 u16 chain);
+void ocfs2_rollback_alloc_dinode_counts(struct inode *inode,
+			 struct buffer_head *di_bh,
+			 u32 num_bits,
+			 u16 chain);
 int ocfs2_block_group_set_bits(handle_t *handle,
 			 struct inode *alloc_inode,
 			 struct ocfs2_group_desc *bg,
-- 
1.7.9.5

On Mon, 31 Mar 2014 21:07:40 +0800 Younger Liu <younger.liucn at
gmail.com> wrote:
>   After updating alloc_dinode counts in ocfs2_alloc_dinode_update_counts(),
> if ocfs2_alloc_dinode_update_bitmap() failed, there is a rare case that
some
> space may be lost.
>   So, we rollback alloc_dinode counts when ocfs2_block_group_set_bits()
failed.
This patch wildly conflicts with your earlier patch
ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously.patch.
 below.

What should be done?


From: Younger Liu <younger.liucn at gmail.com>
Subject: ocfs2: alloc_dinode counts and group bitmap should be update
simultaneously

Updating alloc_dinode counts in ocfs2_alloc_dinode_update_counts() and
setting group bitmap in ocfs2_alloc_dinode_update_bitmap() have to be done
simultaneously.  Two cases are as follow:

(1) If ocfs2_alloc_dinode_update_counts() fails, there is no need to
    set group bitmap.  This case has been considered.

(2) If ocfs2_alloc_dinode_update_bitmap() fails, alloc_dinode counts
    should be rolled back.  Otherwise, some clusters would never be used. 
    This case is not considered.

So, we combine two functions, and ensure simultaneity.

Signed-off-by: Younger Liu <younger.liucn at gmail.com>
Cc: Joel Becker <jlbec at evilplan.org>
Cc: Mark Fasheh <mfasheh at suse.com>
Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
---

 fs/ocfs2/move_extents.c |   11 --
 fs/ocfs2/ocfs2_trace.h  |    2 
 fs/ocfs2/suballoc.c     |  155 +++++++++++++++-----------------------
 fs/ocfs2/suballoc.h     |   21 +----
 4 files changed, 77 insertions(+), 112 deletions(-)

diff -puN
fs/ocfs2/move_extents.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously
fs/ocfs2/move_extents.c
---
a/fs/ocfs2/move_extents.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously
+++ a/fs/ocfs2/move_extents.c
@@ -681,18 +681,15 @@ static int ocfs2_move_extent(struct ocfs
 	}
 
 	gd = (struct ocfs2_group_desc *)gd_bh->b_data;
-	ret = ocfs2_alloc_dinode_update_counts(gb_inode, handle, gb_bh, len,
-					       le16_to_cpu(gd->bg_chain));
+	ret = ocfs2_alloc_dinode_update_bitmap(handle,
+				gb_inode, gb_bh, gd, gb_bh,
+				le16_to_cpu(gd->bg_chain),
+				goal_bit, len);
 	if (ret) {
 		mlog_errno(ret);
 		goto out_commit;
 	}
 
-	ret = ocfs2_block_group_set_bits(handle, gb_inode, gd, gd_bh,
-					 goal_bit, len);
-	if (ret)
-		mlog_errno(ret);
-
 	/*
 	 * Here we should write the new page out first if we are
 	 * in write-back mode.
diff -puN
fs/ocfs2/ocfs2_trace.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously
fs/ocfs2/ocfs2_trace.h
---
a/fs/ocfs2/ocfs2_trace.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously
+++ a/fs/ocfs2/ocfs2_trace.h
@@ -788,7 +788,7 @@ DEFINE_OCFS2_UINT_UINT_UINT_EVENT(ocfs2_
 
 DEFINE_OCFS2_ULL_EVENT(ocfs2_reserve_new_inode_new_group);
 
-DEFINE_OCFS2_UINT_UINT_EVENT(ocfs2_block_group_set_bits);
+DEFINE_OCFS2_UINT_UINT_EVENT(ocfs2_alloc_dinode_update_bitmap);
 
 TRACE_EVENT(ocfs2_relink_block_group,
 	TP_PROTO(unsigned long long i_blkno, unsigned int chain,
diff -puN
fs/ocfs2/suballoc.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously
fs/ocfs2/suballoc.c
---
a/fs/ocfs2/suballoc.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously
+++ a/fs/ocfs2/suballoc.c
@@ -1337,54 +1337,6 @@ static int ocfs2_block_group_find_clear_
 	return status;
 }
 
-int ocfs2_block_group_set_bits(handle_t *handle,
-					     struct inode *alloc_inode,
-					     struct ocfs2_group_desc *bg,
-					     struct buffer_head *group_bh,
-					     unsigned int bit_off,
-					     unsigned int num_bits)
-{
-	int status;
-	void *bitmap = bg->bg_bitmap;
-	int journal_type = OCFS2_JOURNAL_ACCESS_WRITE;
-
-	/* All callers get the descriptor via
-	 * ocfs2_read_group_descriptor().  Any corruption is a code bug. */
-	BUG_ON(!OCFS2_IS_VALID_GROUP_DESC(bg));
-	BUG_ON(le16_to_cpu(bg->bg_free_bits_count) < num_bits);
-
-	trace_ocfs2_block_group_set_bits(bit_off, num_bits);
-
-	if (ocfs2_is_cluster_bitmap(alloc_inode))
-		journal_type = OCFS2_JOURNAL_ACCESS_UNDO;
-
-	status = ocfs2_journal_access_gd(handle,
-					 INODE_CACHE(alloc_inode),
-					 group_bh,
-					 journal_type);
-	if (status < 0) {
-		mlog_errno(status);
-		goto bail;
-	}
-
-	le16_add_cpu(&bg->bg_free_bits_count, -num_bits);
-	if (le16_to_cpu(bg->bg_free_bits_count) > le16_to_cpu(bg->bg_bits)) {
-		ocfs2_error(alloc_inode->i_sb, "Group descriptor # %llu has bit"
-			    " count %u but claims %u are freed. num_bits %d",
-			    (unsigned long long)le64_to_cpu(bg->bg_blkno),
-			    le16_to_cpu(bg->bg_bits),
-			    le16_to_cpu(bg->bg_free_bits_count), num_bits);
-		return -EROFS;
-	}
-	while(num_bits--)
-		ocfs2_set_bit(bit_off++, bitmap);
-
-	ocfs2_journal_dirty(handle, group_bh);
-
-bail:
-	return status;
-}
-
 /* find the one with the most empty bits */
 static inline u16 ocfs2_find_victim_chain(struct ocfs2_chain_list *cl)
 {
@@ -1580,31 +1532,78 @@ static int ocfs2_block_group_search(stru
 	return ret;
 }
 
-int ocfs2_alloc_dinode_update_counts(struct inode *inode,
-				       handle_t *handle,
-				       struct buffer_head *di_bh,
-				       u32 num_bits,
-				       u16 chain)
+int ocfs2_alloc_dinode_update_bitmap(handle_t *handle,
+				struct inode *alloc_inode,
+				struct buffer_head *di_bh,
+				struct ocfs2_group_desc *bg,
+				struct buffer_head *group_bh,
+				u16 chain, u32 bit_off, u32 num_bits)
 {
 	int ret;
 	u32 tmp_used;
 	struct ocfs2_dinode *di = (struct ocfs2_dinode *) di_bh->b_data;
 	struct ocfs2_chain_list *cl = (struct ocfs2_chain_list *)
&di->id2.i_chain;
+	void *bitmap = bg->bg_bitmap;
+	int journal_type = OCFS2_JOURNAL_ACCESS_WRITE;
 
-	ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh,
-				      OCFS2_JOURNAL_ACCESS_WRITE);
+	/*
+	 * All callers get the descriptor via
+	 * ocfs2_read_group_descriptor().  Any corruption is a code bug.
+	 */
+	BUG_ON(!OCFS2_IS_VALID_GROUP_DESC(bg));
+	BUG_ON(le16_to_cpu(bg->bg_free_bits_count) < num_bits);
+
+	trace_ocfs2_alloc_dinode_update_bitmap(bit_off, num_bits);
+
+	ret = ocfs2_journal_access_di(handle,
+			INODE_CACHE(alloc_inode), di_bh, journal_type);
 	if (ret < 0) {
 		mlog_errno(ret);
 		goto out;
 	}
 
+	if (ocfs2_is_cluster_bitmap(alloc_inode))
+		journal_type = OCFS2_JOURNAL_ACCESS_UNDO;
+
+	ret = ocfs2_journal_access_gd(handle,
+			INODE_CACHE(alloc_inode), group_bh, journal_type);
+	if (ret < 0) {
+		mlog_errno(ret);
+		goto out;
+	}
+
+	/* update alloc_dinode counts */
 	tmp_used = le32_to_cpu(di->id1.bitmap1.i_used);
 	di->id1.bitmap1.i_used = cpu_to_le32(num_bits + tmp_used);
 	le32_add_cpu(&cl->cl_recs[chain].c_free, -num_bits);
+
+	/* update bg counts and bitmap*/
+	le16_add_cpu(&bg->bg_free_bits_count, -num_bits);
+	if (le16_to_cpu(bg->bg_free_bits_count) > le16_to_cpu(bg->bg_bits)) {
+		ocfs2_error(alloc_inode->i_sb, "Group descriptor # %llu has bit"
+			" count %u but claims %u are freed. num_bits %d",
+			(unsigned long long)le64_to_cpu(bg->bg_blkno),
+			le16_to_cpu(bg->bg_bits),
+			le16_to_cpu(bg->bg_free_bits_count), num_bits);
+		ret = -EROFS;
+		goto out_rollback;
+	}
+	while (num_bits--)
+		ocfs2_set_bit(bit_off++, bitmap);
+
 	ocfs2_journal_dirty(handle, di_bh);
+	ocfs2_journal_dirty(handle, group_bh);
 
 out:
 	return ret;
+
+out_rollback:
+	le16_add_cpu(&bg->bg_free_bits_count, num_bits);
+
+	di->id1.bitmap1.i_used = cpu_to_le32(tmp_used - num_bits);
+	le32_add_cpu(&cl->cl_recs[chain].c_free, num_bits);
+
+	return ret;
 }
 
 static int ocfs2_bg_discontig_fix_by_rec(struct ocfs2_suballoc_result *res,
@@ -1697,19 +1696,15 @@ static int ocfs2_search_one_group(struct
 	if (ac->ac_find_loc_only)
 		goto out_loc_only;
 
-	ret = ocfs2_alloc_dinode_update_counts(alloc_inode, handle, ac->ac_bh,
-					       res->sr_bits,
-					       le16_to_cpu(gd->bg_chain));
+	ret = ocfs2_alloc_dinode_update_bitmap(handle,
+			alloc_inode, ac->ac_bh, gd, group_bh,
+			le16_to_cpu(gd->bg_chain),
+			res->sr_bit_offset, res->sr_bits);
 	if (ret < 0) {
 		mlog_errno(ret);
 		goto out;
 	}
 
-	ret = ocfs2_block_group_set_bits(handle, alloc_inode, gd, group_bh,
-					 res->sr_bit_offset, res->sr_bits);
-	if (ret < 0)
-		mlog_errno(ret);
-
 out_loc_only:
 	*bits_left = le16_to_cpu(gd->bg_free_bits_count);
 
@@ -1823,20 +1818,9 @@ static int ocfs2_search_chain(struct ocf
 	if (ac->ac_find_loc_only)
 		goto out_loc_only;
 
-	status = ocfs2_alloc_dinode_update_counts(alloc_inode, handle,
-						  ac->ac_bh, res->sr_bits,
-						  chain);
-	if (status) {
-		mlog_errno(status);
-		goto bail;
-	}
-
-	status = ocfs2_block_group_set_bits(handle,
-					    alloc_inode,
-					    bg,
-					    group_bh,
-					    res->sr_bit_offset,
-					    res->sr_bits);
+	status = ocfs2_alloc_dinode_update_bitmap(handle,
+			alloc_inode, ac->ac_bh, bg, group_bh,
+			chain, res->sr_bit_offset, res->sr_bits);
 	if (status < 0) {
 		mlog_errno(status);
 		goto bail;
@@ -2134,20 +2118,9 @@ int ocfs2_claim_new_inode_at_loc(handle_
 	bg = (struct ocfs2_group_desc *) bg_bh->b_data;
 	chain = le16_to_cpu(bg->bg_chain);
 
-	ret = ocfs2_alloc_dinode_update_counts(ac->ac_inode, handle,
-					       ac->ac_bh, res->sr_bits,
-					       chain);
-	if (ret) {
-		mlog_errno(ret);
-		goto out;
-	}
-
-	ret = ocfs2_block_group_set_bits(handle,
-					 ac->ac_inode,
-					 bg,
-					 bg_bh,
-					 res->sr_bit_offset,
-					 res->sr_bits);
+	ret = ocfs2_alloc_dinode_update_bitmap(handle,
+			ac->ac_inode, ac->ac_bh, bg, bg_bh,
+			chain, res->sr_bit_offset, res->sr_bits);
 	if (ret < 0) {
 		mlog_errno(ret);
 		goto out;
diff -puN
fs/ocfs2/suballoc.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously
fs/ocfs2/suballoc.h
---
a/fs/ocfs2/suballoc.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously
+++ a/fs/ocfs2/suballoc.h
@@ -85,19 +85,14 @@ int ocfs2_reserve_new_inode(struct ocfs2
 int ocfs2_reserve_clusters(struct ocfs2_super *osb,
 			   u32 bits_wanted,
 			   struct ocfs2_alloc_context **ac);
-
-int ocfs2_alloc_dinode_update_counts(struct inode *inode,
-			 handle_t *handle,
-			 struct buffer_head *di_bh,
-			 u32 num_bits,
-			 u16 chain);
-int ocfs2_block_group_set_bits(handle_t *handle,
-			 struct inode *alloc_inode,
-			 struct ocfs2_group_desc *bg,
-			 struct buffer_head *group_bh,
-			 unsigned int bit_off,
-			 unsigned int num_bits);
-
+int ocfs2_alloc_dinode_update_bitmap(handle_t *handle,
+				struct inode *alloc_inode,
+				struct buffer_head *di_bh,
+				struct ocfs2_group_desc *bg,
+				struct buffer_head *group_bh,
+				u16 chain,
+				u32 bit_off,
+				u32 num_bits);
 int ocfs2_claim_metadata(handle_t *handle,
 			 struct ocfs2_alloc_context *ac,
 			 u32 bits_wanted,
_

On Mon, Mar 31, 2014 at 09:07:40PM +0800, Younger Liu
wrote:
>   After updating alloc_dinode counts in ocfs2_alloc_dinode_update_counts(),
> if ocfs2_alloc_dinode_update_bitmap() failed, there is a rare case that
some
> space may be lost.
>   So, we rollback alloc_dinode counts when ocfs2_block_group_set_bits()
failed.
> 
> Signed-off-by: Younger Liu <younger.liucn at gmail.com>
> Cc: Mark Fasheh <mfasheh at suse.de>
Thanks for the latest version fo this patch Younger, it looks good.
	--Mark

Reviewed-by: Mark Fasheh <mfasheh at suse.de>

--
Mark Fasheh