[PATCH v3 0/2] fix extents need to be restored when ext4_ext_insert_extent failed

[PATCH v3 0/2] fix extents need to be restored when ext4_ext_insert_extent failed

[zhanchengbin]

Inside the ext4_ext_insert_extent function, every error returned will
not destroy the consistency of the tree. Even if it fails after changing
half of the tree, can also ensure that the tree is self-consistent, like
function ext4_ext_create_new_leaf.
After ext4_ext_insert_extent fails, update extent status tree depends on
the incoming split_flag. So restore the len of extent to be split when
ext4_ext_insert_extent return failed in ext4_split_extent_at.

Diff v2 Vs v1:
1) return directly after inserting successfully
2) restore the length of extent in memory after inserting unsuccessfully

Diff v3 Vs v2:
Sorry for not taking into account the successful extent insertion. and I
reanalyzed the ext4_ext_insert_extent function and modified the conditions
for restoring the length.

zhanchengbin (2):
  ext4: fix inode tree inconsistency caused by ENOMEM in
    ext4_split_extent_at
  ext4: restore len when ext4_ext_insert_extent failed

 fs/ext4/extents.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH v3 1/2] ext4: fix inode tree inconsistency caused by ENOMEM in ext4_split_extent_at

[zhanchengbin]

If ENOMEM fails when the extent is splitting, we need to restore the length
of the split extent.
In the call stack of the ext4_split_extent_at function, only in
ext4_ext_create_new_leaf will it alloc memory and change the shape of the
extent tree,even if an ENOMEM is returned at this time, the extent tree is
still self-consistent, Just restore the split extent lens in the function
ext4_split_extent_at.

ext4_split_extent_at
 ext4_ext_insert_extent
  ext4_ext_create_new_leaf
   1)ext4_ext_split
     ext4_find_extent
   2)ext4_ext_grow_indepth
     ext4_find_extent

Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/extents.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 9de1c9d1a13d..3559ea6b0781 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3251,7 +3251,7 @@ 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 && err != -EDQUOT)
+	if (err != -ENOSPC && err != -EDQUOT && err != -ENOMEM)
 		goto out;
 
 	if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
-- 
2.31.1

[PATCH v3 2/2] ext4: restore len when ext4_ext_insert_extent failed

[zhanchengbin]

Inside the ext4_ext_insert_extent function, every error returned will
not destroy the consistency of the tree. Even if it fails after changing
half of the tree, can also ensure that the tree is self-consistent, like
function ext4_ext_create_new_leaf.
After ext4_ext_insert_extent fails, update extent status tree depends on
the incoming split_flag. So restore the len of extent to be split when
ext4_ext_insert_extent return failed in ext4_split_extent_at.

Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/extents.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 3559ea6b0781..b926fef73de4 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -935,6 +935,7 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
 
 		bh = read_extent_tree_block(inode, path[ppos].p_idx, --i, flags);
 		if (IS_ERR(bh)) {
+			EXT4_ERROR_INODE(inode, "IO error reading extent block");
 			ret = PTR_ERR(bh);
 			goto err;
 		}
@@ -3251,7 +3252,7 @@ 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 && err != -EDQUOT && err != -ENOMEM)
+	if (!err)
 		goto out;
 
 	if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
-- 
2.31.1

Re: [PATCH v3 2/2] ext4: restore len when ext4_ext_insert_extent failed

[Jan Kara]

On Tue 07-02-23 15:09:31, zhanchengbin wrote:
> Inside the ext4_ext_insert_extent function, every error returned will
> not destroy the consistency of the tree. Even if it fails after changing
> half of the tree, can also ensure that the tree is self-consistent, like
> function ext4_ext_create_new_leaf.

Hum, but e.g. if ext4_ext_correct_indexes() fails, we *will* end up with
corrupted extent tree pretty much without a chance for recovery, won't we?

								Honza

> After ext4_ext_insert_extent fails, update extent status tree depends on
> the incoming split_flag. So restore the len of extent to be split when
> ext4_ext_insert_extent return failed in ext4_split_extent_at.
> 
> Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> ---
>  fs/ext4/extents.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 3559ea6b0781..b926fef73de4 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -935,6 +935,7 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
>  
>  		bh = read_extent_tree_block(inode, path[ppos].p_idx, --i, flags);
>  		if (IS_ERR(bh)) {
> +			EXT4_ERROR_INODE(inode, "IO error reading extent block");
>  			ret = PTR_ERR(bh);
>  			goto err;
>  		}
> @@ -3251,7 +3252,7 @@ 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 && err != -EDQUOT && err != -ENOMEM)
> +	if (!err)
>  		goto out;
>  
>  	if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 2/2] ext4: restore len when ext4_ext_insert_extent failed

[zhanchengbin]

Thanks for your comments.
I've analyzed this situation, If a failure occurs at a certain layer, the
start of the upper and lower logical blocks is different, this's same as
ext4_ext_rm_idx.
If this happens data is not flushed to disks so data on disks is
consistent, but data on the memory is inconsistent (have journal). In my
opinion, we just need to ensure that we don't use the wrong data and flush
to disk. Look code we can know if ext4_ext_get_access and ext4_ext_dirty
faild, the verified flag of bh will be cleared, if read this bad inode
again, read_extent_tree_block will check verified flag and goto
__ext4_ext_check, finally, return error in the ext4_valid_extent_entries
function if the logical block start is incorrect, So does not change the
consistency of data on the disk. (Emmmmmm, I misunderstand the judgment in
ext4_valid_extent_entries. Later, I will clear the verified flag from the
modified bh when ext4_valid_extent_entries fails.)
If no journal, the data on the disk is inconsistent, too. Can use fsck to
fix it.
What do you think?

  - bin.

On 2023/2/7 22:23, Jan Kara wrote:
> On Tue 07-02-23 15:09:31, zhanchengbin wrote:
>> Inside the ext4_ext_insert_extent function, every error returned will
>> not destroy the consistency of the tree. Even if it fails after changing
>> half of the tree, can also ensure that the tree is self-consistent, like
>> function ext4_ext_create_new_leaf.
> 
> Hum, but e.g. if ext4_ext_correct_indexes() fails, we *will* end up with
> corrupted extent tree pretty much without a chance for recovery, won't we?
> 
> 								Honza
> 
>> After ext4_ext_insert_extent fails, update extent status tree depends on
>> the incoming split_flag. So restore the len of extent to be split when
>> ext4_ext_insert_extent return failed in ext4_split_extent_at.
>>
>> Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> ---
>>   fs/ext4/extents.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 3559ea6b0781..b926fef73de4 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -935,6 +935,7 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
>>   
>>   		bh = read_extent_tree_block(inode, path[ppos].p_idx, --i, flags);
>>   		if (IS_ERR(bh)) {
>> +			EXT4_ERROR_INODE(inode, "IO error reading extent block");
>>   			ret = PTR_ERR(bh);
>>   			goto err;
>>   		}
>> @@ -3251,7 +3252,7 @@ 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 && err != -EDQUOT && err != -ENOMEM)
>> +	if (!err)
>>   		goto out;
>>   
>>   	if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
>> -- 
>> 2.31.1
>>

Re: [PATCH v3 2/2] ext4: restore len when ext4_ext_insert_extent failed

[Jan Kara]

On Wed 08-02-23 15:10:35, zhanchengbin wrote:
> Thanks for your comments.
> I've analyzed this situation, If a failure occurs at a certain layer, the
> start of the upper and lower logical blocks is different, this's same as
> ext4_ext_rm_idx.
> If this happens data is not flushed to disks so data on disks is
> consistent, but data on the memory is inconsistent (have journal). In my
> opinion, we just need to ensure that we don't use the wrong data and flush
> to disk. Look code we can know if ext4_ext_get_access and ext4_ext_dirty
> faild, the verified flag of bh will be cleared, if read this bad inode
> again, read_extent_tree_block will check verified flag and goto
> __ext4_ext_check, finally, return error in the ext4_valid_extent_entries
> function if the logical block start is incorrect, So does not change the
> consistency of data on the disk. (Emmmmmm, I misunderstand the judgment in
> ext4_valid_extent_entries. Later, I will clear the verified flag from the
> modified bh when ext4_valid_extent_entries fails.)
> If no journal, the data on the disk is inconsistent, too. Can use fsck to
> fix it.
> What do you think?

So I agree that as soon as we abort the journal, modified data cannot get
to the disk and so we will not be writing inconsistent extent tree to the
disk. But we could still succeed in submitting requests to zero-out parts
of existing extent and that may corrupt the filesystem if the journal is
already aborted at that moment and gets replayed to some previous, not
quite known state.  So in that case is there any point in trying to fixup
anything?  The only occasions where it makes sense trying to keep extent
tree consistent is during some non-catastrophical errors - currently we
have ENOSPC, EDQUOT, ENOMEM - which make sense because from these we should
better recover without corrupting the filesystem. But I don't really see
any point in trying to fixup the "catastrophical" errors like EIO or
EFSCORRUPTED, it can do only harm.

								Honza
> On 2023/2/7 22:23, Jan Kara wrote:
> > On Tue 07-02-23 15:09:31, zhanchengbin wrote:
> > > Inside the ext4_ext_insert_extent function, every error returned will
> > > not destroy the consistency of the tree. Even if it fails after changing
> > > half of the tree, can also ensure that the tree is self-consistent, like
> > > function ext4_ext_create_new_leaf.
> > 
> > Hum, but e.g. if ext4_ext_correct_indexes() fails, we *will* end up with
> > corrupted extent tree pretty much without a chance for recovery, won't we?
> > 
> > 								Honza
> > 
> > > After ext4_ext_insert_extent fails, update extent status tree depends on
> > > the incoming split_flag. So restore the len of extent to be split when
> > > ext4_ext_insert_extent return failed in ext4_split_extent_at.
> > > 
> > > Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
> > > Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> > > ---
> > >   fs/ext4/extents.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > > index 3559ea6b0781..b926fef73de4 100644
> > > --- a/fs/ext4/extents.c
> > > +++ b/fs/ext4/extents.c
> > > @@ -935,6 +935,7 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
> > >   		bh = read_extent_tree_block(inode, path[ppos].p_idx, --i, flags);
> > >   		if (IS_ERR(bh)) {
> > > +			EXT4_ERROR_INODE(inode, "IO error reading extent block");
> > >   			ret = PTR_ERR(bh);
> > >   			goto err;
> > >   		}
> > > @@ -3251,7 +3252,7 @@ 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 && err != -EDQUOT && err != -ENOMEM)
> > > +	if (!err)
> > >   		goto out;
> > >   	if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
> > > -- 
> > > 2.31.1
> > > 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR