On Wed, Jul 10, 2024 at 12:06:47PM +0800, 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.
Looks good Baokun, feel free to add:
Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
One small comment below..
>
> Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
> ---
> 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
.. snip ..
> @@ -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;
> }
>
In ext4_swap_extents, maybe we should keep the refactoring to a separate
patch than the changes needed to get rid of ppath in
ext4_force_split_extent_at(), the commits would look a bit cleaner and
easier to read that way. I don't feel too strongly about it tho and
I'll let you take a call.
Regards,
ojaswin
