Re: [f2fs-dev] [PATCH 1/2] f2fs: fix wrong extent_info data for precache extents

[Chao Yu <chao@kernel.org>]

On 9/12/2025 6:06 PM, wangzijie wrote:
>> On 9/12/2025 11:36 AM, wangzijie wrote:
>>>> On 9/11/2025 5:07 PM, wangzijie wrote:
>>>>>> On 9/10/25 21:58, wangzijie wrote:
>>>>>>> When the data layout is like this:
>>>>>>> dnode1:                     dnode2:
>>>>>>> [0]      A                  [0]    NEW_ADDR
>>>>>>> [1]      A+1                [1]    0x0
>>>>>>> ...                         ....
>>>>>>> [1016]   A+1016
>>>>>>> [1017]   B (B!=A+1017)      [1017] 0x0
>>>>>>>
>>>>>>> We can build this kind of layout by following steps(with i_extra_isize:36):
>>>>>>> ./f2fs_io write 1 0 1881 rand dsync testfile
>>>>>>> ./f2fs_io write 1 1881 1 rand buffered testfile
>>>>>>> ./f2fs_io fallocate 0 7708672 4096 testfile
>>>>>>>
>>>>>>> And when we map first data block in dnode2, we will get wrong extent_info data:
>>>>>>> map->m_len = 1
>>>>>>> ofs = start_pgofs - map->m_lblk = 1882 - 1881 = 1
>>>>>>>
>>>>>>> ei.fofs = start_pgofs = 1882
>>>>>>> ei.len = map->m_len - ofs = 1 - 1 = 0
>>>>>>>
>>>>>>> Fix it by skipping updating this kind of extent info.
>>>>>>>
>>>>>>> Signed-off-by: wangzijie <wangzijie1@honor.com>
>>>>>>> ---
>>>>>>>     fs/f2fs/data.c | 3 +++
>>>>>>>     1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>>>> index 7961e0ddf..b8bb71852 100644
>>>>>>> --- a/fs/f2fs/data.c
>>>>>>> +++ b/fs/f2fs/data.c
>>>>>>> @@ -1649,6 +1649,9 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
>>>>>>>     
>>>>>>>     		switch (flag) {
>>>>>>>     		case F2FS_GET_BLOCK_PRECACHE:
>>>>>>> +			if (__is_valid_data_blkaddr(map->m_pblk) &&
>>>>>>> +				start_pgofs - map->m_lblk == map->m_len)
>>>>>>> +				map->m_flags &= ~F2FS_MAP_MAPPED;
>>>>>>
>>>>>> It looks we missed to reset value for map variable in f2fs_precache_extents(),
>>>>>> what do you think of this?
>>>>>>
>>>>>> ---
>>>>>>     fs/f2fs/file.c | 4 +++-
>>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>> index 1aae4361d0a8..2b14151d4130 100644
>>>>>> --- a/fs/f2fs/file.c
>>>>>> +++ b/fs/f2fs/file.c
>>>>>> @@ -3599,7 +3599,7 @@ static int f2fs_ioc_io_prio(struct file *filp, unsigned long arg)
>>>>>>     int f2fs_precache_extents(struct inode *inode)
>>>>>>     {
>>>>>>     	struct f2fs_inode_info *fi = F2FS_I(inode);
>>>>>> -	struct f2fs_map_blocks map;
>>>>>> +	struct f2fs_map_blocks map = { 0 };
>>>>>>     	pgoff_t m_next_extent;
>>>>>>     	loff_t end;
>>>>>>     	int err;
>>>>>> @@ -3617,6 +3617,8 @@ int f2fs_precache_extents(struct inode *inode)
>>>>>>
>>>>>>     	while (map.m_lblk < end) {
>>>>>>     		map.m_len = end - map.m_lblk;
>>>>>> +		map.m_pblk = 0;
>>>>>> +		map.m_flags = 0;
>>>>>>
>>>>>>     		f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
>>>>>>     		err = f2fs_map_blocks(inode, &map, F2FS_GET_BLOCK_PRECACHE);
>>>>>> -- 
>>>>>> 2.49.0
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>>     			goto sync_out;
>>>>>>>     		case F2FS_GET_BLOCK_BMAP:
>>>>>>>     			map->m_pblk = 0;
>>>>>
>>>>>
>>>>> We have already reset m_flags (map->m_flags = 0) in f2fs_map_blocks().
>>>>
>>>> Zijie:
>>>>
>>>> Oops, that's right, thanks for correcting me.
>>>>
>>>>>
>>>>> I think that this bug is caused by we missed to reset m_flags when we
>>>>> goto next_dnode in below case：
>>>>>
>>>>> Data layout is something like this:
>>>>> dnode1:                     dnode2:
>>>>> [0]      A                  [0]    NEW_ADDR
>>>>> [1]      A+1                [1]    0x0
>>>>> ...
>>>>> [1016]   A+1016
>>>>> [1017]   B (B!=A+1017)      [1017] 0x0
>>>>>
>>>>> we map the last block(valid blkaddr) in dnode1:
>>>>> map->m_flags |= F2FS_MAP_MAPPED;
>>>>> map->m_pblk = blkaddr(valid blkaddr);
>>>>> map->m_len = 1;
>>>>> then we goto next_dnode, meet the first block in dnode2(hole), goto sync_out:
>>>>> map->m_flags & F2FS_MAP_MAPPED == true, and we make wrong blkaddr/len for extent_info.
>>>>
>>>> So, can you please add above explanation into commit message? that
>>>> should be helpful for understanding the problem more clearly.
>>>>
>>>> Please take a look at this case w/ your patch:
>>>>
>>>> mkfs.f2fs -O extra_attr,compression /dev/vdb -f
>>>> mount /dev/vdb /mnt/f2fs -o mode=lfs
>>>> cd /mnt/f2fs
>>>> f2fs_io write 1 0 1883 rand dsync testfile
>>>> f2fs_io fallocate 0 7712768 4096 testfile
>>>> f2fs_io write 1 1881 1 rand buffered testfile
>>>> xfs_io testfile -c "fsync"
>>>> cd /
>>>> umount /mnt/f2fs
>>>> mount /dev/vdb /mnt/f2fs
>>>> f2fs_io precache_extents /mnt/f2fs/testfile
>>>> umount /mnt/f2fs
>>>>
>>>>            f2fs_io-733     [010] .....    78.134136: f2fs_update_read_extent_tree_range: dev = (253,16), ino = 4, pgofs = 1882, len = 0, blkaddr = 17410, c_len = 0
>>>>
>>>> I suspect we need this?
>>>>
>>>> @@ -1784,7 +1781,8 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
>>>>           }
>>>>
>>>>           if (flag == F2FS_GET_BLOCK_PRECACHE) {
>>>> -               if (map->m_flags & F2FS_MAP_MAPPED) {
>>>> +               if ((map->m_flags & F2FS_MAP_MAPPED) &&
>>>> +                       (map->m_len - ofs)) {
>>>>                           unsigned int ofs = start_pgofs - map->m_lblk;
>>>>
>>>>                           f2fs_update_read_extent_cache_range(&dn,
>>>
>>> Thanks for pointing out this. Let me find a way to cover these cases and do more test.
>>>
>>>> BTW, I find another bug, if one blkaddr is adjcent to previous extent,
>>>> but and it is valid, we need to set m_next_extent to pgofs rather than
>>>> pgofs + 1.
>>>>
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index cbf8841642c7..ac88ed68059c 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -1789,8 +1789,11 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
>>>>                                   start_pgofs, map->m_pblk + ofs,
>>>>                                   map->m_len - ofs);
>>>>                   }
>>>> -               if (map->m_next_extent)
>>>> -                       *map->m_next_extent = pgofs + 1;
>>>> +               if (map->m_next_extent) {
>>>> +                       *map->m_next_extent = pgofs;
>>>> +                       if (!__is_valid_data_blkaddr(blkaddr))
>>>> +                               *map->m_next_extent += 1;
>>>> +               }
>>>>           }
>>>>           f2fs_put_dnode(&dn);
>>>
>>> Maybe it can be this?
>>> if (map->m_next_extent)
>>> 	*map->m_next_extent = is_hole ? pgofs + 1 : pgofs;
>>
>> It's better, will update, thank you. :)
>>
>> Thanks,
> 
> Hi Chao,
> I test some cases with this change:
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 7961e0ddf..7093fdc95 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1777,13 +1777,13 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
>          if (flag == F2FS_GET_BLOCK_PRECACHE) {
>                  if (map->m_flags & F2FS_MAP_MAPPED) {
>                          unsigned int ofs = start_pgofs - map->m_lblk;
> -
> -                       f2fs_update_read_extent_cache_range(&dn,
> -                               start_pgofs, map->m_pblk + ofs,
> -                               map->m_len - ofs);
> +                       if (map->m_len - ofs > 0)
> +                               f2fs_update_read_extent_cache_range(&dn,
> +                                       start_pgofs, map->m_pblk + ofs,
> +                                       map->m_len - ofs);
>                  }
>                  if (map->m_next_extent)
> -                       *map->m_next_extent = pgofs + 1;
> +                       *map->m_next_extent = is_hole ? pgofs + 1 : pgofs;
>          }
>          f2fs_put_dnode(&dn);
>   unlock_out:
> 
> 
> test cases:
> 
> case1:
> dnode1:                     dnode2:
> [0]      A                  [0]    NEW_ADDR
> [1]      A+1                [1]    0x0
> ...                         ....
> [1016]   A+1016
> [1017]   B (B!=A+1017)      [1017] 0x0
> 
> case2:
> dnode1:                     dnode2:
> [0]      A                  [0]    C (C!=B+1)
> [1]      A+1                [1]    C+1
> ...                         ....
> [1016]   A+1016
> [1017]   B (B!=A+1017)      [1017] 0x0
> 
> case3:
> dnode1:                     dnode2:
> [0]      A                  [0]    C (C!=B+2)
> [1]      A+1                [1]    C+1
> ...                         ....
> [1015]   A+1015
> [1016]   B (B!=A+1016)
> [1017]   B+1                [1017] 0x0
> 
> case4:
> one blkaddr is adjcent to previous extent, and it is valid.
 > > And from the result, it seems this change can cover these
> situations correctly.
> Do we need a patch with this change?

Zijie, thanks for the test.

IMO, we'd better use these changes:

-
-                       f2fs_update_read_extent_cache_range(&dn,
-                               start_pgofs, map->m_pblk + ofs,
-                               map->m_len - ofs);
+                       if (map->m_len - ofs > 0)
+                               f2fs_update_read_extent_cache_range(&dn,
+                                       start_pgofs, map->m_pblk + ofs,
+                                       map->m_len - ofs);

instead of

    		switch (flag) {
    		case F2FS_GET_BLOCK_PRECACHE:
+			if (__is_valid_data_blkaddr(map->m_pblk) &&
+				start_pgofs - map->m_lblk == map->m_len)
+				map->m_flags &= ~F2FS_MAP_MAPPED;

Can you please rebase your patchset on mine and send v2?

https://lore.kernel.org/linux-f2fs-devel/20250912081250.44383-1-chao@kernel.org

BTW, please add fixes line in your patch.

Thanks,