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

[zhanchengbin <zhanchengbin1@xxxxxxxxxx>]

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;