Message-ID: <6e6bb868-7107-3528-db6d-0ddc275f6326@huawei.com>
Date:   Thu, 16 Feb 2023 15:25:23 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.2
Subject: Re: [PATCH v5 2/2] ext4: clear the verified flag of the modified leaf
 or idx if error
To:     Jan Kara <jack@suse.cz>
CC:     <tytso@mit.edu>, <jack@suse.com>, <linux-ext4@vger.kernel.org>,
        <yi.zhang@huawei.com>, <linfeilong@huawei.com>,
        <liuzhiqiang26@huawei.com>, kernel test robot <lkp@intel.com>
References: <20230213080514.535568-1-zhanchengbin1@huawei.com>
 <20230213080514.535568-3-zhanchengbin1@huawei.com>
 <20230214125211.o2j3vpkopvas2niq@quack3>
From:   zhanchengbin <zhanchengbin1@huawei.com>
In-Reply-To: <20230214125211.o2j3vpkopvas2niq@quack3>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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;