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

[wangzijie <wangzijie1@honor.com>]

>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?


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel