>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 <[email protected]> >>>>>>>> --- >>>>>>>> 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/[email protected] > >BTW, please add fixes line in your patch. > >Thanks,
OK, I will correct this part and follow your suggestion. Thank you. _______________________________________________ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
