2022-02-01 20:42:01

by Ritesh Harjani

[permalink] [raw]
Subject: [RFC 5/6] ext4: Refactor ext4_free_blocks() to pull out ext4_mb_clear_bb()

ext4_free_blocks() function became too long and confusing, this patch
just pulls out the ext4_mb_clear_bb() function logic from it
which clears the block bitmap and frees it.

No functionality change in this patch

Signed-off-by: Ritesh Harjani <[email protected]>
---
fs/ext4/mballoc.c | 180 ++++++++++++++++++++++++++--------------------
1 file changed, 102 insertions(+), 78 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 2f931575e6c2..5f20e355d08c 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5870,7 +5870,8 @@ static void ext4_free_blocks_simple(struct inode *inode, ext4_fsblk_t block,
}

/**
- * ext4_free_blocks() -- Free given blocks and update quota
+ * ext4_mb_clear_bb() -- helper function for freeing blocks.
+ * Used by ext4_free_blocks()
* @handle: handle for this transaction
* @inode: inode
* @bh: optional buffer of the block to be freed
@@ -5878,9 +5879,9 @@ static void ext4_free_blocks_simple(struct inode *inode, ext4_fsblk_t block,
* @count: number of blocks to be freed
* @flags: flags used by ext4_free_blocks
*/
-void ext4_free_blocks(handle_t *handle, struct inode *inode,
- struct buffer_head *bh, ext4_fsblk_t block,
- unsigned long count, int flags)
+static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
+ ext4_fsblk_t block, unsigned long count,
+ int flags)
{
struct buffer_head *bitmap_bh = NULL;
struct super_block *sb = inode->i_sb;
@@ -5897,80 +5898,6 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,

sbi = EXT4_SB(sb);

- if (sbi->s_mount_state & EXT4_FC_REPLAY) {
- ext4_free_blocks_simple(inode, block, count);
- return;
- }
-
- might_sleep();
- if (bh) {
- if (block)
- BUG_ON(block != bh->b_blocknr);
- else
- block = bh->b_blocknr;
- }
-
- if (!(flags & EXT4_FREE_BLOCKS_VALIDATED) &&
- !ext4_inode_block_valid(inode, block, count)) {
- ext4_error(sb, "Freeing blocks not in datazone - "
- "block = %llu, count = %lu", block, count);
- goto error_return;
- }
-
- ext4_debug("freeing block %llu\n", block);
- trace_ext4_free_blocks(inode, block, count, flags);
-
- if (bh && (flags & EXT4_FREE_BLOCKS_FORGET)) {
- BUG_ON(count > 1);
-
- ext4_forget(handle, flags & EXT4_FREE_BLOCKS_METADATA,
- inode, bh, block);
- }
-
- /*
- * If the extent to be freed does not begin on a cluster
- * boundary, we need to deal with partial clusters at the
- * beginning and end of the extent. Normally we will free
- * blocks at the beginning or the end unless we are explicitly
- * requested to avoid doing so.
- */
- overflow = EXT4_PBLK_COFF(sbi, block);
- if (overflow) {
- if (flags & EXT4_FREE_BLOCKS_NOFREE_FIRST_CLUSTER) {
- overflow = sbi->s_cluster_ratio - overflow;
- block += overflow;
- if (count > overflow)
- count -= overflow;
- else
- return;
- } else {
- block -= overflow;
- count += overflow;
- }
- }
- overflow = EXT4_LBLK_COFF(sbi, count);
- if (overflow) {
- if (flags & EXT4_FREE_BLOCKS_NOFREE_LAST_CLUSTER) {
- if (count > overflow)
- count -= overflow;
- else
- return;
- } else
- count += sbi->s_cluster_ratio - overflow;
- }
-
- if (!bh && (flags & EXT4_FREE_BLOCKS_FORGET)) {
- int i;
- int is_metadata = flags & EXT4_FREE_BLOCKS_METADATA;
-
- for (i = 0; i < count; i++) {
- cond_resched();
- if (is_metadata)
- bh = sb_find_get_block(inode->i_sb, block + i);
- ext4_forget(handle, is_metadata, inode, bh, block + i);
- }
- }
-
do_more:
overflow = 0;
ext4_get_group_no_and_offset(sb, block, &block_group, &bit);
@@ -6132,6 +6059,103 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
return;
}

+/**
+ * ext4_free_blocks() -- Free given blocks and update quota
+ * @handle: handle for this transaction
+ * @inode: inode
+ * @bh: optional buffer of the block to be freed
+ * @block: starting physical block to be freed
+ * @count: number of blocks to be freed
+ * @flags: flags used by ext4_free_blocks
+ */
+void ext4_free_blocks(handle_t *handle, struct inode *inode,
+ struct buffer_head *bh, ext4_fsblk_t block,
+ unsigned long count, int flags)
+{
+ struct super_block *sb = inode->i_sb;
+ unsigned int overflow;
+ struct ext4_sb_info *sbi;
+
+ sbi = EXT4_SB(sb);
+
+ if (sbi->s_mount_state & EXT4_FC_REPLAY) {
+ ext4_free_blocks_simple(inode, block, count);
+ return;
+ }
+
+ might_sleep();
+ if (bh) {
+ if (block)
+ BUG_ON(block != bh->b_blocknr);
+ else
+ block = bh->b_blocknr;
+ }
+
+ if (!(flags & EXT4_FREE_BLOCKS_VALIDATED) &&
+ !ext4_inode_block_valid(inode, block, count)) {
+ ext4_error(sb, "Freeing blocks not in datazone - "
+ "block = %llu, count = %lu", block, count);
+ return;
+ }
+
+ ext4_debug("freeing block %llu\n", block);
+ trace_ext4_free_blocks(inode, block, count, flags);
+
+ if (bh && (flags & EXT4_FREE_BLOCKS_FORGET)) {
+ BUG_ON(count > 1);
+
+ ext4_forget(handle, flags & EXT4_FREE_BLOCKS_METADATA,
+ inode, bh, block);
+ }
+
+ /*
+ * If the extent to be freed does not begin on a cluster
+ * boundary, we need to deal with partial clusters at the
+ * beginning and end of the extent. Normally we will free
+ * blocks at the beginning or the end unless we are explicitly
+ * requested to avoid doing so.
+ */
+ overflow = EXT4_PBLK_COFF(sbi, block);
+ if (overflow) {
+ if (flags & EXT4_FREE_BLOCKS_NOFREE_FIRST_CLUSTER) {
+ overflow = sbi->s_cluster_ratio - overflow;
+ block += overflow;
+ if (count > overflow)
+ count -= overflow;
+ else
+ return;
+ } else {
+ block -= overflow;
+ count += overflow;
+ }
+ }
+ overflow = EXT4_LBLK_COFF(sbi, count);
+ if (overflow) {
+ if (flags & EXT4_FREE_BLOCKS_NOFREE_LAST_CLUSTER) {
+ if (count > overflow)
+ count -= overflow;
+ else
+ return;
+ } else
+ count += sbi->s_cluster_ratio - overflow;
+ }
+
+ if (!bh && (flags & EXT4_FREE_BLOCKS_FORGET)) {
+ int i;
+ int is_metadata = flags & EXT4_FREE_BLOCKS_METADATA;
+
+ for (i = 0; i < count; i++) {
+ cond_resched();
+ if (is_metadata)
+ bh = sb_find_get_block(inode->i_sb, block + i);
+ ext4_forget(handle, is_metadata, inode, bh, block + i);
+ }
+ }
+
+ ext4_mb_clear_bb(handle, inode, block, count, flags);
+ return;
+}
+
/**
* ext4_group_add_blocks() -- Add given blocks to an existing group
* @handle: handle to this transaction
--
2.31.1


2022-02-02 13:34:26

by Jan Kara

[permalink] [raw]
Subject: Re: [RFC 5/6] ext4: Refactor ext4_free_blocks() to pull out ext4_mb_clear_bb()

On Mon 31-01-22 20:46:54, Ritesh Harjani wrote:
> ext4_free_blocks() function became too long and confusing, this patch
> just pulls out the ext4_mb_clear_bb() function logic from it
> which clears the block bitmap and frees it.
>
> No functionality change in this patch
>
> Signed-off-by: Ritesh Harjani <[email protected]>

Yeah, the function was rather long. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

Honza

> ---
> fs/ext4/mballoc.c | 180 ++++++++++++++++++++++++++--------------------
> 1 file changed, 102 insertions(+), 78 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 2f931575e6c2..5f20e355d08c 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -5870,7 +5870,8 @@ static void ext4_free_blocks_simple(struct inode *inode, ext4_fsblk_t block,
> }
>
> /**
> - * ext4_free_blocks() -- Free given blocks and update quota
> + * ext4_mb_clear_bb() -- helper function for freeing blocks.
> + * Used by ext4_free_blocks()
> * @handle: handle for this transaction
> * @inode: inode
> * @bh: optional buffer of the block to be freed
> @@ -5878,9 +5879,9 @@ static void ext4_free_blocks_simple(struct inode *inode, ext4_fsblk_t block,
> * @count: number of blocks to be freed
> * @flags: flags used by ext4_free_blocks
> */
> -void ext4_free_blocks(handle_t *handle, struct inode *inode,
> - struct buffer_head *bh, ext4_fsblk_t block,
> - unsigned long count, int flags)
> +static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
> + ext4_fsblk_t block, unsigned long count,
> + int flags)
> {
> struct buffer_head *bitmap_bh = NULL;
> struct super_block *sb = inode->i_sb;
> @@ -5897,80 +5898,6 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
>
> sbi = EXT4_SB(sb);
>
> - if (sbi->s_mount_state & EXT4_FC_REPLAY) {
> - ext4_free_blocks_simple(inode, block, count);
> - return;
> - }
> -
> - might_sleep();
> - if (bh) {
> - if (block)
> - BUG_ON(block != bh->b_blocknr);
> - else
> - block = bh->b_blocknr;
> - }
> -
> - if (!(flags & EXT4_FREE_BLOCKS_VALIDATED) &&
> - !ext4_inode_block_valid(inode, block, count)) {
> - ext4_error(sb, "Freeing blocks not in datazone - "
> - "block = %llu, count = %lu", block, count);
> - goto error_return;
> - }
> -
> - ext4_debug("freeing block %llu\n", block);
> - trace_ext4_free_blocks(inode, block, count, flags);
> -
> - if (bh && (flags & EXT4_FREE_BLOCKS_FORGET)) {
> - BUG_ON(count > 1);
> -
> - ext4_forget(handle, flags & EXT4_FREE_BLOCKS_METADATA,
> - inode, bh, block);
> - }
> -
> - /*
> - * If the extent to be freed does not begin on a cluster
> - * boundary, we need to deal with partial clusters at the
> - * beginning and end of the extent. Normally we will free
> - * blocks at the beginning or the end unless we are explicitly
> - * requested to avoid doing so.
> - */
> - overflow = EXT4_PBLK_COFF(sbi, block);
> - if (overflow) {
> - if (flags & EXT4_FREE_BLOCKS_NOFREE_FIRST_CLUSTER) {
> - overflow = sbi->s_cluster_ratio - overflow;
> - block += overflow;
> - if (count > overflow)
> - count -= overflow;
> - else
> - return;
> - } else {
> - block -= overflow;
> - count += overflow;
> - }
> - }
> - overflow = EXT4_LBLK_COFF(sbi, count);
> - if (overflow) {
> - if (flags & EXT4_FREE_BLOCKS_NOFREE_LAST_CLUSTER) {
> - if (count > overflow)
> - count -= overflow;
> - else
> - return;
> - } else
> - count += sbi->s_cluster_ratio - overflow;
> - }
> -
> - if (!bh && (flags & EXT4_FREE_BLOCKS_FORGET)) {
> - int i;
> - int is_metadata = flags & EXT4_FREE_BLOCKS_METADATA;
> -
> - for (i = 0; i < count; i++) {
> - cond_resched();
> - if (is_metadata)
> - bh = sb_find_get_block(inode->i_sb, block + i);
> - ext4_forget(handle, is_metadata, inode, bh, block + i);
> - }
> - }
> -
> do_more:
> overflow = 0;
> ext4_get_group_no_and_offset(sb, block, &block_group, &bit);
> @@ -6132,6 +6059,103 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
> return;
> }
>
> +/**
> + * ext4_free_blocks() -- Free given blocks and update quota
> + * @handle: handle for this transaction
> + * @inode: inode
> + * @bh: optional buffer of the block to be freed
> + * @block: starting physical block to be freed
> + * @count: number of blocks to be freed
> + * @flags: flags used by ext4_free_blocks
> + */
> +void ext4_free_blocks(handle_t *handle, struct inode *inode,
> + struct buffer_head *bh, ext4_fsblk_t block,
> + unsigned long count, int flags)
> +{
> + struct super_block *sb = inode->i_sb;
> + unsigned int overflow;
> + struct ext4_sb_info *sbi;
> +
> + sbi = EXT4_SB(sb);
> +
> + if (sbi->s_mount_state & EXT4_FC_REPLAY) {
> + ext4_free_blocks_simple(inode, block, count);
> + return;
> + }
> +
> + might_sleep();
> + if (bh) {
> + if (block)
> + BUG_ON(block != bh->b_blocknr);
> + else
> + block = bh->b_blocknr;
> + }
> +
> + if (!(flags & EXT4_FREE_BLOCKS_VALIDATED) &&
> + !ext4_inode_block_valid(inode, block, count)) {
> + ext4_error(sb, "Freeing blocks not in datazone - "
> + "block = %llu, count = %lu", block, count);
> + return;
> + }
> +
> + ext4_debug("freeing block %llu\n", block);
> + trace_ext4_free_blocks(inode, block, count, flags);
> +
> + if (bh && (flags & EXT4_FREE_BLOCKS_FORGET)) {
> + BUG_ON(count > 1);
> +
> + ext4_forget(handle, flags & EXT4_FREE_BLOCKS_METADATA,
> + inode, bh, block);
> + }
> +
> + /*
> + * If the extent to be freed does not begin on a cluster
> + * boundary, we need to deal with partial clusters at the
> + * beginning and end of the extent. Normally we will free
> + * blocks at the beginning or the end unless we are explicitly
> + * requested to avoid doing so.
> + */
> + overflow = EXT4_PBLK_COFF(sbi, block);
> + if (overflow) {
> + if (flags & EXT4_FREE_BLOCKS_NOFREE_FIRST_CLUSTER) {
> + overflow = sbi->s_cluster_ratio - overflow;
> + block += overflow;
> + if (count > overflow)
> + count -= overflow;
> + else
> + return;
> + } else {
> + block -= overflow;
> + count += overflow;
> + }
> + }
> + overflow = EXT4_LBLK_COFF(sbi, count);
> + if (overflow) {
> + if (flags & EXT4_FREE_BLOCKS_NOFREE_LAST_CLUSTER) {
> + if (count > overflow)
> + count -= overflow;
> + else
> + return;
> + } else
> + count += sbi->s_cluster_ratio - overflow;
> + }
> +
> + if (!bh && (flags & EXT4_FREE_BLOCKS_FORGET)) {
> + int i;
> + int is_metadata = flags & EXT4_FREE_BLOCKS_METADATA;
> +
> + for (i = 0; i < count; i++) {
> + cond_resched();
> + if (is_metadata)
> + bh = sb_find_get_block(inode->i_sb, block + i);
> + ext4_forget(handle, is_metadata, inode, bh, block + i);
> + }
> + }
> +
> + ext4_mb_clear_bb(handle, inode, block, count, flags);
> + return;
> +}
> +
> /**
> * ext4_group_add_blocks() -- Add given blocks to an existing group
> * @handle: handle to this transaction
> --
> 2.31.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR