Chao Yu <c...@kernel.org> 于2024年11月7日周四 14:18写道： > > On 2024/11/6 16:41, Zhiguo Niu wrote: > > Chao Yu <c...@kernel.org> 于2024年11月6日周三 15:40写道： > >> > >> On 2024/11/6 14:08, Zhiguo Niu wrote: > >>> Chao Yu <c...@kernel.org> 于2024年11月6日周三 10:40写道： > >>>> > >>>> On 2024/11/6 10:26, Zhiguo Niu wrote: > >>>>> Chao Yu <c...@kernel.org> 于2024年11月6日周三 10:16写道： > >>>>>> > >>>>>> On 2024/11/5 19:02, Zhiguo Niu wrote: > >>>>>>> Chao Yu <c...@kernel.org> 于2024年11月5日周二 18:39写道： > >>>>>>>> > >>>>>>>> On 2024/11/5 15:28, Zhiguo Niu wrote: > >>>>>>>>> Chao Yu <c...@kernel.org> 于2024年11月5日周二 15:04写道： > >>>>>>>>>> > >>>>>>>>>> On 2024/11/4 9:56, Zhiguo Niu wrote: > >>>>>>>>>>> If user give a file size as "length" parameter for fiemap > >>>>>>>>>>> operations, but if this size is non-block size aligned, > >>>>>>>>>>> it will show 2 segments fiemap results even this whole file > >>>>>>>>>>> is contiguous on disk, such as the following results: > >>>>>>>>>>> > >>>>>>>>>>> ./f2fs_io fiemap 0 19034 ylog/analyzer.py > >>>>>>>>>>> Fiemap: offset = 0 len = 19034 > >>>>>>>>>>> logical addr. physical addr. length > >>>>>>>>>>> flags > >>>>>>>>>>> 0 0000000000000000 0000000020baa000 0000000000004000 > >>>>>>>>>>> 00001000 > >>>>>>>>>>> 1 0000000000004000 0000000020bae000 0000000000001000 > >>>>>>>>>>> 00001001 > >>>>>>>>>>> > >>>>>>>>>>> after this patch: > >>>>>>>>>>> ./f2fs_io fiemap 0 19034 ylog/analyzer.py > >>>>>>>>>>> Fiemap: offset = 0 len = 19034 > >>>>>>>>>>> logical addr. physical addr. length flags > >>>>>>>>>>> 0 0000000000000000 00000000315f3000 0000000000005000 00001001 > >>>>>>>>>>> > >>>>>>>>>>> Signed-off-by: Zhiguo Niu <zhiguo....@unisoc.com> > >>>>>>>>>>> --- > >>>>>>>>>>> V2: correct commit msg according to Chao's questions > >>>>>>>>>>> f2fs_io has been modified for testing, the length for fiemap is > >>>>>>>>>>> real file size, not block number > >>>>>>>>>>> --- > >>>>>>>>>>> fs/f2fs/data.c | 4 ++-- > >>>>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>>>>>>>>> > >>>>>>>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > >>>>>>>>>>> index 306b86b0..9fc229d 100644 > >>>>>>>>>>> --- a/fs/f2fs/data.c > >>>>>>>>>>> +++ b/fs/f2fs/data.c > >>>>>>>>>>> @@ -1966,8 +1966,8 @@ int f2fs_fiemap(struct inode *inode, struct > >>>>>>>>>>> fiemap_extent_info *fieinfo, > >>>>>>>>>>> goto out; > >>>>>>>>>>> } > >>>>>>>>>>> > >>>>>>>>>>> - if (bytes_to_blks(inode, len) == 0) > >>>>>>>>>>> - len = blks_to_bytes(inode, 1); > >>>>>>>>>>> + if (len & (blks_to_bytes(inode, 1) - 1)) > >>>>>>>>>>> + len = round_up(len, blks_to_bytes(inode, 1)); > >>>>>>>>>> > >>>>>>>>>> How do you think of getting rid of above alignment for len? > >>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> start_blk = bytes_to_blks(inode, start); > >>>>>>>>>>> last_blk = bytes_to_blks(inode, start + len - 1); > >>>>>>>>>> > >>>>>>>>>> And round up end position w/: > >>>>>>>>>> > >>>>>>>>>> last_blk = bytes_to_blks(inode, round_up(start + len - 1, > >>>>>>>>>> F2FS_BLKSIZE)); > >>>>>>>>> Hi Chao, > >>>>>>>>> I think this will change the current code logic > >>>>>>>>> ------------- > >>>>>>>>> if (start_blk > last_blk) > >>>>>>>>> goto out; > >>>>>>>>> ------------- > >>>>>>>>> for example, a file with size 19006, but the length from the user > >>>>>>>>> is 16384. > >>>>>>>>> before this modification, last_blk = bytes_to_blks(inode, start + > >>>>>>>>> len - 1) = (inode, 16383) = 3 > >>>>>>>>> after the first f2fs_map_blocks(). start_blk change to be 4, > >>>>>>>>> after the second f2fs_map_blocks(), fiemap_fill_nex_exten will be > >>>>>>>>> called to fill user parameter and then > >>>>>>>>> will goto out because start_blk > last_blk, then fiemap flow > >>>>>>>>> finishes. > >>>>>>>>> but after this modification, last_blk will be 4 > >>>>>>>>> will do f2fs_map_blocks() until reach the max_file_blocks(inode) > >>>>>>>> > >>>>>>>> Yes, you're right, however, w/ this patch, it may change last_blk, > >>>>>>>> e.g. > >>>>>>>> > >>>>>>>> xfs_io file -c "fiemap -v 0 19006" vs xfs_io file -c "fiemap -v 2 > >>>>>>>> 19006" > >>>>>>>> start_blk and last_blk will be: 0, 4 and 0, 5. > >>>>>>> Hi Chao, > >>>>>>> yes, but w/o this patch , the original code still has the same > >>>>>>> situation?? > >>>>>>> for example > >>>>>>> xfs_io file -c "fiemap -v 0 16384" vs xfs_io file -c "fiemap -v 2 > >>>>>>> 16384" > >>>>>>> start_blk and last_blk will be: 0, 3 and 0, 4. > >>>>>> > >>>>>> For the case "fiemap -v 2 19006", offset is 2, and length is 19006, so > >>>>>> last_offset > >>>>>> is 19008, and last_blk should be 4 rather than 5, right? > >>>>> hi Chao, > >>>>> it is right w/o my patch. > >>>>>> > >>>>>> And for you case, it calculates last_blk correctly. > >>>>> So you suggest that "Should we round_up len after start_blk & last_blk > >>>>> calculation?" > >>>> > >>>> Zhiguo, > >>>> > >>>> Yes, I think alignment of len should not affect calculation of last_blk. > >>>> > >>>> I mean this, > >>>> > >>>> --- > >>>> fs/f2fs/data.c | 6 +++--- > >>>> include/linux/f2fs_fs.h | 3 ++- > >>>> 2 files changed, 5 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > >>>> index 7d1bb9518a40..cbbb956f420d 100644 > >>>> --- a/fs/f2fs/data.c > >>>> +++ b/fs/f2fs/data.c > >>>> @@ -1967,12 +1967,12 @@ int f2fs_fiemap(struct inode *inode, struct > >>>> fiemap_extent_info *fieinfo, > >>>> goto out; > >>>> } > >>>> > >>>> - if (bytes_to_blks(inode, len) == 0) > >>>> - len = blks_to_bytes(inode, 1); > >>>> - > >>>> start_blk = bytes_to_blks(inode, start); > >>>> last_blk = bytes_to_blks(inode, start + len - 1); > >>>> > >>>> + if (len & F2FS_BLKSIZE_MASK) > >>>> + len = round_up(len, F2FS_BLKSIZE); > >>>> + > >>> Hi Chao, > >>> this verion verify pass with my test case. > >>> > >>> but there is still another issue in orginal code: > >>> ylog/analyzer.py size = 19034 > >>> if I input the following cmd(start/length are both real size, not block > >>> number) > >>> /f2fs_io fiemap 2 16384 ylog/analyzer.py > >>> and the results shows: > >>> Fiemap: offset = 2 len = 16384 > >>> logical addr. physical addr. length flags > >>> 0 0000000000000000 0000000e2ebca000 0000000000004000 00001000 > >>> 1 0000000000004000 0000000e2ebce000 0000000000001000 00001001 > >>> so start_blk/last_blk should be calculate it in the following way? > >> > >> IIUC, the root cause is f2fs_map_blocks() will truncate size of > >> returned extent to F2FS_BYTES_TO_BLK(len), so whenever parameter > >> @len doesn't cover last extent, it triggers this bug. > >> > >> next: > >> memset(&map, 0, sizeof(map)); > >> map.m_lblk = start_blk; > >> map.m_len = F2FS_BYTES_TO_BLK(len); --- limit max size of extent > >> it founds > > yes, I think so too. > >> map.m_next_pgofs = &next_pgofs; > >> map.m_seg_type = NO_CHECK_TYPE; > >> ... > >> ret = f2fs_map_blocks(inode, &map, F2FS_GET_BLOCK_FIEMAP); > >> > >> xfs_io file -c "fiemap -v 2 16384" > >> file: > >> EXT: FILE-OFFSET BLOCK-RANGE TOTAL FLAGS > >> 0: [0..31]: 139272..139303 32 0x1000 > >> 1: [32..39]: 139304..139311 8 0x1001 > >> xfs_io file -c "fiemap -v 0 16384" > >> file: > >> EXT: FILE-OFFSET BLOCK-RANGE TOTAL FLAGS > >> 0: [0..31]: 139272..139303 32 0x1000 > >> xfs_io file -c "fiemap -v 0 16385" > >> file: > >> EXT: FILE-OFFSET BLOCK-RANGE TOTAL FLAGS > >> 0: [0..39]: 139272..139311 40 0x1001 > > > > But If the correct last_blk is calculated correctly, fiemap can be > > ended as soon as possible? so the results shown is also right? > > Zhiguo, > > IMO, it's not, due to 1) if the extent is last one, FIEMAP_EXTENT_LAST > must be tagged to notice user that it doesn't need further fiemap on > latter LBA, 2) one continuous extent should not be split to two. > > Let me figure out a fix for that. Hi Chao, OK, thanks for your explaination. btw, Do I need to update a patch about the original issue we disscussed? or you will modify it together? thanks! > > Thanks, > > > such as this special case "xfs_io file -c "fiemap -v 2 16384" we discussed. > > but it is fine for me to keep the current codes. > > thanks! > >> > >> Thoughts? > >> > >> Thanks, > >> > >>> before: > >>> start_blk = bytes_to_blks(inode, start); > >>> last_blk = bytes_to_blks(inode, start + len - 1); > >>> after: > >>> > >>> start_blk = bytes_to_blks(inode, start); > >>> last_blk = start_blk + bytes_to_blks(inode, len - 1); > >>> thanks! > >>>> next: > >>>> memset(&map, 0, sizeof(map)); > >>>> map.m_lblk = start_blk; > >>>> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h > >>>> index b0b821edfd97..954e8e8344b7 100644 > >>>> --- a/include/linux/f2fs_fs.h > >>>> +++ b/include/linux/f2fs_fs.h > >>>> @@ -24,10 +24,11 @@ > >>>> #define NEW_ADDR ((block_t)-1) /* used as block_t > >>>> addresses */ > >>>> #define COMPRESS_ADDR ((block_t)-2) /* used as compressed > >>>> data flag */ > >>>> > >>>> +#define F2FS_BLKSIZE_MASK (F2FS_BLKSIZE - 1) > >>>> #define F2FS_BYTES_TO_BLK(bytes) ((bytes) >> F2FS_BLKSIZE_BITS) > >>>> #define F2FS_BLK_TO_BYTES(blk) ((blk) << > >>>> F2FS_BLKSIZE_BITS) > >>>> #define F2FS_BLK_END_BYTES(blk) (F2FS_BLK_TO_BYTES(blk > >>>> + 1) - 1) > >>>> -#define F2FS_BLK_ALIGN(x) (F2FS_BYTES_TO_BLK((x) + > >>>> F2FS_BLKSIZE - 1)) > >>>> +#define F2FS_BLK_ALIGN(x) (F2FS_BYTES_TO_BLK((x) + > >>>> F2FS_BLKSIZE - 1)) > >>>> > >>>> /* 0, 1(node nid), 2(meta nid) are reserved node id */ > >>>> #define F2FS_RESERVED_NODE_NUM 3 > >>>> -- > >>>> 2.40.1 > >>>> > >>>> > >>>> > >>>>> Thanks > >>>>>> > >>>>>> Thanks, > >>>>>> > >>>>>>> but overall last_blk will change loop counts but has not affect on > >>>>>>> the results. > >>>>>>>> > >>>>>>>> Should we round_up len after start_blk & last_blk calculation? > >>>>>>> I thinks it is ok ,but just a little bit redundant with the following > >>>>>>> handling about len. > >>>>>>> > >>>>>>> if (bytes_to_blks(inode, len) == 0) > >>>>>>> len = blks_to_bytes(inode, 1); > >>>>>>> > >>>>>>> Based on the above situation, > >>>>>>> do you have any other good suggestions? ^^ > >>>>>>> thanks! > >>>>>>> > >>>>>>>> > >>>>>>>> Thanks, > >>>>>>>> > >>>>>>>>> thanks！ > >>>>>>>>>> > >>>>>>>>>> Thanks, > >>>>>>>>>> > >>>>>>>> > >>>>>> > >>>> > >> >
_______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
