Skip to contentSkip to conversation

ocfs2: alloc_dinode counts and group bitmap should be update simultaneously

PATCH New akpm at linux-foundation.org 3 participants 3 messages Mar 19, 2014 – Mar 31, 2014 OCFS2

lore
Share & tools
lore

akpm at linux-foundation.org

author
#0 lore raw

PATCH 1/8 ocfs2: alloc_dinode counts and group bitmap should be update simultaneously

From: Younger Liu <[email protected]>
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 <[email protected]>
Cc Joel Becker <[email protected]>
Cc Mark Fasheh <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

---

 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(-)
fs/ocfs2/move_extents.c renamed from fs/ocfs2/move_extents.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously +4 -7
diff -puN fs/ocfs2/move_extents.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously 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.
fs/ocfs2/ocfs2_trace.h renamed from fs/ocfs2/ocfs2_trace.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously +1 -1
diff -puN fs/ocfs2/ocfs2_trace.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously 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,
fs/ocfs2/suballoc.c renamed from fs/ocfs2/suballoc.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously +64 -91

208 lines not on this page · raw message

fs/ocfs2/suballoc.h renamed from fs/ocfs2/suballoc.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously +8 -13
diff -puN fs/ocfs2/suballoc.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously 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,

_

#1 lore raw

PATCH 1/8 ocfs2: alloc_dinode counts and group bitmap should be update simultaneously

Quoted 7 lines · Andrew Morton

From: Younger Liu <[email protected]>
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:

Quoted 8 lines

(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.

By 'ocfs2_alloc_dinode_update_bitmap' above, you mean ocfs2_block_group_set_bits() correct? I am going under that assumption.

Anyway, I believe you're trying to fix a rare case where we might leak some space in that our chain counts might be a bit high. Basically the call to journal_access() in ocfs2_block_group_set_bits() has to fail. To be honest, this has existed for a while and hasn't ever been an issue.

That said, we can fix it but I don't like that your approach changes core bitmap handling code. Can you please do this by writing a simple fallback function to set the correct values on the bitmap dinode? Have that called in case of error from ocfs2_block_group_set_bits(). It doesn't need to make journal calls because that has already been set up for it so this is literally a couple lines of code.

If you like, you can take the pattern of:

call ocfs2_alloc_dinode_update_counts()
call ocfs2_block_group_set_bits()
if (error)
call ocfs2_rollback_alloc_dinode_counts()
...

And put that into a master function.

Thanks,

	--Mark

--
Mark Fasheh

#2 lore raw

PATCH 1/8 ocfs2: alloc_dinode counts and group bitmap should be update simultaneously

Quoted 35 lines · Mark Fasheh
Quoted 7 lines · Andrew Morton

From: Younger Liu <[email protected]>
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:

Quoted 8 lines

(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.

By 'ocfs2_alloc_dinode_update_bitmap' above, you mean ocfs2_block_group_set_bits() correct? I am going under that assumption.

Anyway, I believe you're trying to fix a rare case where we might leak some space in that our chain counts might be a bit high. Basically the call to journal_access() in ocfs2_block_group_set_bits() has to fail. To be honest, this has existed for a while and hasn't ever been an issue.

That said, we can fix it but I don't like that your approach changes core bitmap handling code. Can you please do this by writing a simple fallback function to set the correct values on the bitmap dinode? Have that called in case of error from ocfs2_block_group_set_bits(). It doesn't need to make journal calls because that has already been set up for it so this is literally a couple lines of code.

If you like, you can take the pattern of:

Quoted 8 lines

call ocfs2_alloc_dinode_update_counts()
call ocfs2_block_group_set_bits()
if (error)
call ocfs2_rollback_alloc_dinode_counts()
...

And put that into a master function.

Hi Mark,
Thanks for your review.
It is not a good idea to change core code. I will resend a patch in a
monent.
Younger

Quoted 8 lines

Thanks,

	--Mark

--
Mark Fasheh