| Message ID | tencent_BCDA824359279D18963B838B15B01BC1CD06@qq.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ext4: propagate errors from fast commit i_blocks replay | expand |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Incomplete error propagation in `ext4_ext_replay_set_iblocks` when encountering an empty extent leaf. -- commit d0296b8bc160aac911c99a12a53e4f06189bb5cd Author: lty <781735889@qq.com> ext4: propagate errors from fast commit i_blocks replay This commit modifies ext4_ext_replay_set_iblocks() to stop calculating blocks and propagate errors when mapping fails. It ensures failures from extent lookups abort the fast commit inode replay. > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c [ ... ] > @@ -6291,15 +6291,19 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) > numblks += path->p_depth; > while (cur < end) { > path = ext4_find_extent(inode, cur, path, 0); > - if (IS_ERR(path)) > - break; > + if (IS_ERR(path)) { > + ret = PTR_ERR(path); > + goto cleanup; > + } > ex = path[path->p_depth].p_ext; > if (!ex) > goto cleanup; [Severity: High] Since ret is 0 on a successful extent lookup, does this branch silently return success when it encounters an empty extent leaf? If an empty leaf (!ex) represents a corrupted extent tree, should this set an error code like -EFSCORRUPTED before jumping to cleanup, so the fast commit replay can properly abort?
On Mon 14-09-26 20:05:56, lty wrote: > ext4_ext_replay_set_iblocks() breaks out of its mapping loop when > ext4_map_blocks() fails, but then continues with a partial block count > and unconditionally returns success. The fast commit replay caller also > ignores the helper return value. > > Stop the count immediately on mapping errors, leave i_blocks unchanged, > and propagate the failure to abort inode replay. > > Fixes: 8016e29f4362 ("ext4: fast commit recovery path") > > Signed-off-by: lty <781735889@qq.com> > > Changes in v2: > - Propagate errors from all ext4_find_extent() and skip_hole() calls in > ext4_ext_replay_set_iblocks(), as requested by Jan Kara. > - Keep i_blocks unchanged when any replay lookup fails. The 'Changes' paragraph belongs after the --- separator below so that it doesn't get into the pernament changelog. Otherwise the fix looks good to me. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/ext4/extents.c | 26 ++++++++++++++++---------- > fs/ext4/fast_commit.c | 7 +++++-- > 2 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index 76038b6c3655..f8c805d80c33 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -6275,7 +6275,7 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) > map.m_len = end - cur; > ret = ext4_map_blocks(NULL, inode, &map, 0); > if (ret < 0) > - break; > + goto cleanup; > if (ret > 0) > numblks += ret; > cur = cur + map.m_len; > @@ -6291,15 +6291,19 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) > cur = 0; > ret = skip_hole(inode, &cur); > if (ret < 0) > - goto out; > + goto cleanup; > path = ext4_find_extent(inode, cur, path, 0); > - if (IS_ERR(path)) > - goto out; > + if (IS_ERR(path)) { > + ret = PTR_ERR(path); > + goto cleanup; > + } > numblks += path->p_depth; > while (cur < end) { > path = ext4_find_extent(inode, cur, path, 0); > - if (IS_ERR(path)) > - break; > + if (IS_ERR(path)) { > + ret = PTR_ERR(path); > + goto cleanup; > + } > ex = path[path->p_depth].p_ext; > if (!ex) > goto cleanup; > @@ -6308,11 +6312,13 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) > ext4_ext_get_actual_len(ex)); > ret = skip_hole(inode, &cur); > if (ret < 0) > - break; > + goto cleanup; > > path2 = ext4_find_extent(inode, cur, path2, 0); > - if (IS_ERR(path2)) > - break; > + if (IS_ERR(path2)) { > + ret = PTR_ERR(path2); > + goto cleanup; > + } > > for (i = 0; i <= max(path->p_depth, path2->p_depth); i++) { > cmp1 = cmp2 = 0; > @@ -6333,7 +6339,7 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) > cleanup: > ext4_free_ext_path(path); > ext4_free_ext_path(path2); > - return 0; > + return ret; > } > > int ext4_ext_clear_bb(struct inode *inode) > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c > index 0cac890cf370..8110b01e36c1 100644 > --- a/fs/ext4/fast_commit.c > +++ b/fs/ext4/fast_commit.c > @@ -2002,8 +2002,11 @@ static int ext4_fc_replay_inode(struct super_block *sb, > * crashing. This should be fixed but until then, we calculate > * the number of blocks the inode. > */ > - if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA)) > - ext4_ext_replay_set_iblocks(inode); > + if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA)) { > + ret = ext4_ext_replay_set_iblocks(inode); > + if (ret) > + goto out_brelse; > + } > > inode->i_generation = le32_to_cpu(ext4_raw_inode(&iloc)->i_generation); > ext4_reset_inode_seed(inode); > -- > 2.34.1 >
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 76038b6c3655..f8c805d80c33 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -6275,7 +6275,7 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) map.m_len = end - cur; ret = ext4_map_blocks(NULL, inode, &map, 0); if (ret < 0) - break; + goto cleanup; if (ret > 0) numblks += ret; cur = cur + map.m_len; @@ -6291,15 +6291,19 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) cur = 0; ret = skip_hole(inode, &cur); if (ret < 0) - goto out; + goto cleanup; path = ext4_find_extent(inode, cur, path, 0); - if (IS_ERR(path)) - goto out; + if (IS_ERR(path)) { + ret = PTR_ERR(path); + goto cleanup; + } numblks += path->p_depth; while (cur < end) { path = ext4_find_extent(inode, cur, path, 0); - if (IS_ERR(path)) - break; + if (IS_ERR(path)) { + ret = PTR_ERR(path); + goto cleanup; + } ex = path[path->p_depth].p_ext; if (!ex) goto cleanup; @@ -6308,11 +6312,13 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) ext4_ext_get_actual_len(ex)); ret = skip_hole(inode, &cur); if (ret < 0) - break; + goto cleanup; path2 = ext4_find_extent(inode, cur, path2, 0); - if (IS_ERR(path2)) - break; + if (IS_ERR(path2)) { + ret = PTR_ERR(path2); + goto cleanup; + } for (i = 0; i <= max(path->p_depth, path2->p_depth); i++) { cmp1 = cmp2 = 0; @@ -6333,7 +6339,7 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) cleanup: ext4_free_ext_path(path); ext4_free_ext_path(path2); - return 0; + return ret; } int ext4_ext_clear_bb(struct inode *inode) diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c index 0cac890cf370..8110b01e36c1 100644 --- a/fs/ext4/fast_commit.c +++ b/fs/ext4/fast_commit.c @@ -2002,8 +2002,11 @@ static int ext4_fc_replay_inode(struct super_block *sb, * crashing. This should be fixed but until then, we calculate * the number of blocks the inode. */ - if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA)) - ext4_ext_replay_set_iblocks(inode); + if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA)) { + ret = ext4_ext_replay_set_iblocks(inode); + if (ret) + goto out_brelse; + } inode->i_generation = le32_to_cpu(ext4_raw_inode(&iloc)->i_generation); ext4_reset_inode_seed(inode);
ext4_ext_replay_set_iblocks() breaks out of its mapping loop when ext4_map_blocks() fails, but then continues with a partial block count and unconditionally returns success. The fast commit replay caller also ignores the helper return value. Stop the count immediately on mapping errors, leave i_blocks unchanged, and propagate the failure to abort inode replay. Fixes: 8016e29f4362 ("ext4: fast commit recovery path") Signed-off-by: lty <781735889@qq.com> Changes in v2: - Propagate errors from all ext4_find_extent() and skip_hole() calls in ext4_ext_replay_set_iblocks(), as requested by Jan Kara. - Keep i_blocks unchanged when any replay lookup fails. --- fs/ext4/extents.c | 26 ++++++++++++++++---------- fs/ext4/fast_commit.c | 7 +++++-- 2 files changed, 21 insertions(+), 12 deletions(-)