We got follow bug_on when run fsstress with injecting IO fault:
[130747.323114] kernel BUG at fs/ext4/extents_status.c:762!
[130747.323117] Internal error: Oops - BUG: 0 [#1] SMP
......
[130747.334329] Call trace:
[130747.334553] ext4_es_cache_extent+0x150/0x168 [ext4]
[130747.334975] ext4_cache_extents+0x64/0xe8 [ext4]
[130747.335368] ext4_find_extent+0x300/0x330 [ext4]
[130747.335759] ext4_ext_map_blocks+0x74/0x1178 [ext4]
[130747.336179] ext4_map_blocks+0x2f4/0x5f0 [ext4]
[130747.336567] ext4_mpage_readpages+0x4a8/0x7a8 [ext4]
[130747.336995] ext4_readpage+0x54/0x100 [ext4]
[130747.337359] generic_file_buffered_read+0x410/0xae8
[130747.337767] generic_file_read_iter+0x114/0x190
[130747.338152] ext4_file_read_iter+0x5c/0x140 [ext4]
[130747.338556] __vfs_read+0x11c/0x188
[130747.338851] vfs_read+0x94/0x150
[130747.339110] ksys_read+0x74/0xf0
If call ext4_ext_insert_extent failed but new extent already inserted, we just
update "ex->ee_len = orig_ex.ee_len", this will lead to extent overlap, then
cause bug on when cache extent.
If call ext4_ext_insert_extent failed don't update ex->ee_len with old value.
Maybe there will lead to block leak, but it can be fixed by fsck later.
After we fixed above issue with v2 patch, but we got the same issue.
ext4_split_extent_at:
{
......
err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
if (err == -ENOSPC && (EXT4_EXT_MAY_ZEROOUT & split_flag)) {
......
ext4_ext_try_to_merge(handle, inode, path, ex); ->step(1)
err = ext4_ext_dirty(handle, inode, path + path->p_depth); ->step(2)
if (err)
goto fix_extent_len;
......
}
......
fix_extent_len:
ex->ee_len = orig_ex.ee_len; ->step(3)
......
}
If step(1) have been merged, but step(2) dirty extent failed, then go to
fix_extent_len label to fix ex->ee_len with orig_ex.ee_len. But "ex" may not be
old one, will cause overwritten. Then will trigger the same issue as previous.
If step(2) failed, just return error, don't fix ex->ee_len with old value.
Signed-off-by: Ye Bin <[email protected]>
---
fs/ext4/extents.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 77c84d6f1af6..d4aa24a09d8b 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3238,15 +3238,12 @@ static int ext4_split_extent_at(handle_t *handle,
ex->ee_len = cpu_to_le16(ee_len);
ext4_ext_try_to_merge(handle, inode, path, ex);
err = ext4_ext_dirty(handle, inode, path + path->p_depth);
- if (err)
- goto fix_extent_len;
-
- /* update extent status tree */
- err = ext4_zeroout_es(inode, &zero_ex);
-
- goto out;
- } else if (err)
+ if (!err)
+ /* update extent status tree */
+ err = ext4_zeroout_es(inode, &zero_ex);
+ } else if (err && err != -EROFS) {
goto fix_extent_len;
+ }
out:
ext4_ext_show_leaf(inode, path);
--
2.25.4
On Wed 28-04-21 16:51:58, Ye Bin wrote:
> We got follow bug_on when run fsstress with injecting IO fault:
> [130747.323114] kernel BUG at fs/ext4/extents_status.c:762!
> [130747.323117] Internal error: Oops - BUG: 0 [#1] SMP
> ......
> [130747.334329] Call trace:
> [130747.334553] ext4_es_cache_extent+0x150/0x168 [ext4]
> [130747.334975] ext4_cache_extents+0x64/0xe8 [ext4]
> [130747.335368] ext4_find_extent+0x300/0x330 [ext4]
> [130747.335759] ext4_ext_map_blocks+0x74/0x1178 [ext4]
> [130747.336179] ext4_map_blocks+0x2f4/0x5f0 [ext4]
> [130747.336567] ext4_mpage_readpages+0x4a8/0x7a8 [ext4]
> [130747.336995] ext4_readpage+0x54/0x100 [ext4]
> [130747.337359] generic_file_buffered_read+0x410/0xae8
> [130747.337767] generic_file_read_iter+0x114/0x190
> [130747.338152] ext4_file_read_iter+0x5c/0x140 [ext4]
> [130747.338556] __vfs_read+0x11c/0x188
> [130747.338851] vfs_read+0x94/0x150
> [130747.339110] ksys_read+0x74/0xf0
>
> If call ext4_ext_insert_extent failed but new extent already inserted, we just
> update "ex->ee_len = orig_ex.ee_len", this will lead to extent overlap, then
> cause bug on when cache extent.
Thanks for the patch but I'm still not quite sure, how overlapping extents
in the extent tree can lead to triggering BUG_ON(lblk + len - 1 < lblk) in
ext4_es_cache_extent(). Can you ellaborate a bit more how this happens?
> If call ext4_ext_insert_extent failed don't update ex->ee_len with old value.
> Maybe there will lead to block leak, but it can be fixed by fsck later.
>
> After we fixed above issue with v2 patch, but we got the same issue.
> ext4_split_extent_at:
> {
> ......
> err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
> if (err == -ENOSPC && (EXT4_EXT_MAY_ZEROOUT & split_flag)) {
> ......
> ext4_ext_try_to_merge(handle, inode, path, ex); ->step(1)
> err = ext4_ext_dirty(handle, inode, path + path->p_depth); ->step(2)
> if (err)
> goto fix_extent_len;
> ......
> }
> ......
> fix_extent_len:
> ex->ee_len = orig_ex.ee_len; ->step(3)
> ......
> }
> If step(1) have been merged, but step(2) dirty extent failed, then go to
> fix_extent_len label to fix ex->ee_len with orig_ex.ee_len. But "ex" may not be
> old one, will cause overwritten. Then will trigger the same issue as previous.
> If step(2) failed, just return error, don't fix ex->ee_len with old value.
>
> Signed-off-by: Ye Bin <[email protected]>
> ---
> fs/ext4/extents.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 77c84d6f1af6..d4aa24a09d8b 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3238,15 +3238,12 @@ static int ext4_split_extent_at(handle_t *handle,
> ex->ee_len = cpu_to_le16(ee_len);
> ext4_ext_try_to_merge(handle, inode, path, ex);
> err = ext4_ext_dirty(handle, inode, path + path->p_depth);
> - if (err)
> - goto fix_extent_len;
> -
> - /* update extent status tree */
> - err = ext4_zeroout_es(inode, &zero_ex);
> -
> - goto out;
> - } else if (err)
> + if (!err)
> + /* update extent status tree */
> + err = ext4_zeroout_es(inode, &zero_ex);
> + } else if (err && err != -EROFS) {
I fail to see why EROFS is special here. Can you explain a bit please?
> goto fix_extent_len;
> + }
>
> out:
> ext4_ext_show_leaf(inode, path);
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 2021/5/5 18:41, Jan Kara wrote:
> On Wed 05-05-21 11:29:57, yebin wrote:
>>
>> On 2021/4/30 20:58, Jan Kara wrote:
>>> On Wed 28-04-21 16:51:58, Ye Bin wrote:
>>>> We got follow bug_on when run fsstress with injecting IO fault:
>>>> [130747.323114] kernel BUG at fs/ext4/extents_status.c:762!
>>>> [130747.323117] Internal error: Oops - BUG: 0 [#1] SMP
>>>> ......
>>>> [130747.334329] Call trace:
>>>> [130747.334553] ext4_es_cache_extent+0x150/0x168 [ext4]
>>>> [130747.334975] ext4_cache_extents+0x64/0xe8 [ext4]
>>>> [130747.335368] ext4_find_extent+0x300/0x330 [ext4]
>>>> [130747.335759] ext4_ext_map_blocks+0x74/0x1178 [ext4]
>>>> [130747.336179] ext4_map_blocks+0x2f4/0x5f0 [ext4]
>>>> [130747.336567] ext4_mpage_readpages+0x4a8/0x7a8 [ext4]
>>>> [130747.336995] ext4_readpage+0x54/0x100 [ext4]
>>>> [130747.337359] generic_file_buffered_read+0x410/0xae8
>>>> [130747.337767] generic_file_read_iter+0x114/0x190
>>>> [130747.338152] ext4_file_read_iter+0x5c/0x140 [ext4]
>>>> [130747.338556] __vfs_read+0x11c/0x188
>>>> [130747.338851] vfs_read+0x94/0x150
>>>> [130747.339110] ksys_read+0x74/0xf0
>>>>
>>>> If call ext4_ext_insert_extent failed but new extent already inserted, we just
>>>> update "ex->ee_len = orig_ex.ee_len", this will lead to extent overlap, then
>>>> cause bug on when cache extent.
>>> Thanks for the patch but I'm still not quite sure, how overlapping extents
>>> in the extent tree can lead to triggering BUG_ON(lblk + len - 1 < lblk) in
>>> ext4_es_cache_extent(). Can you ellaborate a bit more how this happens?
>> Assume that there is extent [10, 100] (ee_block=10 ee_len=91), call
>> ext4_split_extent_at split at 50,
>> we get two extent [10, 49] and [50, 100], then call ext4_ext_insert_extent
>> to insert new extent [50, 100],
>> if insert extent successed, but call ext4_ext_dirty failed(return -EROFS)
>> as JBD maybe abort as io error.
>> Then fix old extent length with old value, so we get two extent [10,
>> 100] (ee_block=10 ee_len=91) and
>> [50, 100](ee_block=50 ee_len=51).
>> If call ext4_cache_extent to cache above extents as follow:
>> prev = 0 lblk = 10 len = 91 --> cache [10, 100] ---> prev = lblk + len =
>> 101
>> prev = 101 lblk = 50 len = 51 --> prev != 0 && prev != lblk --> cache [prev
>> = 101, lblk - prev = 50 - 101 = -51]
>> Obvious if call ext4_es_cache_extent cache extent[101, -51] wil trigger
>> "BUG_ON(end < lblk)" .
> Thanks for great explanation! Now I understand.
>
>>>> If call ext4_ext_insert_extent failed don't update ex->ee_len with old value.
>>>> Maybe there will lead to block leak, but it can be fixed by fsck later.
>>>>
>>>> After we fixed above issue with v2 patch, but we got the same issue.
>>>> ext4_split_extent_at:
>>>> {
>>>> ......
>>>> err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
>>>> if (err == -ENOSPC && (EXT4_EXT_MAY_ZEROOUT & split_flag)) {
>>>> ......
>>>> ext4_ext_try_to_merge(handle, inode, path, ex); ->step(1)
>>>> err = ext4_ext_dirty(handle, inode, path + path->p_depth); ->step(2)
>>>> if (err)
>>>> goto fix_extent_len;
>>>> ......
>>>> }
>>>> ......
>>>> fix_extent_len:
>>>> ex->ee_len = orig_ex.ee_len; ->step(3)
>>>> ......
>>>> }
>>>> If step(1) have been merged, but step(2) dirty extent failed, then go to
>>>> fix_extent_len label to fix ex->ee_len with orig_ex.ee_len. But "ex" may not be
>>>> old one, will cause overwritten. Then will trigger the same issue as previous.
>>>> If step(2) failed, just return error, don't fix ex->ee_len with old value.
>>>>
>>>> Signed-off-by: Ye Bin <[email protected]>
>>>> ---
>>>> fs/ext4/extents.c | 13 +++++--------
>>>> 1 file changed, 5 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>>>> index 77c84d6f1af6..d4aa24a09d8b 100644
>>>> --- a/fs/ext4/extents.c
>>>> +++ b/fs/ext4/extents.c
>>>> @@ -3238,15 +3238,12 @@ static int ext4_split_extent_at(handle_t *handle,
>>>> ex->ee_len = cpu_to_le16(ee_len);
>>>> ext4_ext_try_to_merge(handle, inode, path, ex);
>>>> err = ext4_ext_dirty(handle, inode, path + path->p_depth);
>>>> - if (err)
>>>> - goto fix_extent_len;
>>>> -
>>>> - /* update extent status tree */
>>>> - err = ext4_zeroout_es(inode, &zero_ex);
>>>> -
>>>> - goto out;
>>>> - } else if (err)
>>>> + if (!err)
>>>> + /* update extent status tree */
>>>> + err = ext4_zeroout_es(inode, &zero_ex);
>>>> + } else if (err && err != -EROFS) {
>>> I fail to see why EROFS is special here. Can you explain a bit please?
>> V1 patch Ted suggest me to fix length only when "err != -EROSFS". As if
>> we don't
>> fix origin extent with old extent length, it will lead to block leak.
>> Ted said as follow:
>>
>> If you don't want to do that, then a "do no harm" fix would be
>> something like this:
>>
>> ...
>> } else if (err == -EROFS) {
>> return err;
>> } else if (err)
>> goto fix_extent_len;
>>
>> So in the journal abort case, when err is set to EROFS, we don't try
>> to reset the length, since in theory the file system is read-only
>> already anyway. However, in the ENOSPC case, we won't end up silently
>> leaking blocks that will be lost until the user somehow decides to run
>> fsck.
> I see. Now I understand your patch. Honestly, seeing how fragile is trying
> to fix extent tree after split has failed in the middle, I would probably
> go even further and make sure we fix the tree properly in case of ENOSPC
> and EDQUOT (those are easily user triggerable). Anything else indicates a
> HW problem or fs corruption so I'd rather leave the extent tree as is and
> don't try to fix it (which also means we will not create overlapping
> extents). So something like:
>
>
> err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
> - if (err == -ENOSPC && (EXT4_EXT_MAY_ZEROOUT & split_flag)) {
> + if (err == -ENOSPC || err == -EDQUOT) {
> + if (EXT4_EXT_MAY_ZEROOUT & split_flag)
> + err = handle zeroing...
> if (err) {
> fix extent len
> goto out;
> }
> ...
> }
>
> and in all other cases just 'goto out' in case of error. What do you think?
>
> Honza
Thanks for your suggesttion. If you have no objection to following
modification, i will send it as V4.
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 77c84d6f1af6..f9cbd11e1eae 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3206,7 +3206,10 @@ static int ext4_split_extent_at(handle_t *handle,
ext4_ext_mark_unwritten(ex2);
err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
- if (err == -ENOSPC && (EXT4_EXT_MAY_ZEROOUT & split_flag)) {
+ if (err != -ENOSPC && err != -EDQUOT)
+ goto out;
+
+ if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
if (split_flag &
(EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) {
if (split_flag & EXT4_EXT_DATA_VALID1) {
err = ext4_ext_zeroout(inode, ex2);
@@ -3232,22 +3235,23 @@ static int ext4_split_extent_at(handle_t *handle,
ext4_ext_pblock(&orig_ex));
}
- if (err)
- goto fix_extent_len;
- /* update the extent length and mark as initialized */
- ex->ee_len = cpu_to_le16(ee_len);
- ext4_ext_try_to_merge(handle, inode, path, ex);
- err = ext4_ext_dirty(handle, inode, path + path->p_depth);
- if (err)
- goto fix_extent_len;
-
- /* update extent status tree */
- err = ext4_zeroout_es(inode, &zero_ex);
-
- goto out;
- } else if (err)
- goto fix_extent_len;
-
+ if (!err) {
+ /* update the extent length and mark as
initialized */
+ ex->ee_len = cpu_to_le16(ee_len);
+ ext4_ext_try_to_merge(handle, inode, path, ex);
+ err = ext4_ext_dirty(handle, inode, path +
path->p_depth);
+ if (!err)
+ /* update extent status tree */
+ err = ext4_zeroout_es(inode, &zero_ex);
+ /* At here, ext4_ext_try_to_merge maybe already
merge
+ * extent, if fix origin extent length may lead to
+ * overwritten.
+ */
+ goto out;
+ }
+ }
+ if (err)
+ goto fix_extent_len;
out:
ext4_ext_show_leaf(inode, path);
return err;
On Thu 06-05-21 16:26:24, yebin wrote:
> Thanks for your suggesttion. If you have no objection to following
> modification, i will send it as V4.
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 77c84d6f1af6..f9cbd11e1eae 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3206,7 +3206,10 @@ static int ext4_split_extent_at(handle_t *handle,
> ext4_ext_mark_unwritten(ex2);
>
> err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
> - if (err == -ENOSPC && (EXT4_EXT_MAY_ZEROOUT & split_flag)) {
> + if (err != -ENOSPC && err != -EDQUOT)
> + goto out;
> +
> + if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
You need:
if (err && (EXT4_EXT_MAY_ZEROOUT & split_flag))
there, don't you? You don't want to zero-out if there's no error.
> @@ -3232,22 +3235,23 @@ static int ext4_split_extent_at(handle_t *handle,
> ext4_ext_pblock(&orig_ex));
> }
>
> - if (err)
> - goto fix_extent_len;
> - /* update the extent length and mark as initialized */
> - ex->ee_len = cpu_to_le16(ee_len);
> - ext4_ext_try_to_merge(handle, inode, path, ex);
> - err = ext4_ext_dirty(handle, inode, path + path->p_depth);
> - if (err)
> - goto fix_extent_len;
> -
> - /* update extent status tree */
> - err = ext4_zeroout_es(inode, &zero_ex);
> -
> - goto out;
> - } else if (err)
> - goto fix_extent_len;
> -
> + if (!err) {
> + /* update the extent length and mark as initialized
> */
> + ex->ee_len = cpu_to_le16(ee_len);
> + ext4_ext_try_to_merge(handle, inode, path, ex);
> + err = ext4_ext_dirty(handle, inode, path +
> path->p_depth);
> + if (!err)
> + /* update extent status tree */
> + err = ext4_zeroout_es(inode, &zero_ex);
> + /* At here, ext4_ext_try_to_merge maybe already
> merge
> + * extent, if fix origin extent length may lead to
> + * overwritten.
> + */
I'd rephrase the comment as:
/*
* If we failed at this point, we don't know in which state the extent tree
* exactly is so don't try to fix length of the original extent as it may do
* even more damage.
*/
> + goto out;
> + }
> + }
> + if (err)
> + goto fix_extent_len;
And you can move this if (err) before if (!err) above to make code easier
to read and save one indentation level.
> out:
> ext4_ext_show_leaf(inode, path);
> return err;
>
>
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 2021/5/6 18:19, Jan Kara wrote:
> On Thu 06-05-21 16:26:24, yebin wrote:
>> Thanks for your suggesttion. If you have no objection to following
>> modification, i will send it as V4.
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 77c84d6f1af6..f9cbd11e1eae 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -3206,7 +3206,10 @@ static int ext4_split_extent_at(handle_t *handle,
>> ext4_ext_mark_unwritten(ex2);
>>
>> err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
>> - if (err == -ENOSPC && (EXT4_EXT_MAY_ZEROOUT & split_flag)) {
>> + if (err != -ENOSPC && err != -EDQUOT)
>> + goto out;
>> +
>> + if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
> You need:
>
> if (err && (EXT4_EXT_MAY_ZEROOUT & split_flag))
>
> there, don't you? You don't want to zero-out if there's no error.
If (err != -ENOSPC && err != -EDQUOT) already goto out, so there is needn't
judge "err" again.
>
>> @@ -3232,22 +3235,23 @@ static int ext4_split_extent_at(handle_t *handle,
>> ext4_ext_pblock(&orig_ex));
>> }
>>
>> - if (err)
>> - goto fix_extent_len;
>> - /* update the extent length and mark as initialized */
>> - ex->ee_len = cpu_to_le16(ee_len);
>> - ext4_ext_try_to_merge(handle, inode, path, ex);
>> - err = ext4_ext_dirty(handle, inode, path + path->p_depth);
>> - if (err)
>> - goto fix_extent_len;
>> -
>> - /* update extent status tree */
>> - err = ext4_zeroout_es(inode, &zero_ex);
>> -
>> - goto out;
>> - } else if (err)
>> - goto fix_extent_len;
>> -
>> + if (!err) {
>> + /* update the extent length and mark as initialized
>> */
>> + ex->ee_len = cpu_to_le16(ee_len);
>> + ext4_ext_try_to_merge(handle, inode, path, ex);
>> + err = ext4_ext_dirty(handle, inode, path +
>> path->p_depth);
>> + if (!err)
>> + /* update extent status tree */
>> + err = ext4_zeroout_es(inode, &zero_ex);
>> + /* At here, ext4_ext_try_to_merge maybe already
>> merge
>> + * extent, if fix origin extent length may lead to
>> + * overwritten.
>> + */
> I'd rephrase the comment as:
>
> /*
> * If we failed at this point, we don't know in which state the extent tree
> * exactly is so don't try to fix length of the original extent as it may do
> * even more damage.
> */
I will replace it with your comment.
>
>> + goto out;
>> + }
>> + }
>> + if (err)
>> + goto fix_extent_len;
> And you can move this if (err) before if (!err) above to make code easier
> to read and save one indentation level.
if (EXT4_EXT_MAY_ZEROOUT & split_flag) do zero-out, if failed, we
don't need fix extent length.
But if (!EXT4_EXT_MAY_ZEROOUT & split_flag) we need to fix extent
length. Maybe i can move
label "out" behind label "fix_extent_len", then this judement can be
removed.
Did i misunderstand what you meant earlier?
>> out:
>> ext4_ext_show_leaf(inode, path);
>> return err;
>>
>>
> Honza
The diff as following:
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 77c84d6f1af6..cbf37b2cf871 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3206,7 +3206,10 @@ static int ext4_split_extent_at(handle_t *handle,
ext4_ext_mark_unwritten(ex2);
err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
- if (err == -ENOSPC && (EXT4_EXT_MAY_ZEROOUT & split_flag)) {
+ if (err != -ENOSPC && err != -EDQUOT)
+ goto out;
+
+ if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
if (split_flag &
(EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) {
if (split_flag & EXT4_EXT_DATA_VALID1) {
err = ext4_ext_zeroout(inode, ex2);
@@ -3232,25 +3235,22 @@ static int ext4_split_extent_at(handle_t *handle,
ext4_ext_pblock(&orig_ex));
}
- if (err)
- goto fix_extent_len;
- /* update the extent length and mark as initialized */
- ex->ee_len = cpu_to_le16(ee_len);
- ext4_ext_try_to_merge(handle, inode, path, ex);
- err = ext4_ext_dirty(handle, inode, path + path->p_depth);
- if (err)
- goto fix_extent_len;
-
- /* update extent status tree */
- err = ext4_zeroout_es(inode, &zero_ex);
-
- goto out;
- } else if (err)
- goto fix_extent_len;
-
-out:
- ext4_ext_show_leaf(inode, path);
- return err;
+ if (!err) {
+ /* update the extent length and mark as
initialized */
+ ex->ee_len = cpu_to_le16(ee_len);
+ ext4_ext_try_to_merge(handle, inode, path, ex);
+ err = ext4_ext_dirty(handle, inode, path +
path->p_depth);
+ if (!err)
+ /* update extent status tree */
+ err = ext4_zeroout_es(inode, &zero_ex);
+ /* If we failed at this point, we don't know in
which
+ * state the extent tree exactly is so don't try
to fix
+ * length of the original extent as it may do
even more
+ * damage.
+ */
+ goto out;
+ }
+ }
fix_extent_len:
ex->ee_len = orig_ex.ee_len;
@@ -3260,6 +3260,9 @@ static int ext4_split_extent_at(handle_t *handle,
*/
ext4_ext_dirty(handle, inode, path + path->p_depth);
return err;
+out:
+ ext4_ext_show_leaf(inode, path);
+ return err;
}
The whole code as folowing:
ext4_split_extent_at:
.......
3208 err = ext4_ext_insert_extent(handle, inode, ppath, &newex,
flags);
3209 if (err != -ENOSPC && err != -EDQUOT)
3210 goto out;
3211
3212 if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
3213 if (split_flag &
(EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) {
3214 if (split_flag & EXT4_EXT_DATA_VALID1) {
3215 err = ext4_ext_zeroout(inode, ex2);
3216 zero_ex.ee_block = ex2->ee_block;
3217 zero_ex.ee_len = cpu_to_le16(
3218 ext4_ext_get_actual_len(ex2));
3219 ext4_ext_store_pblock(&zero_ex,
3220 ext4_ext_pblock(ex2));
3221 } else {
3222 err = ext4_ext_zeroout(inode, ex);
3223 zero_ex.ee_block = ex->ee_block;
3224 zero_ex.ee_len = cpu_to_le16(
3225 ext4_ext_get_actual_len(ex));
3226 ext4_ext_store_pblock(&zero_ex,
3227 ext4_ext_pblock(ex));
3228 }
3229 } else {
3230 err = ext4_ext_zeroout(inode, &orig_ex);
3231 zero_ex.ee_block = orig_ex.ee_block;
3232 zero_ex.ee_len = cpu_to_le16(
3233 ext4_ext_get_actual_len(&orig_ex));
3234 ext4_ext_store_pblock(&zero_ex,
3235 ext4_ext_pblock(&orig_ex));
3236 }
3237
3238 if (!err) {
3239 /* update the extent length and mark as
initialized */
3240 ex->ee_len = cpu_to_le16(ee_len);
3241 ext4_ext_try_to_merge(handle, inode, path, ex);
3242 err = ext4_ext_dirty(handle, inode, path +
path->p_depth);
3243 if (!err)
3244 /* update extent status tree */
3245 err = ext4_zeroout_es(inode, &zero_ex);
3246 /* If we failed at this point, we don't
know in which
3247 * state the extent tree exactly is so
don't try to fix
3248 * length of the original extent as it may
do even more
3249 * damage.
3250 */
3251 goto out;
3252 }
3253 }
3254
3255 fix_extent_len:
3256 ex->ee_len = orig_ex.ee_len;
3257 /*
3258 * Ignore ext4_ext_dirty return value since we are already
in error path
3259 * and err is a non-zero error code.
3260 */
3261 ext4_ext_dirty(handle, inode, path + path->p_depth);
3262 return err;
3263 out:
3264 ext4_ext_show_leaf(inode, path);
3265 return err;
3266 }
On Thu 06-05-21 19:47:11, yebin wrote:
>
>
> On 2021/5/6 18:19, Jan Kara wrote:
> > On Thu 06-05-21 16:26:24, yebin wrote:
> > > Thanks for your suggesttion. If you have no objection to following
> > > modification, i will send it as V4.
> > >
> > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > > index 77c84d6f1af6..f9cbd11e1eae 100644
> > > --- a/fs/ext4/extents.c
> > > +++ b/fs/ext4/extents.c
> > > @@ -3206,7 +3206,10 @@ static int ext4_split_extent_at(handle_t *handle,
> > > ext4_ext_mark_unwritten(ex2);
> > >
> > > err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
> > > - if (err == -ENOSPC && (EXT4_EXT_MAY_ZEROOUT & split_flag)) {
> > > + if (err != -ENOSPC && err != -EDQUOT)
> > > + goto out;
> > > +
> > > + if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
> > You need:
> >
> > if (err && (EXT4_EXT_MAY_ZEROOUT & split_flag))
> >
> > there, don't you? You don't want to zero-out if there's no error.
>
> If (err != -ENOSPC && err != -EDQUOT) already goto out, so there is needn't
> judge "err" again.
Right, my fault. I was confused.
> > > @@ -3232,22 +3235,23 @@ static int ext4_split_extent_at(handle_t *handle,
> > > ext4_ext_pblock(&orig_ex));
> > > }
> > >
> > > - if (err)
> > > - goto fix_extent_len;
> > > - /* update the extent length and mark as initialized */
> > > - ex->ee_len = cpu_to_le16(ee_len);
> > > - ext4_ext_try_to_merge(handle, inode, path, ex);
> > > - err = ext4_ext_dirty(handle, inode, path + path->p_depth);
> > > - if (err)
> > > - goto fix_extent_len;
> > > -
> > > - /* update extent status tree */
> > > - err = ext4_zeroout_es(inode, &zero_ex);
> > > -
> > > - goto out;
> > > - } else if (err)
> > > - goto fix_extent_len;
> > > -
> > > + if (!err) {
> > > + /* update the extent length and mark as initialized
> > > */
> > > + ex->ee_len = cpu_to_le16(ee_len);
> > > + ext4_ext_try_to_merge(handle, inode, path, ex);
> > > + err = ext4_ext_dirty(handle, inode, path +
> > > path->p_depth);
> > > + if (!err)
> > > + /* update extent status tree */
> > > + err = ext4_zeroout_es(inode, &zero_ex);
> > > + /* At here, ext4_ext_try_to_merge maybe already
> > > merge
> > > + * extent, if fix origin extent length may lead to
> > > + * overwritten.
> > > + */
> > I'd rephrase the comment as:
> >
> > /*
> > * If we failed at this point, we don't know in which state the extent tree
> > * exactly is so don't try to fix length of the original extent as it may do
> > * even more damage.
> > */
> I will replace it with your comment.
> >
> > > + goto out;
> > > + }
> > > + }
> > > + if (err)
> > > + goto fix_extent_len;
> > And you can move this if (err) before if (!err) above to make code easier
> > to read and save one indentation level.
> if (EXT4_EXT_MAY_ZEROOUT & split_flag) do zero-out, if failed, we don't
> need fix extent length.
> But if (!EXT4_EXT_MAY_ZEROOUT & split_flag) we need to fix extent length.
> Maybe i can move
> label "out" behind label "fix_extent_len", then this judement can be
> removed.
> Did i misunderstand what you meant earlier?
Thanks for the update! The diff now looks good to me so feel free to add:
Reviewed-by: Jan Kara <[email protected]>
for your next posting.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR