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@xxxxxxxxxxxxxxx/
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
