Message ID | 20240710040654.1714672-13-libaokun@huaweicloud.com |
---|---|
State | Superseded |
Headers | show |
Series | ext4: some bugfixes and cleanups for ext4 extents path | expand |
On Wed 10-07-24 12:06:46, libaokun@huaweicloud.com wrote: > From: Baokun Li <libaokun1@huawei.com> > > 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@huawei.com> One nit below: > @@ -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; > } AFAICT this actually fixes a bug where we could have returned 0 although ext4_find_extent() spotted an error? This would deserve a separate patch so that it could be easily pulled into stable. Otherwise looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza
On 2024/7/25 19:07, Jan Kara wrote: > On Wed 10-07-24 12:06:46, libaokun@huaweicloud.com wrote: >> From: Baokun Li <libaokun1@huawei.com> >> >> 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@huawei.com> > One nit below: > >> @@ -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; >> } > AFAICT this actually fixes a bug where we could have returned 0 although > ext4_find_extent() spotted an error? Yeah, exactly. > This would deserve a separate patch so > that it could be easily pulled into stable. > > Otherwise looks good. Feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > Honza Ok, I'll put this in a separate patch in the next version. Thank you very much for your review!
On Wed, Jul 10, 2024 at 12:06:46PM +0800, libaokun@huaweicloud.com wrote: > From: Baokun Li <libaokun1@huawei.com> > > 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@huawei.com> Hi Baokun, Change looks good, feel free to add: Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> 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 >
On Sat, Jul 27, 2024 at 02:42:50PM +0800, Baokun Li wrote: > Honza > Ok, I'll put this in a separate patch in the next version. > > Thank you very much for your review! > Hi Baokun, Did you send out a newer version of this patch series? I can't seem to find it in patchwork. Thanks, - Ted
On 2024/8/21 11:19, Theodore Ts'o wrote: > On Sat, Jul 27, 2024 at 02:42:50PM +0800, Baokun Li wrote: > > Honza >> Ok, I'll put this in a separate patch in the next version. >> >> Thank you very much for your review! >> > Hi Baokun, > > Did you send out a newer version of this patch series? I can't seem > to find it in patchwork. > > Thanks, > > - Ted Hi Ted. I'm very sorry for the slow update, it's been very busy for a while now. Last week I started preparing the new version and performing some tests. Now that the tests are almost complete, I'll send the new version out tomorrow at the latest.
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); /*