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

[PATCH v5 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.

Diff v4 Vs v3:
Clear the verified flag from the modified bh when failed inext4_ext_rm_idx
or ext4_ext_correct_indexes.

Diff v5 Vs v4:
Change path[k]->p_bh to path[k].p_bh.

zhanchengbin (2):
  ext4: fix inode tree inconsistency caused by ENOMEM in
    ext4_split_extent_at
  ext4: clear the verified flag of the modified leaf or idx if error

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

-- 
2.31.1

[PATCH v5 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>
---
 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 9de1c9d1a13d..0f95e857089e 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)
+	if (err != -ENOSPC && err != -EDQUOT && err != -ENOMEM)
 		goto out;
 
 	if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
-- 
2.31.1

[PATCH v5 2/2] ext4: clear the verified flag of the modified leaf or idx if error

[zhanchengbin]

Clear the verified flag from the modified bh when failed in ext4_ext_rm_idx
or ext4_ext_correct_indexes.
In this way, the start value of the logical block itself and its
parents' will be checked in ext4_valid_extent_entries.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
Link: https://lore.kernel.org/oe-kbuild-all/202302131414.5RKeHgAZ-lkp@intel.com/
Link: https://lore.kernel.org/oe-kbuild-all/202302131407.XrieHNuN-lkp@intel.com/
---
 fs/ext4/extents.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 0f95e857089e..bbf34679e10c 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -1756,6 +1756,8 @@ static int ext4_ext_correct_indexes(handle_t *handle, struct inode *inode,
 		if (err)
 			break;
 	}
+	while (!(k < 0) && k++ < depth)
+		clear_buffer_verified(path[k].p_bh);
 
 	return err;
 }
@@ -2304,6 +2306,7 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
 {
 	int err;
 	ext4_fsblk_t leaf;
+	int b_depth = depth;
 
 	/* free index block */
 	depth--;
@@ -2345,6 +2348,9 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
 		if (err)
 			break;
 	}
+	while (!(depth < 0) && depth++ < b_depth - 1)
+		clear_buffer_verified(path[depth].p_bh);
+
 	return err;
 }
 
-- 
2.31.1

Re: [PATCH v5 2/2] ext4: clear the verified flag of the modified leaf or idx if error

[Jan Kara]

On Mon 13-02-23 16:05:14, zhanchengbin wrote:
> Clear the verified flag from the modified bh when failed in ext4_ext_rm_idx
> or ext4_ext_correct_indexes.
> In this way, the start value of the logical block itself and its
> parents' will be checked in ext4_valid_extent_entries.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
> Link: https://lore.kernel.org/oe-kbuild-all/202302131414.5RKeHgAZ-lkp@intel.com/
> Link: https://lore.kernel.org/oe-kbuild-all/202302131407.XrieHNuN-lkp@intel.com/

Thanks for the patch! Two comments below:

> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 0f95e857089e..bbf34679e10c 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -1756,6 +1756,8 @@ static int ext4_ext_correct_indexes(handle_t *handle, struct inode *inode,
>  		if (err)
>  			break;
>  	}
> +	while (!(k < 0) && k++ < depth)
> +		clear_buffer_verified(path[k].p_bh);

This would be more understandable as:

	if (k >= 0)
		while (k++ < depth)
			...

Also the loop is IMO wrong because it will run with k == depth as well (due
to post-increment) and that is not initialized. Furthermore it will run
also if we exit the previous loop due to:

                /* change all left-side indexes */
                if (path[k+1].p_idx != EXT_FIRST_INDEX(path[k+1].p_hdr))
                        break;

which is unwanted as well. Which suggests that you didn't test your changes
much (if at all...). So please make sure your changes are tested next time.
Thank you!

								Honza

>  
>  	return err;
>  }
> @@ -2304,6 +2306,7 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
>  {
>  	int err;
>  	ext4_fsblk_t leaf;
> +	int b_depth = depth;
>  
>  	/* free index block */
>  	depth--;
> @@ -2345,6 +2348,9 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
>  		if (err)
>  			break;
>  	}
> +	while (!(depth < 0) && depth++ < b_depth - 1)
> +		clear_buffer_verified(path[depth].p_bh);
> +
>  	return err;
>  }
>  
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v5 2/2] ext4: clear the verified flag of the modified leaf or idx if error

[zhanchengbin]

The last patch did not take into account path[0].p_bh == NULL, so I
reworked the code.

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 0f95e857089e..05585afae0db 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -1750,13 +1750,19 @@ static int ext4_ext_correct_indexes(handle_t 
*handle, struct inode *inode,
                         break;
                 err = ext4_ext_get_access(handle, inode, path + k);
                 if (err)
-                       break;
+                       goto clean;
                 path[k].p_idx->ei_block = border;
                 err = ext4_ext_dirty(handle, inode, path + k);
                 if (err)
-                       break;
+                       goto clean;
         }
+       return 0;

+clean:
+       while (k++ < depth) {
+               /* k here will not be 0, so don't consider the case 
where path[0].p_bh is NULL */
+               clear_buffer_verified(path[k].p_bh);
+       }
         return err;
  }

@@ -2304,6 +2310,7 @@ static int ext4_ext_rm_idx(handle_t *handle, 
struct inode *inode,
  {
         int err;
         ext4_fsblk_t leaf;
+       int b_depth = depth;

         /* free index block */
         depth--;
@@ -2339,11 +2346,18 @@ static int ext4_ext_rm_idx(handle_t *handle, 
struct inode *inode,
                 path--;
                 err = ext4_ext_get_access(handle, inode, path);
                 if (err)
-                       break;
+                       goto clean;
                 path->p_idx->ei_block = (path+1)->p_idx->ei_block;
                 err = ext4_ext_dirty(handle, inode, path);
                 if (err)
-                       break;
+                       goto clean;
+       }
+       return 0;
+
+clean:
+       while (depth++ < b_depth - 1) {
+               /* depth here will not be 0, so don't consider the case 
where path[0].p_bh is NULL */
+               clear_buffer_verified(path[depth].p_bh);
         }
         return err;
  }


On 2023/2/14 20:52, Jan Kara wrote:
> 
> This would be more understandable as:
> 
> 	if (k >= 0)
> 		while (k++ < depth)
> 			...
> 
> Also the loop is IMO wrong because it will run with k == depth as well (due
> to post-increment) and that is not initialized. Furthermore it will run
> also if we exit the previous loop due to:
> 
>                  /* change all left-side indexes */
>                  if (path[k+1].p_idx != EXT_FIRST_INDEX(path[k+1].p_hdr))
>                          break;
> 
> which is unwanted as well. Which suggests that you didn't test your changes
> much (if at all...). So please make sure your changes are tested next time.
> Thank you!
> 
> 								Honza
I only ran xfstest locally. Do you have any better suggestions?

Thanks,
  - bin.
> 
>>   
>>   	return err;
>>   }
>> @@ -2304,6 +2306,7 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
>>   {
>>   	int err;
>>   	ext4_fsblk_t leaf;
>> +	int b_depth = depth;

Re: [PATCH v5 2/2] ext4: clear the verified flag of the modified leaf or idx if error

[Jan Kara]

On Thu 16-02-23 15:25:23, zhanchengbin wrote:
> The last patch did not take into account path[0].p_bh == NULL, so I
> reworked the code.
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 0f95e857089e..05585afae0db 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -1750,13 +1750,19 @@ static int ext4_ext_correct_indexes(handle_t
> *handle, struct inode *inode,
>                         break;
>                 err = ext4_ext_get_access(handle, inode, path + k);
>                 if (err)
> -                       break;
> +                       goto clean;
>                 path[k].p_idx->ei_block = border;
>                 err = ext4_ext_dirty(handle, inode, path + k);
>                 if (err)
> -                       break;
> +                       goto clean;
>         }
> +       return 0;
> 
> +clean:
> +       while (k++ < depth) {
> +               /* k here will not be 0, so don't consider the case where
> path[0].p_bh is NULL */

Please avoid the line over 80 characters.

> +               clear_buffer_verified(path[k].p_bh);
> +       }
>         return err;
>  }
> 
> @@ -2304,6 +2310,7 @@ static int ext4_ext_rm_idx(handle_t *handle, struct
> inode *inode,
>  {
>         int err;
>         ext4_fsblk_t leaf;
> +       int b_depth = depth;
> 
>         /* free index block */
>         depth--;
> @@ -2339,11 +2346,18 @@ static int ext4_ext_rm_idx(handle_t *handle, struct
> inode *inode,
>                 path--;
>                 err = ext4_ext_get_access(handle, inode, path);
>                 if (err)
> -                       break;
> +                       goto clean;
>                 path->p_idx->ei_block = (path+1)->p_idx->ei_block;
>                 err = ext4_ext_dirty(handle, inode, path);
>                 if (err)
> -                       break;
> +                       goto clean;
> +       }
> +       return 0;
> +
> +clean:
> +       while (depth++ < b_depth - 1) {
> +               /* depth here will not be 0, so don't consider the case
> where path[0].p_bh is NULL */

Again please avoid the overly long line.

> +               clear_buffer_verified(path[depth].p_bh);
>         }

I think this is still problematic because 'path' is being updated in the
above loop as well so this will still access beyond the end of the array.
So I think you first need to modify ext4_ext_rm_idx() to leave 'path' alone
and just index it like ext4_ext_correct_indexes() does it (separate patch
please) and then add this error recovery path.

> On 2023/2/14 20:52, Jan Kara wrote:
> > 
> > This would be more understandable as:
> > 
> > 	if (k >= 0)
> > 		while (k++ < depth)
> > 			...
> > 
> > Also the loop is IMO wrong because it will run with k == depth as well (due
> > to post-increment) and that is not initialized. Furthermore it will run
> > also if we exit the previous loop due to:
> > 
> >                  /* change all left-side indexes */
> >                  if (path[k+1].p_idx != EXT_FIRST_INDEX(path[k+1].p_hdr))
> >                          break;
> > 
> > which is unwanted as well. Which suggests that you didn't test your changes
> > much (if at all...). So please make sure your changes are tested next time.
> > Thank you!
> > 
> I only ran xfstest locally. Do you have any better suggestions?

Yes that's good. But that will not run your new error handling code at all,
will it? It would be good if you also ran the reproducer that presumably
triggered these fixes to exercise the new code...

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