Re: [PATCH 20/20] ext4: avoid unnecessary extent path frees and allocations
From: Jan Kara
Date: Thu Jul 25 2024 - 08:31:31 EST
On Wed 10-07-24 12:06:54, libaokun@xxxxxxxxxxxxxxx wrote:
> From: Baokun Li <libaokun1@xxxxxxxxxx>
>
> The ext4_find_extent() can update the extent path so that it does not have
> to allocate and free the path repeatedly, thus reducing the consumption of
> memory allocation and freeing in the following functions:
>
> ext4_ext_clear_bb
> ext4_ext_replay_set_iblocks
> ext4_fc_replay_add_range
> ext4_fc_set_bitmaps_and_counters
>
> No functional changes. Note that ext4_find_extent() does not support error
> pointers, so in this case set path to NULL first.
>
> Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
Looks good! Feel free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
> ---
> fs/ext4/extents.c | 51 +++++++++++++++++++------------------------
> fs/ext4/fast_commit.c | 11 ++++++----
> 2 files changed, 29 insertions(+), 33 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 737432bb316e..5ff92cd50dc8 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -6068,12 +6068,9 @@ int ext4_ext_replay_set_iblocks(struct inode *inode)
> if (IS_ERR(path))
> return PTR_ERR(path);
> ex = path[path->p_depth].p_ext;
> - if (!ex) {
> - ext4_free_ext_path(path);
> + if (!ex)
> goto out;
> - }
> end = le32_to_cpu(ex->ee_block) + ext4_ext_get_actual_len(ex);
> - ext4_free_ext_path(path);
>
> /* Count the number of data blocks */
> cur = 0;
> @@ -6099,32 +6096,28 @@ int ext4_ext_replay_set_iblocks(struct inode *inode)
> ret = skip_hole(inode, &cur);
> if (ret < 0)
> goto out;
> - path = ext4_find_extent(inode, cur, NULL, 0);
> + path = ext4_find_extent(inode, cur, path, 0);
> if (IS_ERR(path))
> goto out;
> numblks += path->p_depth;
> - ext4_free_ext_path(path);
> while (cur < end) {
> - path = ext4_find_extent(inode, cur, NULL, 0);
> + path = ext4_find_extent(inode, cur, path, 0);
> if (IS_ERR(path))
> break;
> ex = path[path->p_depth].p_ext;
> - if (!ex) {
> - ext4_free_ext_path(path);
> - return 0;
> - }
> + if (!ex)
> + goto cleanup;
> +
> cur = max(cur + 1, le32_to_cpu(ex->ee_block) +
> ext4_ext_get_actual_len(ex));
> ret = skip_hole(inode, &cur);
> - if (ret < 0) {
> - ext4_free_ext_path(path);
> + if (ret < 0)
> break;
> - }
> - path2 = ext4_find_extent(inode, cur, NULL, 0);
> - if (IS_ERR(path2)) {
> - ext4_free_ext_path(path);
> +
> + path2 = ext4_find_extent(inode, cur, path2, 0);
> + if (IS_ERR(path2))
> break;
> - }
> +
> for (i = 0; i <= max(path->p_depth, path2->p_depth); i++) {
> cmp1 = cmp2 = 0;
> if (i <= path->p_depth)
> @@ -6136,13 +6129,14 @@ int ext4_ext_replay_set_iblocks(struct inode *inode)
> if (cmp1 != cmp2 && cmp2 != 0)
> numblks++;
> }
> - ext4_free_ext_path(path);
> - ext4_free_ext_path(path2);
> }
>
> out:
> inode->i_blocks = numblks << (inode->i_sb->s_blocksize_bits - 9);
> ext4_mark_inode_dirty(NULL, inode);
> +cleanup:
> + ext4_free_ext_path(path);
> + ext4_free_ext_path(path2);
> return 0;
> }
>
> @@ -6163,12 +6157,9 @@ int ext4_ext_clear_bb(struct inode *inode)
> if (IS_ERR(path))
> return PTR_ERR(path);
> ex = path[path->p_depth].p_ext;
> - if (!ex) {
> - ext4_free_ext_path(path);
> - return 0;
> - }
> + if (!ex)
> + goto out;
> end = le32_to_cpu(ex->ee_block) + ext4_ext_get_actual_len(ex);
> - ext4_free_ext_path(path);
>
> cur = 0;
> while (cur < end) {
> @@ -6178,16 +6169,16 @@ int ext4_ext_clear_bb(struct inode *inode)
> if (ret < 0)
> break;
> if (ret > 0) {
> - path = ext4_find_extent(inode, map.m_lblk, NULL, 0);
> - if (!IS_ERR_OR_NULL(path)) {
> + path = ext4_find_extent(inode, map.m_lblk, path, 0);
> + if (!IS_ERR(path)) {
> for (j = 0; j < path->p_depth; j++) {
> -
> ext4_mb_mark_bb(inode->i_sb,
> path[j].p_block, 1, false);
> ext4_fc_record_regions(inode->i_sb, inode->i_ino,
> 0, path[j].p_block, 1, 1);
> }
> - ext4_free_ext_path(path);
> + } else {
> + path = NULL;
> }
> ext4_mb_mark_bb(inode->i_sb, map.m_pblk, map.m_len, false);
> ext4_fc_record_regions(inode->i_sb, inode->i_ino,
> @@ -6196,5 +6187,7 @@ int ext4_ext_clear_bb(struct inode *inode)
> cur = cur + map.m_len;
> }
>
> +out:
> + ext4_free_ext_path(path);
> return 0;
> }
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 1dee40477727..1426d595fab7 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -1766,7 +1766,7 @@ static int ext4_fc_replay_add_range(struct super_block *sb,
>
> if (ret == 0) {
> /* Range is not mapped */
> - path = ext4_find_extent(inode, cur, NULL, 0);
> + path = ext4_find_extent(inode, cur, path, 0);
> if (IS_ERR(path))
> goto out;
> memset(&newex, 0, sizeof(newex));
> @@ -1782,7 +1782,6 @@ static int ext4_fc_replay_add_range(struct super_block *sb,
> up_write((&EXT4_I(inode)->i_data_sem));
> if (IS_ERR(path))
> goto out;
> - ext4_free_ext_path(path);
> goto next;
> }
>
> @@ -1830,6 +1829,7 @@ static int ext4_fc_replay_add_range(struct super_block *sb,
> ext4_ext_replay_shrink_inode(inode, i_size_read(inode) >>
> sb->s_blocksize_bits);
> out:
> + ext4_free_ext_path(path);
> iput(inode);
> return 0;
> }
> @@ -1930,12 +1930,13 @@ static void ext4_fc_set_bitmaps_and_counters(struct super_block *sb)
> break;
>
> if (ret > 0) {
> - path = ext4_find_extent(inode, map.m_lblk, NULL, 0);
> + path = ext4_find_extent(inode, map.m_lblk, path, 0);
> if (!IS_ERR(path)) {
> for (j = 0; j < path->p_depth; j++)
> ext4_mb_mark_bb(inode->i_sb,
> path[j].p_block, 1, true);
> - ext4_free_ext_path(path);
> + } else {
> + path = NULL;
> }
> cur += ret;
> ext4_mb_mark_bb(inode->i_sb, map.m_pblk,
> @@ -1946,6 +1947,8 @@ static void ext4_fc_set_bitmaps_and_counters(struct super_block *sb)
> }
> iput(inode);
> }
> +
> + ext4_free_ext_path(path);
> }
>
> /*
> --
> 2.39.2
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR