Patchwork [v2,1/5] ext4: move ext4_add_groupblocks() to mballoc.c

login
register
mail settings
Submitter Amir G.
Date March 24, 2011, 4:58 p.m.
Message ID <1300985893-4371-2-git-send-email-amir73il@users.sourceforge.net>
Download mbox | patch
Permalink /patch/88240/
State Superseded
Headers show

Comments

Amir G. - March 24, 2011, 4:58 p.m.
From: Amir Goldstein <amir73il@users.sf.net>

In preparation for the next patch, the function ext4_add_groupblocks()
is moved to mballoc.c, where it could use some static functions.

I also fixed a checkpatch warning and replaced obsolete get_undo_access
for bitmap with get_write_access.

Signed-off-by: Amir Goldstein <amir73il@users.sf.net>
---
 fs/ext4/balloc.c  |  124 -----------------------------------------------------
 fs/ext4/mballoc.c |  120 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+), 124 deletions(-)
Theodore Ts'o - May 9, 2011, 2:54 p.m.
On Thu, Mar 24, 2011 at 06:58:09PM +0200, amir73il@users.sourceforge.net wrote:
> From: Amir Goldstein <amir73il@users.sf.net>
> 
> In preparation for the next patch, the function ext4_add_groupblocks()
> is moved to mballoc.c, where it could use some static functions.
> 
> I also fixed a checkpatch warning and replaced obsolete get_undo_access
> for bitmap with get_write_access.
> 
> Signed-off-by: Amir Goldstein <amir73il@users.sf.net>

Please don't move code and make changes in one patch.  #1, it's hard
to review changes that happened in the middle of code movement.  #2,
if there has been any changes in the source function caused by other
patches, I can't regenerate a patch by simply redoing the function
move --- I have to reverse engineer the change that happened under the
cover of code movement, regnerate the patch, and then redo the change.

I've split this into two patches, one which is just a simple code
movement (note that I also moved the function declaration in ext4.h so
it function is listed under the correct .c file), and the second which
changed the use of ext4_journal_get_undo_access to
ext4_journal_get_write_access.  Since this was also the last use of
ext4_journal_get_undo_access(), I also removed the now-unneeded code
in ext4_jbd2.[ch].

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amir G. - May 9, 2011, 4:01 p.m.
On Mon, May 9, 2011 at 5:54 PM, Ted Ts'o <tytso@mit.edu> wrote:
> On Thu, Mar 24, 2011 at 06:58:09PM +0200, amir73il@users.sourceforge.net wrote:
>> From: Amir Goldstein <amir73il@users.sf.net>
>>
>> In preparation for the next patch, the function ext4_add_groupblocks()
>> is moved to mballoc.c, where it could use some static functions.
>>
>> I also fixed a checkpatch warning and replaced obsolete get_undo_access
>> for bitmap with get_write_access.
>>
>> Signed-off-by: Amir Goldstein <amir73il@users.sf.net>
>
> Please don't move code and make changes in one patch.  #1, it's hard
> to review changes that happened in the middle of code movement.  #2,
> if there has been any changes in the source function caused by other
> patches, I can't regenerate a patch by simply redoing the function
> move --- I have to reverse engineer the change that happened under the
> cover of code movement, regnerate the patch, and then redo the change.
>

Sorry for the trouble. At least I (sort of) documented the change in the commit
description, so I hope it wasn't too hard to reverse engineer...
Fixing the checkpatch error I just kind of felt obligated to do, changing
get_undo_access to get_write_access in this patch was just me being lazy.


> I've split this into two patches, one which is just a simple code
> movement (note that I also moved the function declaration in ext4.h so
> it function is listed under the correct .c file), and the second which
> changed the use of ext4_journal_get_undo_access to
> ext4_journal_get_write_access.  Since this was also the last use of
> ext4_journal_get_undo_access(), I also removed the now-unneeded code
> in ext4_jbd2.[ch].
>

Thanks. FYI, in one of the snapshot patches this get_write_access instance is
replaced with get_bitmap_access (which calls a different snapshot hook).
That patch also removes the get_undo_access function, but now you beat
me to it :-)

FYI2, the snapshot patches are waiting in my outbox for me to push send.
When running xfstests I caught a hang in test 225 with 1K blocksize
(all other tests were fine),
so I asked Yongqiang to take a look at it because his patch (6d9c85) had fixed
a problem in test 225. He just said that the hang was caused by a bug
in his patch
and that the hang happen with tytso/master branch and that he is
working on a fix,
so I may just go a head and send out the snapshot patches anyway.



>                                                - Ted
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yongqiang Yang - May 9, 2011, 4:28 p.m.
On Tue, May 10, 2011 at 12:01 AM, Amir G.
<amir73il@users.sourceforge.net> wrote:
> On Mon, May 9, 2011 at 5:54 PM, Ted Ts'o <tytso@mit.edu> wrote:
>> On Thu, Mar 24, 2011 at 06:58:09PM +0200, amir73il@users.sourceforge.net wrote:
>>> From: Amir Goldstein <amir73il@users.sf.net>
>>>
>>> In preparation for the next patch, the function ext4_add_groupblocks()
>>> is moved to mballoc.c, where it could use some static functions.
>>>
>>> I also fixed a checkpatch warning and replaced obsolete get_undo_access
>>> for bitmap with get_write_access.
>>>
>>> Signed-off-by: Amir Goldstein <amir73il@users.sf.net>
>>
>> Please don't move code and make changes in one patch.  #1, it's hard
>> to review changes that happened in the middle of code movement.  #2,
>> if there has been any changes in the source function caused by other
>> patches, I can't regenerate a patch by simply redoing the function
>> move --- I have to reverse engineer the change that happened under the
>> cover of code movement, regnerate the patch, and then redo the change.
>>
>
> Sorry for the trouble. At least I (sort of) documented the change in the commit
> description, so I hope it wasn't too hard to reverse engineer...
> Fixing the checkpatch error I just kind of felt obligated to do, changing
> get_undo_access to get_write_access in this patch was just me being lazy.
>
>
>> I've split this into two patches, one which is just a simple code
>> movement (note that I also moved the function declaration in ext4.h so
>> it function is listed under the correct .c file), and the second which
>> changed the use of ext4_journal_get_undo_access to
>> ext4_journal_get_write_access.  Since this was also the last use of
>> ext4_journal_get_undo_access(), I also removed the now-unneeded code
>> in ext4_jbd2.[ch].
>>
>
> Thanks. FYI, in one of the snapshot patches this get_write_access instance is
> replaced with get_bitmap_access (which calls a different snapshot hook).
> That patch also removes the get_undo_access function, but now you beat
> me to it :-)
>
> FYI2, the snapshot patches are waiting in my outbox for me to push send.
> When running xfstests I caught a hang in test 225 with 1K blocksize
> (all other tests were fine),
> so I asked Yongqiang to take a look at it because his patch (6d9c85) had fixed
> a problem in test 225. He just said that the hang was caused by a bug
> in his patch
> and that the hang happen with tytso/master branch and that he is
> working on a fix,
> so I may just go a head and send out the snapshot patches anyway.

Fixed.   You can send snapshot patches out.
>
>
>
>>                                                - Ted
>>
>

Patch

diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index adf96b8..1288f80 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -359,130 +359,6 @@  ext4_read_block_bitmap(struct super_block *sb, ext4_group_t block_group)
 }
 
 /**
- * ext4_add_groupblocks() -- Add given blocks to an existing group
- * @handle:			handle to this transaction
- * @sb:				super block
- * @block:			start physcial block to add to the block group
- * @count:			number of blocks to free
- *
- * This marks the blocks as free in the bitmap. We ask the
- * mballoc to reload the buddy after this by setting group
- * EXT4_GROUP_INFO_NEED_INIT_BIT flag
- */
-void ext4_add_groupblocks(handle_t *handle, struct super_block *sb,
-			 ext4_fsblk_t block, unsigned long count)
-{
-	struct buffer_head *bitmap_bh = NULL;
-	struct buffer_head *gd_bh;
-	ext4_group_t block_group;
-	ext4_grpblk_t bit;
-	unsigned int i;
-	struct ext4_group_desc *desc;
-	struct ext4_sb_info *sbi = EXT4_SB(sb);
-	int err = 0, ret, blk_free_count;
-	ext4_grpblk_t blocks_freed;
-	struct ext4_group_info *grp;
-
-	ext4_debug("Adding block(s) %llu-%llu\n", block, block + count - 1);
-
-	ext4_get_group_no_and_offset(sb, block, &block_group, &bit);
-	grp = ext4_get_group_info(sb, block_group);
-	/*
-	 * Check to see if we are freeing blocks across a group
-	 * boundary.
-	 */
-	if (bit + count > EXT4_BLOCKS_PER_GROUP(sb)) {
-		goto error_return;
-	}
-	bitmap_bh = ext4_read_block_bitmap(sb, block_group);
-	if (!bitmap_bh)
-		goto error_return;
-	desc = ext4_get_group_desc(sb, block_group, &gd_bh);
-	if (!desc)
-		goto error_return;
-
-	if (in_range(ext4_block_bitmap(sb, desc), block, count) ||
-	    in_range(ext4_inode_bitmap(sb, desc), block, count) ||
-	    in_range(block, ext4_inode_table(sb, desc), sbi->s_itb_per_group) ||
-	    in_range(block + count - 1, ext4_inode_table(sb, desc),
-		     sbi->s_itb_per_group)) {
-		ext4_error(sb, "Adding blocks in system zones - "
-			   "Block = %llu, count = %lu",
-			   block, count);
-		goto error_return;
-	}
-
-	/*
-	 * We are about to add blocks to the bitmap,
-	 * so we need undo access.
-	 */
-	BUFFER_TRACE(bitmap_bh, "getting undo access");
-	err = ext4_journal_get_undo_access(handle, bitmap_bh);
-	if (err)
-		goto error_return;
-
-	/*
-	 * We are about to modify some metadata.  Call the journal APIs
-	 * to unshare ->b_data if a currently-committing transaction is
-	 * using it
-	 */
-	BUFFER_TRACE(gd_bh, "get_write_access");
-	err = ext4_journal_get_write_access(handle, gd_bh);
-	if (err)
-		goto error_return;
-	/*
-	 * make sure we don't allow a parallel init on other groups in the
-	 * same buddy cache
-	 */
-	down_write(&grp->alloc_sem);
-	for (i = 0, blocks_freed = 0; i < count; i++) {
-		BUFFER_TRACE(bitmap_bh, "clear bit");
-		if (!ext4_clear_bit_atomic(ext4_group_lock_ptr(sb, block_group),
-						bit + i, bitmap_bh->b_data)) {
-			ext4_error(sb, "bit already cleared for block %llu",
-				   (ext4_fsblk_t)(block + i));
-			BUFFER_TRACE(bitmap_bh, "bit already cleared");
-		} else {
-			blocks_freed++;
-		}
-	}
-	ext4_lock_group(sb, block_group);
-	blk_free_count = blocks_freed + ext4_free_blks_count(sb, desc);
-	ext4_free_blks_set(sb, desc, blk_free_count);
-	desc->bg_checksum = ext4_group_desc_csum(sbi, block_group, desc);
-	ext4_unlock_group(sb, block_group);
-	percpu_counter_add(&sbi->s_freeblocks_counter, blocks_freed);
-
-	if (sbi->s_log_groups_per_flex) {
-		ext4_group_t flex_group = ext4_flex_group(sbi, block_group);
-		atomic_add(blocks_freed,
-			   &sbi->s_flex_groups[flex_group].free_blocks);
-	}
-	/*
-	 * request to reload the buddy with the
-	 * new bitmap information
-	 */
-	set_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, &(grp->bb_state));
-	grp->bb_free += blocks_freed;
-	up_write(&grp->alloc_sem);
-
-	/* We dirtied the bitmap block */
-	BUFFER_TRACE(bitmap_bh, "dirtied bitmap block");
-	err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
-
-	/* And the group descriptor block */
-	BUFFER_TRACE(gd_bh, "dirtied group descriptor block");
-	ret = ext4_handle_dirty_metadata(handle, NULL, gd_bh);
-	if (!err)
-		err = ret;
-
-error_return:
-	brelse(bitmap_bh);
-	ext4_std_error(sb, err);
-	return;
-}
-
-/**
  * ext4_has_free_blocks()
  * @sbi:	in-core super block structure.
  * @nblocks:	number of needed blocks
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 2f6f0dd..1e3c826 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4704,6 +4704,126 @@  error_return:
 }
 
 /**
+ * ext4_add_groupblocks() -- Add given blocks to an existing group
+ * @handle:			handle to this transaction
+ * @sb:				super block
+ * @block:			start physcial block to add to the block group
+ * @count:			number of blocks to free
+ *
+ * This marks the blocks as free in the bitmap. We ask the
+ * mballoc to reload the buddy after this by setting group
+ * EXT4_GROUP_INFO_NEED_INIT_BIT flag
+ */
+void ext4_add_groupblocks(handle_t *handle, struct super_block *sb,
+			 ext4_fsblk_t block, unsigned long count)
+{
+	struct buffer_head *bitmap_bh = NULL;
+	struct buffer_head *gd_bh;
+	ext4_group_t block_group;
+	ext4_grpblk_t bit;
+	unsigned int i;
+	struct ext4_group_desc *desc;
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	int err = 0, ret, blk_free_count;
+	ext4_grpblk_t blocks_freed;
+	struct ext4_group_info *grp;
+
+	ext4_debug("Adding block(s) %llu-%llu\n", block, block + count - 1);
+
+	ext4_get_group_no_and_offset(sb, block, &block_group, &bit);
+	grp = ext4_get_group_info(sb, block_group);
+	/*
+	 * Check to see if we are freeing blocks across a group
+	 * boundary.
+	 */
+	if (bit + count > EXT4_BLOCKS_PER_GROUP(sb))
+		goto error_return;
+
+	bitmap_bh = ext4_read_block_bitmap(sb, block_group);
+	if (!bitmap_bh)
+		goto error_return;
+	desc = ext4_get_group_desc(sb, block_group, &gd_bh);
+	if (!desc)
+		goto error_return;
+
+	if (in_range(ext4_block_bitmap(sb, desc), block, count) ||
+	    in_range(ext4_inode_bitmap(sb, desc), block, count) ||
+	    in_range(block, ext4_inode_table(sb, desc), sbi->s_itb_per_group) ||
+	    in_range(block + count - 1, ext4_inode_table(sb, desc),
+		     sbi->s_itb_per_group)) {
+		ext4_error(sb, "Adding blocks in system zones - "
+			   "Block = %llu, count = %lu",
+			   block, count);
+		goto error_return;
+	}
+
+	BUFFER_TRACE(bitmap_bh, "getting write access");
+	err = ext4_journal_get_write_access(handle, bitmap_bh);
+	if (err)
+		goto error_return;
+
+	/*
+	 * We are about to modify some metadata.  Call the journal APIs
+	 * to unshare ->b_data if a currently-committing transaction is
+	 * using it
+	 */
+	BUFFER_TRACE(gd_bh, "get_write_access");
+	err = ext4_journal_get_write_access(handle, gd_bh);
+	if (err)
+		goto error_return;
+	/*
+	 * make sure we don't allow a parallel init on other groups in the
+	 * same buddy cache
+	 */
+	down_write(&grp->alloc_sem);
+	for (i = 0, blocks_freed = 0; i < count; i++) {
+		BUFFER_TRACE(bitmap_bh, "clear bit");
+		if (!ext4_clear_bit_atomic(ext4_group_lock_ptr(sb, block_group),
+						bit + i, bitmap_bh->b_data)) {
+			ext4_error(sb, "bit already cleared for block %llu",
+				   (ext4_fsblk_t)(block + i));
+			BUFFER_TRACE(bitmap_bh, "bit already cleared");
+		} else {
+			blocks_freed++;
+		}
+	}
+	ext4_lock_group(sb, block_group);
+	blk_free_count = blocks_freed + ext4_free_blks_count(sb, desc);
+	ext4_free_blks_set(sb, desc, blk_free_count);
+	desc->bg_checksum = ext4_group_desc_csum(sbi, block_group, desc);
+	ext4_unlock_group(sb, block_group);
+	percpu_counter_add(&sbi->s_freeblocks_counter, blocks_freed);
+
+	if (sbi->s_log_groups_per_flex) {
+		ext4_group_t flex_group = ext4_flex_group(sbi, block_group);
+		atomic_add(blocks_freed,
+			   &sbi->s_flex_groups[flex_group].free_blocks);
+	}
+	/*
+	 * request to reload the buddy with the
+	 * new bitmap information
+	 */
+	set_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, &(grp->bb_state));
+	grp->bb_free += blocks_freed;
+	up_write(&grp->alloc_sem);
+
+	/* We dirtied the bitmap block */
+	BUFFER_TRACE(bitmap_bh, "dirtied bitmap block");
+	err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
+
+	/* And the group descriptor block */
+	BUFFER_TRACE(gd_bh, "dirtied group descriptor block");
+	ret = ext4_handle_dirty_metadata(handle, NULL, gd_bh);
+	if (!err)
+		err = ret;
+
+error_return:
+	brelse(bitmap_bh);
+	ext4_std_error(sb, err);
+	return;
+}
+
+/**
  * ext4_trim_extent -- function to TRIM one single free extent in the group
  * @sb:		super block for the file system
  * @start:	starting block of the free extent in the alloc. group