ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed
PATCH v3 1/1 ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed
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.
--- fs/ext4/extents.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
fs/ext4/extents.c +5 -8
@@ -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);
Signature · 1 line
2.25.4
Jan Kara
reviewerQuoted 19 lines · Ye Bin
We got follow
bug_onwhen 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
Quoted 3 lines
If call
ext4_ext_insert_extentfailed 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?
Quoted 41 lines
If call
ext4_ext_insert_extentfailed don't updateex->ee_lenwith 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_lenlabel to fixex->ee_lenwith 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 fixex->ee_lenwith old value.Signed-off-by Ye Bin <[email protected]>--- fs/ext4/extents.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)fs/ext4/extents.c +0
-2@@ -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;
fs/ext4/extents.c static int ext4_split_extent_at(handle_t *handle, · line 3244
Quoted 10 lines
fs/ext4/extents.c +4
-6- -/* 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?
fs/ext4/extents.c static int ext4_split_extent_at(handle_t *handle, · line 3249
Quoted 5 lines
fs/ext4/extents.c +1
-0goto fix_extent_len; + } out:ext4_ext_show_leaf (inode, path);
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 20 lines · Jan Kara
Quoted 19 lines · Ye Bin
We got follow
bug_onwhen 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
Quoted 6 lines
Quoted 3 lines
If call
ext4_ext_insert_extentfailed 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)inext4_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)" .
Quoted 44 lines
Quoted 44 lines
If call
ext4_ext_insert_extentfailed don't updateex->ee_lenwith 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_lenlabel to fixex->ee_lenwith 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 fixex->ee_lenwith old value.Signed-off-by Ye Bin <[email protected]>--- fs/ext4/extents.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)fs/ext4/extents.c +0
-5@@ -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);
Quoted 8 lines
fs/ext4/extents.c static int ext4_split_extent_at(handle_t *handle, · line 3244
Quoted 7 lines
fs/ext4/extents.c +4
-3- - 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.
Quoted 6 lines
fs/ext4/extents.c static int ext4_split_extent_at(handle_t *handle, · line 3249
Quoted 5 lines
fs/ext4/extents.c +1
-0goto fix_extent_len; + } out:ext4_ext_show_leaf (inode, path);Honza
Jan Kara
reviewerQuoted 29 lines · yebin
Quoted 20 lines · Jan Kara
Quoted 19 lines · Ye Bin
We got follow
bug_onwhen 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/0xf0Quoted 6 lines
Quoted 3 lines
If call
ext4_ext_insert_extentfailed 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)inext4_es_cache_extent(). Can you ellaborate a bit more how this happens?
Quoted 16 lines
Assume that there is extent [10, 100] (ee_block=10 ee_len=91), call
ext4_split_extent_atsplit at 50,
we get two extent [10, 49] and [50, 100], then callext4_ext_insert_extentto insert new extent [50, 100],
if insert extent successed, but callext4_ext_dirtyfailed(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 callext4_cache_extentto 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 callext4_es_cache_extentcache extent[101, -51] wil trigger "BUG_ON(end < lblk)" .
Thanks for great explanation! Now I understand.
Quoted 66 lines
Quoted 44 lines
Quoted 44 lines
If call
ext4_ext_insert_extentfailed don't updateex->ee_lenwith 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_lenlabel to fixex->ee_lenwith 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 fixex->ee_lenwith old value.Signed-off-by Ye Bin <[email protected]>--- fs/ext4/extents.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)fs/ext4/extents.c +0
-5@@ -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);Quoted 8 lines
fs/ext4/extents.c static int ext4_split_extent_at(handle_t *handle, · line 3244
Quoted 7 lines
fs/ext4/extents.c +4
-3- - 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;
Quoted 5 lines
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
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 139 lines · Jan Kara
Quoted 28 lines · yebin
Quoted 20 lines · Jan Kara
Quoted 19 lines · Ye Bin
We got follow
bug_onwhen 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/0xf0Quoted 6 lines
Quoted 3 lines
If call
ext4_ext_insert_extentfailed 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)inext4_es_cache_extent(). Can you ellaborate a bit more how this happens?Quoted 16 lines
Assume that there is extent [10, 100] (ee_block=10 ee_len=91), call
ext4_split_extent_atsplit at 50,
we get two extent [10, 49] and [50, 100], then callext4_ext_insert_extentto insert new extent [50, 100],
if insert extent successed, but callext4_ext_dirtyfailed(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 callext4_cache_extentto 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 callext4_es_cache_extentcache extent[101, -51] wil trigger "BUG_ON(end < lblk)" .Thanks for great explanation! Now I understand.
Quoted 66 lines
Quoted 44 lines
Quoted 44 lines
If call
ext4_ext_insert_extentfailed don't updateex->ee_lenwith 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_lenlabel to fixex->ee_lenwith 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 fixex->ee_lenwith old value.Signed-off-by Ye Bin <[email protected]>--- fs/ext4/extents.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)fs/ext4/extents.c +0
-5@@ -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);Quoted 8 lines
fs/ext4/extents.c static int ext4_split_extent_at(handle_t *handle, · line 3244
Quoted 7 lines
fs/ext4/extents.c +4
-3- - 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;Quoted 5 lines
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);(quoted diff) +3
-1- 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; } ... }
Quoted 3 lines
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.
fs/ext4/extents.c +4 -1
@@ -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;Jan Kara
reviewerQuoted 8 lines · yebin
Thanks for your suggesttion. If you have no objection to following modification, i will send it as V4.
fs/ext4/extents.c +0
-0
fs/ext4/extents.c static int ext4_split_extent_at(handle_t *handle, · line 3212
Quoted 8 lines
fs/ext4/extents.c +4
-1@@ -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.
Quoted 32 lines
fs/ext4/extents.c +9
-16@@ -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
fs/ext4/extents.c
Quoted 3 lines
fs/ext4/extents.c +3
-0+* 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.
*/
fs/ext4/extents.c
Quoted 5 lines
fs/ext4/extents.c +5
-0+ 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.
Quoted 5 lines
out:
ext4_ext_show_leaf(inode, path); return err;
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 19 lines · Jan Kara
Quoted 8 lines · yebin
Thanks for your suggesttion. If you have no objection to following modification, i will send it as V4.
fs/ext4/extents.c +0
-0fs/ext4/extents.c static int ext4_split_extent_at(handle_t *handle, · line 3212
Quoted 8 lines
fs/ext4/extents.c +4
-1@@ -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:
Quoted 3 lines
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.
Quoted 38 lines
Quoted 33 lines
fs/ext4/extents.c +9
-16@@ -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
fs/ext4/extents.c
Quoted 3 lines
fs/ext4/extents.c +3
-0+* extent, if fix origin extent length may lead to +* overwritten. +*/ I'd rephrase the comment as:
Quoted 5 lines
/*
* 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.
Quoted 8 lines
fs/ext4/extents.c
Quoted 6 lines
fs/ext4/extents.c +5
-0+ 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?
Quoted 6 lines
Quoted 5 lines
out: ext4_ext_show_leaf(inode, path); return err;Honza
The diff as following:
fs/ext4/extents.c +4 -1
@@ -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 }
Quoted 22 lines · yebin
Quoted 19 lines · Jan Kara
Quoted 8 lines · yebin
Thanks for your suggesttion. If you have no objection to following modification, i will send it as V4.
fs/ext4/extents.c +0
-0fs/ext4/extents.c static int ext4_split_extent_at(handle_t *handle, · line 3212
Quoted 8 lines
fs/ext4/extents.c +4
-1@@ -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:
Quoted 6 lines
Quoted 3 lines
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.
Quoted 51 lines
Quoted 37 lines
Quoted 32 lines
fs/ext4/extents.c +9
-16@@ -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
fs/ext4/extents.c
Quoted 3 lines
fs/ext4/extents.c +3
-0+* extent, if fix origin extent length may lead to +* overwritten. +*/ I'd rephrase the comment as:
Quoted 5 lines
/*
* 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.
Quoted 8 lines
fs/ext4/extents.c
Quoted 6 lines
fs/ext4/extents.c +5
-0+ 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.
Quoted 7 lines
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:
for your next posting.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR