On Wed 10-07-24 12:06:47, libaokun@xxxxxxxxxxxxxxx wrote:
> From: Baokun Li <libaokun1@xxxxxxxxxx>
>
> The use of path and ppath is now very confusing, so to make the code more
> readable, pass path between functions uniformly, and get rid of ppath.
>
> To get rid of the ppath in ext4_force_split_extent_at(), the following is
> done here:
>
> * The ext4_find_extent() can update the extent path so it doesn't have to
> allocate and free path repeatedly, thus reducing the consumption of
> memory allocation and freeing in ext4_swap_extents().
>
> No functional changes.
>
> Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
> ---
> fs/ext4/extents.c | 117 ++++++++++++++++++++++++----------------------
> 1 file changed, 60 insertions(+), 57 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index c86b1bb7720f..0bd068ed324f 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -328,27 +328,20 @@ static inline int ext4_ext_space_root_idx(struct inode *inode, int check)
> return size;
> }
>
> -static inline int
> +static inline struct ext4_ext_path *
> ext4_force_split_extent_at(handle_t *handle, struct inode *inode,
> - struct ext4_ext_path **ppath, ext4_lblk_t lblk,
> + struct ext4_ext_path *path, ext4_lblk_t lblk,
> int nofail)
> {
> - struct ext4_ext_path *path = *ppath;
> int unwritten = ext4_ext_is_unwritten(path[path->p_depth].p_ext);
> int flags = EXT4_EX_NOCACHE | EXT4_GET_BLOCKS_PRE_IO;
>
> if (nofail)
> flags |= EXT4_GET_BLOCKS_METADATA_NOFAIL | EXT4_EX_NOFAIL;
>
> - path = ext4_split_extent_at(handle, inode, path, lblk, unwritten ?
> + return ext4_split_extent_at(handle, inode, path, lblk, unwritten ?
> EXT4_EXT_MARK_UNWRIT1|EXT4_EXT_MARK_UNWRIT2 : 0,
> flags);
> - if (IS_ERR(path)) {
> - *ppath = NULL;
> - return PTR_ERR(path);
> - }
> - *ppath = path;
> - return 0;
> }
>
> static int
> @@ -2906,11 +2899,12 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
> * fail removing space due to ENOSPC so try to use
> * reserved block if that happens.
> */
> - err = ext4_force_split_extent_at(handle, inode, &path,
> - end + 1, 1);
> - if (err < 0)
> + path = ext4_force_split_extent_at(handle, inode, path,
> + end + 1, 1);
> + if (IS_ERR(path)) {
> + err = PTR_ERR(path);
> goto out;
> -
> + }
> } else if (sbi->s_cluster_ratio > 1 && end >= ex_end &&
> partial.state == initial) {
> /*
> @@ -5707,25 +5701,21 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
> int e1_len, e2_len, len;
> int split = 0;
>
> - path1 = ext4_find_extent(inode1, lblk1, NULL, EXT4_EX_NOCACHE);
> + path1 = ext4_find_extent(inode1, lblk1, path1, EXT4_EX_NOCACHE);
> if (IS_ERR(path1)) {
> *erp = PTR_ERR(path1);
> - path1 = NULL;
> - finish:
> - count = 0;
> - goto repeat;
> + goto errout;
> }
> - path2 = ext4_find_extent(inode2, lblk2, NULL, EXT4_EX_NOCACHE);
> + path2 = ext4_find_extent(inode2, lblk2, path2, EXT4_EX_NOCACHE);
> if (IS_ERR(path2)) {
> *erp = PTR_ERR(path2);
> - path2 = NULL;
> - goto finish;
> + goto errout;
> }
> ex1 = path1[path1->p_depth].p_ext;
> ex2 = path2[path2->p_depth].p_ext;
> /* Do we have something to swap ? */
> if (unlikely(!ex2 || !ex1))
> - goto finish;
> + goto errout;
>
> e1_blk = le32_to_cpu(ex1->ee_block);
> e2_blk = le32_to_cpu(ex2->ee_block);
> @@ -5747,7 +5737,7 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
> next2 = e2_blk;
> /* Do we have something to swap */
> if (next1 == EXT_MAX_BLOCKS || next2 == EXT_MAX_BLOCKS)
> - goto finish;
> + goto errout;
> /* Move to the rightest boundary */
> len = next1 - lblk1;
> if (len < next2 - lblk2)
> @@ -5757,28 +5747,32 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
> lblk1 += len;
> lblk2 += len;
> count -= len;
> - goto repeat;
> + continue;
> }
>
> /* Prepare left boundary */
> if (e1_blk < lblk1) {
> split = 1;
> - *erp = ext4_force_split_extent_at(handle, inode1,
> - &path1, lblk1, 0);
> - if (unlikely(*erp))
> - goto finish;
> + path1 = ext4_force_split_extent_at(handle, inode1,
> + path1, lblk1, 0);
> + if (IS_ERR(path1)) {
> + *erp = PTR_ERR(path1);
> + goto errout;
> + }
> }
> if (e2_blk < lblk2) {
> split = 1;
> - *erp = ext4_force_split_extent_at(handle, inode2,
> - &path2, lblk2, 0);
> - if (unlikely(*erp))
> - goto finish;
> + path2 = ext4_force_split_extent_at(handle, inode2,
> + path2, lblk2, 0);
> + if (IS_ERR(path2)) {
> + *erp = PTR_ERR(path2);
> + goto errout;
> + }
> }
> /* ext4_split_extent_at() may result in leaf extent split,
> * path must to be revalidated. */
> if (split)
> - goto repeat;
> + continue;
>
> /* Prepare right boundary */
> len = count;
> @@ -5789,30 +5783,34 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
>
> if (len != e1_len) {
> split = 1;
> - *erp = ext4_force_split_extent_at(handle, inode1,
> - &path1, lblk1 + len, 0);
> - if (unlikely(*erp))
> - goto finish;
> + path1 = ext4_force_split_extent_at(handle, inode1,
> + path1, lblk1 + len, 0);
> + if (IS_ERR(path1)) {
> + *erp = PTR_ERR(path1);
> + goto errout;
> + }
> }
> if (len != e2_len) {
> split = 1;
> - *erp = ext4_force_split_extent_at(handle, inode2,
> - &path2, lblk2 + len, 0);
> - if (*erp)
> - goto finish;
> + path2 = ext4_force_split_extent_at(handle, inode2,
> + path2, lblk2 + len, 0);
> + if (IS_ERR(path2)) {
> + *erp = PTR_ERR(path2);
> + goto errout;
> + }
> }
> /* ext4_split_extent_at() may result in leaf extent split,
> * path must to be revalidated. */
> if (split)
> - goto repeat;
> + continue;
>
> BUG_ON(e2_len != e1_len);
> *erp = ext4_ext_get_access(handle, inode1, path1 + path1->p_depth);
> if (unlikely(*erp))
> - goto finish;
> + goto errout;
> *erp = ext4_ext_get_access(handle, inode2, path2 + path2->p_depth);
> if (unlikely(*erp))
> - goto finish;
> + goto errout;
>
> /* Both extents are fully inside boundaries. Swap it now */
> tmp_ex = *ex1;
> @@ -5830,7 +5828,7 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
> *erp = ext4_ext_dirty(handle, inode2, path2 +
> path2->p_depth);
> if (unlikely(*erp))
> - goto finish;
> + goto errout;
> *erp = ext4_ext_dirty(handle, inode1, path1 +
> path1->p_depth);
> /*
> @@ -5840,17 +5838,17 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
> * aborted anyway.
> */
> if (unlikely(*erp))
> - goto finish;
> + goto errout;
> +
> lblk1 += len;
> lblk2 += len;
> replaced_count += len;
> count -= len;
> -
> - repeat:
> - ext4_free_ext_path(path1);
> - ext4_free_ext_path(path2);
> - path1 = path2 = NULL;
> }
> +
> +errout:
> + ext4_free_ext_path(path1);
> + ext4_free_ext_path(path2);
> return replaced_count;
> }
>
> @@ -5965,24 +5963,29 @@ int ext4_ext_replay_update_ex(struct inode *inode, ext4_lblk_t start,
> ext4_ext_get_actual_len(ex) != len) {
> /* We need to split this extent to match our extent first */
> down_write(&EXT4_I(inode)->i_data_sem);
> - ret = ext4_force_split_extent_at(NULL, inode, &path, start, 1);
> + path = ext4_force_split_extent_at(NULL, inode, path, start, 1);
> up_write(&EXT4_I(inode)->i_data_sem);
> - if (ret)
> + if (IS_ERR(path)) {
> + ret = PTR_ERR(path);
> goto out;
> + }
>
> path = ext4_find_extent(inode, start, path, 0);
> if (IS_ERR(path))
> return PTR_ERR(path);
> +
> ex = path[path->p_depth].p_ext;
> WARN_ON(le32_to_cpu(ex->ee_block) != start);
>
> if (ext4_ext_get_actual_len(ex) != len) {
> down_write(&EXT4_I(inode)->i_data_sem);
> - ret = ext4_force_split_extent_at(NULL, inode, &path,
> - start + len, 1);
> + path = ext4_force_split_extent_at(NULL, inode, path,
> + start + len, 1);
> up_write(&EXT4_I(inode)->i_data_sem);
> - if (ret)
> + if (IS_ERR(path)) {
> + ret = PTR_ERR(path);
> goto out;
> + }
>
> path = ext4_find_extent(inode, start, path, 0);
> if (IS_ERR(path))
> --
> 2.39.2
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
