On Wed, Jul 10, 2024 at 12:06:46PM +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_split_extent_at(), the following is done
> here:
>
> * Free the extents path when an error is encountered.
> * Its caller needs to update ppath if it uses ppath.
> * Teach ext4_ext_show_leaf() to skip error pointer.
> * Propagate ext4_find_extent() error return value in ext4_insert_range().
>
> No functional changes.
>
> Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
Hi Baokun,
Change looks good, feel free to add:
Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
Regards,
ojaswin
> ---
> fs/ext4/extents.c | 86 ++++++++++++++++++++++++++---------------------
> 1 file changed, 48 insertions(+), 38 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index fc75390d591a..c86b1bb7720f 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -84,12 +84,11 @@ static void ext4_extent_block_csum_set(struct inode *inode,
> et->et_checksum = ext4_extent_block_csum(inode, eh);
> }
>
> -static int ext4_split_extent_at(handle_t *handle,
> - struct inode *inode,
> - struct ext4_ext_path **ppath,
> - ext4_lblk_t split,
> - int split_flag,
> - int flags);
> +static struct ext4_ext_path *ext4_split_extent_at(handle_t *handle,
> + struct inode *inode,
> + struct ext4_ext_path *path,
> + ext4_lblk_t split,
> + int split_flag, int flags);
>
> static int ext4_ext_trunc_restart_fn(struct inode *inode, int *dropped)
> {
> @@ -341,9 +340,15 @@ ext4_force_split_extent_at(handle_t *handle, struct inode *inode,
> if (nofail)
> flags |= EXT4_GET_BLOCKS_METADATA_NOFAIL | EXT4_EX_NOFAIL;
>
> - return ext4_split_extent_at(handle, inode, ppath, lblk, unwritten ?
> + path = 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
> @@ -694,7 +699,7 @@ static void ext4_ext_show_leaf(struct inode *inode, struct ext4_ext_path *path)
> struct ext4_extent *ex;
> int i;
>
> - if (!path)
> + if (IS_ERR_OR_NULL(path))
> return;
>
> eh = path[depth].p_hdr;
> @@ -3174,16 +3179,14 @@ static int ext4_ext_zeroout(struct inode *inode, struct ext4_extent *ex)
> * a> the extent are splitted into two extent.
> * b> split is not needed, and just mark the extent.
> *
> - * return 0 on success.
> + * Return an extent path pointer on success, or an error pointer on failure.
> */
> -static int ext4_split_extent_at(handle_t *handle,
> - struct inode *inode,
> - struct ext4_ext_path **ppath,
> - ext4_lblk_t split,
> - int split_flag,
> - int flags)
> +static struct ext4_ext_path *ext4_split_extent_at(handle_t *handle,
> + struct inode *inode,
> + struct ext4_ext_path *path,
> + ext4_lblk_t split,
> + int split_flag, int flags)
> {
> - struct ext4_ext_path *path = *ppath;
> ext4_fsblk_t newblock;
> ext4_lblk_t ee_block;
> struct ext4_extent *ex, newex, orig_ex, zero_ex;
> @@ -3254,14 +3257,12 @@ static int ext4_split_extent_at(handle_t *handle,
> ext4_ext_mark_unwritten(ex2);
>
> path = ext4_ext_insert_extent(handle, inode, path, &newex, flags);
> - if (!IS_ERR(path)) {
> - *ppath = path;
> + if (!IS_ERR(path))
> goto out;
> - }
> - *ppath = NULL;
> +
> err = PTR_ERR(path);
> if (err != -ENOSPC && err != -EDQUOT && err != -ENOMEM)
> - return err;
> + return path;
>
> /*
> * Get a new path to try to zeroout or fix the extent length.
> @@ -3271,16 +3272,14 @@ static int ext4_split_extent_at(handle_t *handle,
> * in ext4_da_update_reserve_space() due to an incorrect
> * ee_len causing the i_reserved_data_blocks exception.
> */
> - path = ext4_find_extent(inode, ee_block, NULL,
> - flags | EXT4_EX_NOFAIL);
> + path = ext4_find_extent(inode, ee_block, NULL, flags | EXT4_EX_NOFAIL);
> if (IS_ERR(path)) {
> EXT4_ERROR_INODE(inode, "Failed split extent on %u, err %ld",
> split, PTR_ERR(path));
> - return PTR_ERR(path);
> + return path;
> }
> depth = ext_depth(inode);
> ex = path[depth].p_ext;
> - *ppath = path;
>
> if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
> if (split_flag & (EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) {
> @@ -3332,10 +3331,13 @@ static int ext4_split_extent_at(handle_t *handle,
> * and err is a non-zero error code.
> */
> ext4_ext_dirty(handle, inode, path + path->p_depth);
> - return err;
> out:
> + if (err) {
> + ext4_free_ext_path(path);
> + path = ERR_PTR(err);
> + }
> ext4_ext_show_leaf(inode, path);
> - return err;
> + return path;
> }
>
> /*
> @@ -3379,10 +3381,14 @@ static int ext4_split_extent(handle_t *handle,
> EXT4_EXT_MARK_UNWRIT2;
> if (split_flag & EXT4_EXT_DATA_VALID2)
> split_flag1 |= EXT4_EXT_DATA_VALID1;
> - err = ext4_split_extent_at(handle, inode, ppath,
> + path = ext4_split_extent_at(handle, inode, path,
> map->m_lblk + map->m_len, split_flag1, flags1);
> - if (err)
> + if (IS_ERR(path)) {
> + err = PTR_ERR(path);
> + *ppath = NULL;
> goto out;
> + }
> + *ppath = path;
> } else {
> allocated = ee_len - (map->m_lblk - ee_block);
> }
> @@ -3390,7 +3396,7 @@ static int ext4_split_extent(handle_t *handle,
> * Update path is required because previous ext4_split_extent_at() may
> * result in split of original leaf or extent zeroout.
> */
> - path = ext4_find_extent(inode, map->m_lblk, *ppath, flags);
> + path = ext4_find_extent(inode, map->m_lblk, path, flags);
> if (IS_ERR(path)) {
> *ppath = NULL;
> return PTR_ERR(path);
> @@ -3412,13 +3418,17 @@ static int ext4_split_extent(handle_t *handle,
> split_flag1 |= split_flag & (EXT4_EXT_MAY_ZEROOUT |
> EXT4_EXT_MARK_UNWRIT2);
> }
> - err = ext4_split_extent_at(handle, inode, ppath,
> + path = ext4_split_extent_at(handle, inode, path,
> map->m_lblk, split_flag1, flags);
> - if (err)
> + if (IS_ERR(path)) {
> + err = PTR_ERR(path);
> + *ppath = NULL;
> goto out;
> + }
> + *ppath = path;
> }
>
> - ext4_ext_show_leaf(inode, *ppath);
> + ext4_ext_show_leaf(inode, path);
> out:
> return err ? err : allocated;
> }
> @@ -5596,6 +5606,7 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
> path = ext4_find_extent(inode, offset_lblk, NULL, 0);
> if (IS_ERR(path)) {
> up_write(&EXT4_I(inode)->i_data_sem);
> + ret = PTR_ERR(path);
> goto out_stop;
> }
>
> @@ -5614,22 +5625,21 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
> if (ext4_ext_is_unwritten(extent))
> split_flag = EXT4_EXT_MARK_UNWRIT1 |
> EXT4_EXT_MARK_UNWRIT2;
> - ret = ext4_split_extent_at(handle, inode, &path,
> + path = ext4_split_extent_at(handle, inode, path,
> offset_lblk, split_flag,
> EXT4_EX_NOCACHE |
> EXT4_GET_BLOCKS_PRE_IO |
> EXT4_GET_BLOCKS_METADATA_NOFAIL);
> }
>
> - ext4_free_ext_path(path);
> - if (ret < 0) {
> + if (IS_ERR(path)) {
> up_write(&EXT4_I(inode)->i_data_sem);
> + ret = PTR_ERR(path);
> goto out_stop;
> }
> - } else {
> - ext4_free_ext_path(path);
> }
>
> + ext4_free_ext_path(path);
> ext4_es_remove_extent(inode, offset_lblk, EXT_MAX_BLOCKS - offset_lblk);
>
> /*
> --
> 2.39.2
>
