| Message ID | 20240820013250.4121848-1-lizetao1@huawei.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [-next] ext4: Remove redundant null pointer check | expand |
Hi Zetao, On 2024/8/20 9:32, Li Zetao wrote: > Since the ext4_find_extent() does not return a null pointer, the check for > the null pointer here is redundant. Drop null pointer check for clean > code. > > No functional change intended. > > Signed-off-by: Li Zetao <lizetao1@huawei.com> > --- > fs/ext4/extents.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index e067f2dd0335..12f0771d57d2 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -6112,7 +6112,7 @@ int ext4_ext_clear_bb(struct inode *inode) > break; > if (ret > 0) { > path = ext4_find_extent(inode, map.m_lblk, NULL, 0); > - if (!IS_ERR_OR_NULL(path)) { > + if (!IS_ERR(path)) { > for (j = 0; j < path->p_depth; j++) { > > ext4_mb_mark_bb(inode->i_sb, Thanks for the cleanup patch. But the change is already included in the patch: https://lore.kernel.org/all/20240710040654.1714672-21-libaokun@huaweicloud.com/ Thanks, Baokun
On Tue, Sep 03, 2024 at 03:52:01PM +0800, Baokun Li wrote: > Thanks for the cleanup patch. > > But the change is already included in the patch: > > https://lore.kernel.org/all/20240710040654.1714672-21-libaokun@huaweicloud.com/ Yeah, I noticed. I had already applied Zetao's patch when I processed yours, so I just ended up manually handling the patch conflict. (I haven't set out the patch acks yet, because the current state of the ext4/dev branch is apparently causing a test regression which I'm trying to root cause. They will be in tomorrow's fs-next and linux-next branch, though unless I end up figuring out the problematic patch or patch series, and end up dropping them from the ext4 dev branch today. Still, feel free to take a look and let me know if I screwed up anything.) - Ted
Hi Theodore, Unfortunately it is possible that the regression was introduced precisely by the mishandling of conflicts here. On 2024/9/3 20:50, Theodore Ts'o wrote: > On Tue, Sep 03, 2024 at 03:52:01PM +0800, Baokun Li wrote: >> Thanks for the cleanup patch. >> >> But the change is already included in the patch: >> >> https://lore.kernel.org/all/20240710040654.1714672-21-libaokun@huaweicloud.com/ > Yeah, I noticed. I had already applied Zetao's patch when I processed > yours, so I just ended up manually handling the patch conflict. > > (I haven't set out the patch acks yet, because the current state of > the ext4/dev branch is apparently causing a test regression which I'm > trying to root cause. They will be in tomorrow's fs-next and > linux-next branch, though unless I end up figuring out the problematic > patch or patch series, and end up dropping them from the ext4 dev > branch today. Still, feel free to take a look and let me know if I > screwed up anything.) > > - Ted > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=92bbb922166cd7a829bf60d168372ffa1c54e81d The changes after resolving the conflict in ext4_ext_clear_bb() are as follows: --------------------------------------------------------------------------- @@ -6128,12 +6122,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) { @@ -6146,7 +6137,6 @@ int ext4_ext_clear_bb(struct inode *inode) path = ext4_find_extent(inode, map.m_lblk, NULL, 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, @@ -6161,5 +6151,7 @@ int ext4_ext_clear_bb(struct inode *inode) cur = cur + map.m_len; } +out: + ext4_free_ext_path(path); return 0; } --------------------------------------------------------------------------- This can cause path leaks and path double free. ---------------------------------------- path_A = ext4_find_extent while (cur < end) { path_B = ext4_find_extent(inode, map.m_lblk, NULL, 0); // path_A leak ext4_free_ext_path(path_B); } out: ext4_free_ext_path(path_B); // path_B double free ---------------------------------------- I think it's best to drop Zetao's patch b2e662cb86ca ("ext4: remove redundant null pointer check") and then reapply the conflicting patch 92bbb922166c ("ext4: make some fast commit functions reuse extents path") Or apply the following modifications to conflicting patch in the tree: --------------------------------------------------------------------------- diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 789316f22f97..34e25eee6521 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -6132,7 +6132,7 @@ 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); + 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, @@ -6140,7 +6140,8 @@ int ext4_ext_clear_bb(struct inode *inode) 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, --------------------------------------------------------------------------- Cheers, Baokun
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index e067f2dd0335..12f0771d57d2 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -6112,7 +6112,7 @@ int ext4_ext_clear_bb(struct inode *inode) break; if (ret > 0) { path = ext4_find_extent(inode, map.m_lblk, NULL, 0); - if (!IS_ERR_OR_NULL(path)) { + if (!IS_ERR(path)) { for (j = 0; j < path->p_depth; j++) { ext4_mb_mark_bb(inode->i_sb,
Since the ext4_find_extent() does not return a null pointer, the check for the null pointer here is redundant. Drop null pointer check for clean code. No functional change intended. Signed-off-by: Li Zetao <lizetao1@huawei.com> --- fs/ext4/extents.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)