[PATCH v3] ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed

[PATCH v3] ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed

[Ye Bin]

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 <yebin10@huawei.com>
---
 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

Re: [PATCH v3] ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed

[Jan Kara]

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 <yebin10@huawei.com>
> ---
>  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 <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3] ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed

[yebin]

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)" .
>> 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 <yebin10@huawei.com>
>> ---
>>   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.


>>   		goto fix_extent_len;
>> +	}
>>   
>>   out:
>>   	ext4_ext_show_leaf(inode, path);
> 								Honza

Re: [PATCH v3] ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed

[Jan Kara]

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 <yebin10@huawei.com>
> > > ---
> > >   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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3] ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed

[yebin]

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 <yebin10@huawei.com>
>>>> ---
>>>>    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;

Re: [PATCH v3] ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed

[Jan Kara]

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 <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3] ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed

[yebin]

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 }

Re: [PATCH v3] ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed

[Jan Kara]

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 <jack@suse.cz>

for your next posting.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR