[PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Zhiguo Niu]

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.niu@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));
 
 	start_blk = bytes_to_blks(inode, start);
 	last_blk = bytes_to_blks(inode, start + len - 1);
-- 
1.9.1

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Chao Yu]

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

Hi Zhiguo,

Any testcase to reproduce this bug? w/o this patch, it looks output
from fiemap looks fine?

f2fs_io fiemap 0 19034 file
Fiemap: offset = 0 len = 19034
	logical addr.    physical addr.   length           flags
0	0000000000000000 0000000004401000 0000000000005000 00001001

Thanks,

> 
> Signed-off-by: Zhiguo Niu <zhiguo.niu@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));
>   
>   	start_blk = bytes_to_blks(inode, start);
>   	last_blk = bytes_to_blks(inode, start + len - 1);

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Zhiguo Niu]

Chao Yu <chao@kernel.org> 于2024年11月5日周二 11:15写道：
>
> 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
>
> Hi Zhiguo,
>
> Any testcase to reproduce this bug? w/o this patch, it looks output
> from fiemap looks fine?
>
> f2fs_io fiemap 0 19034 file
> Fiemap: offset = 0 len = 19034
>         logical addr.    physical addr.   length           flags
> 0       0000000000000000 0000000004401000 0000000000005000 00001001
>
Hi Chao,
Sorry I didn't write clearly enough about the test case, and i put the
note below the ""Singed-off" tag.
let me describe it again， f2fs_io fiemap has been modified by me for
testing in my local, and  the length parameter
is the real file size of the file, not the block numer. because user
also pass the real file size to fiemap ioctl.
so with the new f2fs_io fiemap, a contiguous file on disk may be shown
2 segments if the length is not block size alinged.
such as:

ums9632_1h10:/data # ls -l ylog/ap/analyzer.py
-rw-rw-rw- 1 root system 19006 2008-01-01 00:00 ylog/ap/analyzer.py
ums9632_1h10:/data # ./f2fs_io fiemap 0 19006 ylog/ap/analyzer.py
Fiemap: offset = 0 len = 19006
        logical addr.    physical addr.   length           flags
0       0000000000000000 0000000020baa000 0000000000004000 00001000
1       0000000000004000 0000000020bae000 0000000000001000 00001001

but if we pass a length that is block size alinged, it will show one
whole segment in fiemap log.
ums9632_1h10:/data # ./f2fs_io fiemap 0 20480 ylog/ap/analyzer.py
Fiemap: offset = 0 len = 20480
        logical addr.    physical addr.   length           flags
0       0000000000000000 0000000020baa000 0000000000005000 00001001
Thanks.
> Thanks,
>
> >
> > Signed-off-by: Zhiguo Niu <zhiguo.niu@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));
> >
> >       start_blk = bytes_to_blks(inode, start);
> >       last_blk = bytes_to_blks(inode, start + len - 1);
>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Chao Yu]

On 2024/11/5 12:02, Zhiguo Niu wrote:
> Chao Yu <chao@kernel.org> 于2024年11月5日周二 11:15写道：
>>
>> 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
>>
>> Hi Zhiguo,
>>
>> Any testcase to reproduce this bug? w/o this patch, it looks output
>> from fiemap looks fine?
>>
>> f2fs_io fiemap 0 19034 file
>> Fiemap: offset = 0 len = 19034
>>          logical addr.    physical addr.   length           flags
>> 0       0000000000000000 0000000004401000 0000000000005000 00001001
>>
> Hi Chao,
> Sorry I didn't write clearly enough about the test case, and i put the
> note below the ""Singed-off" tag.
> let me describe it again， f2fs_io fiemap has been modified by me for
> testing in my local, and  the length parameter
> is the real file size of the file, not the block numer. because user
> also pass the real file size to fiemap ioctl.
> so with the new f2fs_io fiemap, a contiguous file on disk may be shown
> 2 segments if the length is not block size alinged.
> such as:
> 
> ums9632_1h10:/data # ls -l ylog/ap/analyzer.py
> -rw-rw-rw- 1 root system 19006 2008-01-01 00:00 ylog/ap/analyzer.py
> ums9632_1h10:/data # ./f2fs_io fiemap 0 19006 ylog/ap/analyzer.py
> Fiemap: offset = 0 len = 19006
>          logical addr.    physical addr.   length           flags
> 0       0000000000000000 0000000020baa000 0000000000004000 00001000
> 1       0000000000004000 0000000020bae000 0000000000001000 00001001
> 
> but if we pass a length that is block size alinged, it will show one
> whole segment in fiemap log.
> ums9632_1h10:/data # ./f2fs_io fiemap 0 20480 ylog/ap/analyzer.py
> Fiemap: offset = 0 len = 20480
>          logical addr.    physical addr.   length           flags
> 0       0000000000000000 0000000020baa000 0000000000005000 00001001
> Thanks.

Oh, I can reproduce it w/ xfs_io, thanks for your explanation.

xfs_io file -c "fiemap -v 0 19006"
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 20480"
file:
  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
    0: [0..39]:         139272..139311      40 0x1001

Thanks,

>> Thanks,
>>
>>>
>>> Signed-off-by: Zhiguo Niu <zhiguo.niu@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));
>>>
>>>        start_blk = bytes_to_blks(inode, start);
>>>        last_blk = bytes_to_blks(inode, start + len - 1);
>>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Chao Yu]

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.niu@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));

Thanks,

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Zhiguo Niu]

Chao Yu <chao@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.niu@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)
thanks！
>
> Thanks,
>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Chao Yu]

On 2024/11/5 15:28, Zhiguo Niu wrote:
> Chao Yu <chao@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.niu@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.

Should we round_up len after start_blk & last_blk calculation?

Thanks,

> thanks！
>>
>> Thanks,
>>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Zhiguo Niu]

Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
>
> On 2024/11/5 15:28, Zhiguo Niu wrote:
> > Chao Yu <chao@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.niu@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.
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,
> >>
>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Chao Yu]

On 2024/11/5 19:02, Zhiguo Niu wrote:
> Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
>>
>> On 2024/11/5 15:28, Zhiguo Niu wrote:
>>> Chao Yu <chao@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.niu@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?

And for you case, it calculates last_blk correctly.

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

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Zhiguo Niu]

Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:16写道：
>
> On 2024/11/5 19:02, Zhiguo Niu wrote:
> > Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
> >>
> >> On 2024/11/5 15:28, Zhiguo Niu wrote:
> >>> Chao Yu <chao@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.niu@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?"
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,
> >>>>
> >>
>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Chao Yu]

On 2024/11/6 10:26, Zhiguo Niu wrote:
> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:16写道：
>>
>> On 2024/11/5 19:02, Zhiguo Niu wrote:
>>> Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
>>>>
>>>> On 2024/11/5 15:28, Zhiguo Niu wrote:
>>>>> Chao Yu <chao@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.niu@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);
+
  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,
>>>>>>
>>>>
>>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Zhiguo Niu]

Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:40写道：
>
> On 2024/11/6 10:26, Zhiguo Niu wrote:
> > Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:16写道：
> >>
> >> On 2024/11/5 19:02, Zhiguo Niu wrote:
> >>> Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
> >>>>
> >>>> On 2024/11/5 15:28, Zhiguo Niu wrote:
> >>>>> Chao Yu <chao@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.niu@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,
It looks well and clear, Let me verify this.
another unimportant questions, do we need to use macor
F2FS_BLKSIZE_xxx for round_up?
because in fiemap, it all use bytes_to_blks(inode, xxx) /
blks_to_bytes(inode, xxx)
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,
> >>>>>>
> >>>>
> >>
>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Chao Yu]

On 2024/11/6 10:54, Zhiguo Niu wrote:
> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:40写道：
>>
>> On 2024/11/6 10:26, Zhiguo Niu wrote:
>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:16写道：
>>>>
>>>> On 2024/11/5 19:02, Zhiguo Niu wrote:
>>>>> Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
>>>>>>
>>>>>> On 2024/11/5 15:28, Zhiguo Niu wrote:
>>>>>>> Chao Yu <chao@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.niu@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,
> It looks well and clear, Let me verify this.
> another unimportant questions, do we need to use macor
> F2FS_BLKSIZE_xxx for round_up?

Zhiguo,

Well, f2fs doesn't support different blksize in one instance, so
bytes_to_blks() is equal to F2FS_BYTES_TO_BLK(), I guess we can
use F2FS_BYTES_TO_BLK() instead of bytes_to_blks() for cleanup,
let me figure out a patch for that.

Thanks,

> because in fiemap, it all use bytes_to_blks(inode, xxx) /
> blks_to_bytes(inode, xxx)
> 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,
>>>>>>>>
>>>>>>
>>>>
>>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Zhiguo Niu]

Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:40写道：
>
> On 2024/11/6 10:26, Zhiguo Niu wrote:
> > Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:16写道：
> >>
> >> On 2024/11/5 19:02, Zhiguo Niu wrote:
> >>> Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
> >>>>
> >>>> On 2024/11/5 15:28, Zhiguo Niu wrote:
> >>>>> Chao Yu <chao@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.niu@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?
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,
> >>>>>>
> >>>>
> >>
>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Chao Yu]

On 2024/11/6 14:08, Zhiguo Niu wrote:
> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:40写道：
>>
>> On 2024/11/6 10:26, Zhiguo Niu wrote:
>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:16写道：
>>>>
>>>> On 2024/11/5 19:02, Zhiguo Niu wrote:
>>>>> Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
>>>>>>
>>>>>> On 2024/11/5 15:28, Zhiguo Niu wrote:
>>>>>>> Chao Yu <chao@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.niu@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
	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

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

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Zhiguo Niu]

Chao Yu <chao@kernel.org> 于2024年11月6日周三 15:40写道：
>
> On 2024/11/6 14:08, Zhiguo Niu wrote:
> > Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:40写道：
> >>
> >> On 2024/11/6 10:26, Zhiguo Niu wrote:
> >>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:16写道：
> >>>>
> >>>> On 2024/11/5 19:02, Zhiguo Niu wrote:
> >>>>> Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
> >>>>>>
> >>>>>> On 2024/11/5 15:28, Zhiguo Niu wrote:
> >>>>>>> Chao Yu <chao@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.niu@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?
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,
> >>>>>>>>
> >>>>>>
> >>>>
> >>
>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Chao Yu]

On 2024/11/6 16:41, Zhiguo Niu wrote:
> Chao Yu <chao@kernel.org> 于2024年11月6日周三 15:40写道：
>>
>> On 2024/11/6 14:08, Zhiguo Niu wrote:
>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:40写道：
>>>>
>>>> On 2024/11/6 10:26, Zhiguo Niu wrote:
>>>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:16写道：
>>>>>>
>>>>>> On 2024/11/5 19:02, Zhiguo Niu wrote:
>>>>>>> Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
>>>>>>>>
>>>>>>>> On 2024/11/5 15:28, Zhiguo Niu wrote:
>>>>>>>>> Chao Yu <chao@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.niu@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.

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

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Zhiguo Niu]

Chao Yu <chao@kernel.org> 于2024年11月7日周四 14:18写道：
>
> On 2024/11/6 16:41, Zhiguo Niu wrote:
> > Chao Yu <chao@kernel.org> 于2024年11月6日周三 15:40写道：
> >>
> >> On 2024/11/6 14:08, Zhiguo Niu wrote:
> >>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:40写道：
> >>>>
> >>>> On 2024/11/6 10:26, Zhiguo Niu wrote:
> >>>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:16写道：
> >>>>>>
> >>>>>> On 2024/11/5 19:02, Zhiguo Niu wrote:
> >>>>>>> Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
> >>>>>>>>
> >>>>>>>> On 2024/11/5 15:28, Zhiguo Niu wrote:
> >>>>>>>>> Chao Yu <chao@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.niu@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,
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>
> >>
>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Chao Yu]

On 2024/11/7 14:54, Zhiguo Niu wrote:
> Chao Yu <chao@kernel.org> 于2024年11月7日周四 14:18写道：
>>
>> On 2024/11/6 16:41, Zhiguo Niu wrote:
>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 15:40写道：
>>>>
>>>> On 2024/11/6 14:08, Zhiguo Niu wrote:
>>>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:40写道：
>>>>>>
>>>>>> On 2024/11/6 10:26, Zhiguo Niu wrote:
>>>>>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:16写道：
>>>>>>>>
>>>>>>>> On 2024/11/5 19:02, Zhiguo Niu wrote:
>>>>>>>>> Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
>>>>>>>>>>
>>>>>>>>>> On 2024/11/5 15:28, Zhiguo Niu wrote:
>>>>>>>>>>> Chao Yu <chao@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.niu@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?

Zhiguo, let me send a patchset including your patch, now, I'm testing this:

 From c67cb4782a3f1875865f9ae24cce80a59752d600 Mon Sep 17 00:00:00 2001
From: Chao Yu <chao@kernel.org>
Date: Thu, 7 Nov 2024 14:52:17 +0800
Subject: [PATCH] f2fs: fix to requery extent which cross boundary of inquiry

dd if=/dev/zero of=file bs=4k count=5
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

There are two problems:
- continuous extent is split to two
- FIEMAP_EXTENT_LAST is missing in last extent

The root cause is: if upper boundary of inquiry crosses extent,
f2fs_map_blocks() will truncate length of returned extent to
F2FS_BYTES_TO_BLK(len), and also, it will stop to query latter
extent or hole to make sure current extent is last or not.

In order to fix this issue, once we found an extent locates
in the end of inquiry range by f2fs_map_blocks(), we need to
expand inquiry range to requiry.

Reported-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
Signed-off-by: Chao Yu <chao@kernel.org>
---
  fs/f2fs/data.c | 20 +++++++++++++++-----
  1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 69f1cb0490ee..ee5614324df0 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1896,7 +1896,7 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
  		u64 start, u64 len)
  {
  	struct f2fs_map_blocks map;
-	sector_t start_blk, last_blk;
+	sector_t start_blk, last_blk, blk_len, max_len;
  	pgoff_t next_pgofs;
  	u64 logical = 0, phys = 0, size = 0;
  	u32 flags = 0;
@@ -1940,14 +1940,13 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,

  	start_blk = F2FS_BYTES_TO_BLK(start);
  	last_blk = F2FS_BYTES_TO_BLK(start + len - 1);
-
-	if (len & F2FS_BLKSIZE_MASK)
-		len = round_up(len, F2FS_BLKSIZE);
+	blk_len = last_blk - start_blk + 1;
+	max_len = F2FS_BYTES_TO_BLK(maxbytes) - start_blk;

  next:
  	memset(&map, 0, sizeof(map));
  	map.m_lblk = start_blk;
-	map.m_len = F2FS_BYTES_TO_BLK(len);
+	map.m_len = blk_len;
  	map.m_next_pgofs = &next_pgofs;
  	map.m_seg_type = NO_CHECK_TYPE;

@@ -1970,6 +1969,17 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
  		flags |= FIEMAP_EXTENT_LAST;
  	}

+	/*
+	 * current extent may cross boundary of inquiry, increase len to
+	 * requery.
+	 */
+	if (!compr_cluster && (map.m_flags & F2FS_MAP_MAPPED) &&
+				map.m_lblk + map.m_len - 1 == last_blk &&
+				blk_len != max_len) {
+		blk_len = max_len;
+		goto next;
+	}
+
  	compr_appended = false;
  	/* In a case of compressed cluster, append this to the last extent */
  	if (compr_cluster && ((map.m_flags & F2FS_MAP_DELALLOC) ||
-- 
2.40.1

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

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Zhiguo Niu]

Chao Yu <chao@kernel.org> 于2024年11月7日周四 16:22写道：
>
> On 2024/11/7 14:54, Zhiguo Niu wrote:
> > Chao Yu <chao@kernel.org> 于2024年11月7日周四 14:18写道：
> >>
> >> On 2024/11/6 16:41, Zhiguo Niu wrote:
> >>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 15:40写道：
> >>>>
> >>>> On 2024/11/6 14:08, Zhiguo Niu wrote:
> >>>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:40写道：
> >>>>>>
> >>>>>> On 2024/11/6 10:26, Zhiguo Niu wrote:
> >>>>>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:16写道：
> >>>>>>>>
> >>>>>>>> On 2024/11/5 19:02, Zhiguo Niu wrote:
> >>>>>>>>> Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
> >>>>>>>>>>
> >>>>>>>>>> On 2024/11/5 15:28, Zhiguo Niu wrote:
> >>>>>>>>>>> Chao Yu <chao@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.niu@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?
>
> Zhiguo, let me send a patchset including your patch, now, I'm testing this:
Hi Chao,
It's ok ^^
>
>  From c67cb4782a3f1875865f9ae24cce80a59752d600 Mon Sep 17 00:00:00 2001
> From: Chao Yu <chao@kernel.org>
> Date: Thu, 7 Nov 2024 14:52:17 +0800
> Subject: [PATCH] f2fs: fix to requery extent which cross boundary of inquiry
>
> dd if=/dev/zero of=file bs=4k count=5
> 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
>
> There are two problems:
> - continuous extent is split to two
> - FIEMAP_EXTENT_LAST is missing in last extent
>
> The root cause is: if upper boundary of inquiry crosses extent,
> f2fs_map_blocks() will truncate length of returned extent to
> F2FS_BYTES_TO_BLK(len), and also, it will stop to query latter
> extent or hole to make sure current extent is last or not.
>
> In order to fix this issue, once we found an extent locates
> in the end of inquiry range by f2fs_map_blocks(), we need to
> expand inquiry range to requiry.
>
> Reported-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>   fs/f2fs/data.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 69f1cb0490ee..ee5614324df0 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1896,7 +1896,7 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>                 u64 start, u64 len)
>   {
>         struct f2fs_map_blocks map;
> -       sector_t start_blk, last_blk;
> +       sector_t start_blk, last_blk, blk_len, max_len;
>         pgoff_t next_pgofs;
>         u64 logical = 0, phys = 0, size = 0;
>         u32 flags = 0;
> @@ -1940,14 +1940,13 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>
>         start_blk = F2FS_BYTES_TO_BLK(start);
>         last_blk = F2FS_BYTES_TO_BLK(start + len - 1);
> -
> -       if (len & F2FS_BLKSIZE_MASK)
> -               len = round_up(len, F2FS_BLKSIZE);
> +       blk_len = last_blk - start_blk + 1;
> +       max_len = F2FS_BYTES_TO_BLK(maxbytes) - start_blk;
>
>   next:
>         memset(&map, 0, sizeof(map));
>         map.m_lblk = start_blk;
> -       map.m_len = F2FS_BYTES_TO_BLK(len);
> +       map.m_len = blk_len;
>         map.m_next_pgofs = &next_pgofs;
>         map.m_seg_type = NO_CHECK_TYPE;
>
> @@ -1970,6 +1969,17 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>                 flags |= FIEMAP_EXTENT_LAST;
>         }
>
> +       /*
> +        * current extent may cross boundary of inquiry, increase len to
> +        * requery.
> +        */
> +       if (!compr_cluster && (map.m_flags & F2FS_MAP_MAPPED) &&
> +                               map.m_lblk + map.m_len - 1 == last_blk &&
> +                               blk_len != max_len) {
> +               blk_len = max_len;
> +               goto next;
> +       }
> +
it seems if user input the lenght which is less than the file size,
but return the whole fiemap?
such as:
 dd if=/dev/zero of=file bs=4k count=5
 xfs_io file -c "fiemap -v 0 16384"
will get extent with lenght = 0x5000? Is this expected?
Or did I understand it wrong?
thanks!
>         compr_appended = false;
>         /* In a case of compressed cluster, append this to the last extent */
>         if (compr_cluster && ((map.m_flags & F2FS_MAP_DELALLOC) ||
> --
> 2.40.1
>
> > 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,
> >>>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>
> >>
>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Chao Yu]

On 2024/11/7 18:53, Zhiguo Niu wrote:
> Chao Yu <chao@kernel.org> 于2024年11月7日周四 16:22写道：
>>
>> On 2024/11/7 14:54, Zhiguo Niu wrote:
>>> Chao Yu <chao@kernel.org> 于2024年11月7日周四 14:18写道：
>>>>
>>>> On 2024/11/6 16:41, Zhiguo Niu wrote:
>>>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 15:40写道：
>>>>>>
>>>>>> On 2024/11/6 14:08, Zhiguo Niu wrote:
>>>>>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:40写道：
>>>>>>>>
>>>>>>>> On 2024/11/6 10:26, Zhiguo Niu wrote:
>>>>>>>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:16写道：
>>>>>>>>>>
>>>>>>>>>> On 2024/11/5 19:02, Zhiguo Niu wrote:
>>>>>>>>>>> Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
>>>>>>>>>>>>
>>>>>>>>>>>> On 2024/11/5 15:28, Zhiguo Niu wrote:
>>>>>>>>>>>>> Chao Yu <chao@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.niu@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?
>>
>> Zhiguo, let me send a patchset including your patch, now, I'm testing this:
> Hi Chao,
> It's ok ^^
>>
>>   From c67cb4782a3f1875865f9ae24cce80a59752d600 Mon Sep 17 00:00:00 2001
>> From: Chao Yu <chao@kernel.org>
>> Date: Thu, 7 Nov 2024 14:52:17 +0800
>> Subject: [PATCH] f2fs: fix to requery extent which cross boundary of inquiry
>>
>> dd if=/dev/zero of=file bs=4k count=5
>> 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
>>
>> There are two problems:
>> - continuous extent is split to two
>> - FIEMAP_EXTENT_LAST is missing in last extent
>>
>> The root cause is: if upper boundary of inquiry crosses extent,
>> f2fs_map_blocks() will truncate length of returned extent to
>> F2FS_BYTES_TO_BLK(len), and also, it will stop to query latter
>> extent or hole to make sure current extent is last or not.
>>
>> In order to fix this issue, once we found an extent locates
>> in the end of inquiry range by f2fs_map_blocks(), we need to
>> expand inquiry range to requiry.
>>
>> Reported-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
>> Signed-off-by: Chao Yu <chao@kernel.org>
>> ---
>>    fs/f2fs/data.c | 20 +++++++++++++++-----
>>    1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 69f1cb0490ee..ee5614324df0 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -1896,7 +1896,7 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>>                  u64 start, u64 len)
>>    {
>>          struct f2fs_map_blocks map;
>> -       sector_t start_blk, last_blk;
>> +       sector_t start_blk, last_blk, blk_len, max_len;
>>          pgoff_t next_pgofs;
>>          u64 logical = 0, phys = 0, size = 0;
>>          u32 flags = 0;
>> @@ -1940,14 +1940,13 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>>
>>          start_blk = F2FS_BYTES_TO_BLK(start);
>>          last_blk = F2FS_BYTES_TO_BLK(start + len - 1);
>> -
>> -       if (len & F2FS_BLKSIZE_MASK)
>> -               len = round_up(len, F2FS_BLKSIZE);
>> +       blk_len = last_blk - start_blk + 1;
>> +       max_len = F2FS_BYTES_TO_BLK(maxbytes) - start_blk;
>>
>>    next:
>>          memset(&map, 0, sizeof(map));
>>          map.m_lblk = start_blk;
>> -       map.m_len = F2FS_BYTES_TO_BLK(len);
>> +       map.m_len = blk_len;
>>          map.m_next_pgofs = &next_pgofs;
>>          map.m_seg_type = NO_CHECK_TYPE;
>>
>> @@ -1970,6 +1969,17 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>>                  flags |= FIEMAP_EXTENT_LAST;
>>          }
>>
>> +       /*
>> +        * current extent may cross boundary of inquiry, increase len to
>> +        * requery.
>> +        */
>> +       if (!compr_cluster && (map.m_flags & F2FS_MAP_MAPPED) &&
>> +                               map.m_lblk + map.m_len - 1 == last_blk &&
>> +                               blk_len != max_len) {
>> +               blk_len = max_len;
>> +               goto next;
>> +       }
>> +
> it seems if user input the lenght which is less than the file size,
> but return the whole fiemap?
> such as:
>   dd if=/dev/zero of=file bs=4k count=5
>   xfs_io file -c "fiemap -v 0 16384"
> will get extent with lenght = 0x5000? Is this expected?
> Or did I understand it wrong?

It's fine?

Quoted from Documentation/filesystems/fiemap.rst

"fm_start, and fm_length specify the logical range within the file
which the process would like mappings for. Extents returned mirror
those on disk - that is, the logical offset of the 1st returned extent
may start before fm_start, and the range covered by the last returned
extent may end after fm_length. All offsets and lengths are in bytes."

Quoted from reply of Darrick:

https://lore.kernel.org/fstests/20210224165057.GB7269@magnolia/

Thanks,

> thanks!
>>          compr_appended = false;
>>          /* In a case of compressed cluster, append this to the last extent */
>>          if (compr_cluster && ((map.m_flags & F2FS_MAP_DELALLOC) ||
>> --
>> 2.40.1
>>
>>> 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,
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>

Re: [PATCH V2] f2fs: fix to adjust appropriate length for fiemap

[Zhiguo Niu]

Chao Yu <chao@kernel.org> 于2024年11月8日周五 09:22写道：
>
> On 2024/11/7 18:53, Zhiguo Niu wrote:
> > Chao Yu <chao@kernel.org> 于2024年11月7日周四 16:22写道：
> >>
> >> On 2024/11/7 14:54, Zhiguo Niu wrote:
> >>> Chao Yu <chao@kernel.org> 于2024年11月7日周四 14:18写道：
> >>>>
> >>>> On 2024/11/6 16:41, Zhiguo Niu wrote:
> >>>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 15:40写道：
> >>>>>>
> >>>>>> On 2024/11/6 14:08, Zhiguo Niu wrote:
> >>>>>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:40写道：
> >>>>>>>>
> >>>>>>>> On 2024/11/6 10:26, Zhiguo Niu wrote:
> >>>>>>>>> Chao Yu <chao@kernel.org> 于2024年11月6日周三 10:16写道：
> >>>>>>>>>>
> >>>>>>>>>> On 2024/11/5 19:02, Zhiguo Niu wrote:
> >>>>>>>>>>> Chao Yu <chao@kernel.org> 于2024年11月5日周二 18:39写道：
> >>>>>>>>>>>>
> >>>>>>>>>>>> On 2024/11/5 15:28, Zhiguo Niu wrote:
> >>>>>>>>>>>>> Chao Yu <chao@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.niu@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?
> >>
> >> Zhiguo, let me send a patchset including your patch, now, I'm testing this:
> > Hi Chao,
> > It's ok ^^
> >>
> >>   From c67cb4782a3f1875865f9ae24cce80a59752d600 Mon Sep 17 00:00:00 2001
> >> From: Chao Yu <chao@kernel.org>
> >> Date: Thu, 7 Nov 2024 14:52:17 +0800
> >> Subject: [PATCH] f2fs: fix to requery extent which cross boundary of inquiry
> >>
> >> dd if=/dev/zero of=file bs=4k count=5
> >> 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
> >>
> >> There are two problems:
> >> - continuous extent is split to two
> >> - FIEMAP_EXTENT_LAST is missing in last extent
> >>
> >> The root cause is: if upper boundary of inquiry crosses extent,
> >> f2fs_map_blocks() will truncate length of returned extent to
> >> F2FS_BYTES_TO_BLK(len), and also, it will stop to query latter
> >> extent or hole to make sure current extent is last or not.
> >>
> >> In order to fix this issue, once we found an extent locates
> >> in the end of inquiry range by f2fs_map_blocks(), we need to
> >> expand inquiry range to requiry.
> >>
> >> Reported-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> >> Signed-off-by: Chao Yu <chao@kernel.org>
> >> ---
> >>    fs/f2fs/data.c | 20 +++++++++++++++-----
> >>    1 file changed, 15 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >> index 69f1cb0490ee..ee5614324df0 100644
> >> --- a/fs/f2fs/data.c
> >> +++ b/fs/f2fs/data.c
> >> @@ -1896,7 +1896,7 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> >>                  u64 start, u64 len)
> >>    {
> >>          struct f2fs_map_blocks map;
> >> -       sector_t start_blk, last_blk;
> >> +       sector_t start_blk, last_blk, blk_len, max_len;
> >>          pgoff_t next_pgofs;
> >>          u64 logical = 0, phys = 0, size = 0;
> >>          u32 flags = 0;
> >> @@ -1940,14 +1940,13 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> >>
> >>          start_blk = F2FS_BYTES_TO_BLK(start);
> >>          last_blk = F2FS_BYTES_TO_BLK(start + len - 1);
> >> -
> >> -       if (len & F2FS_BLKSIZE_MASK)
> >> -               len = round_up(len, F2FS_BLKSIZE);
> >> +       blk_len = last_blk - start_blk + 1;
> >> +       max_len = F2FS_BYTES_TO_BLK(maxbytes) - start_blk;
> >>
> >>    next:
> >>          memset(&map, 0, sizeof(map));
> >>          map.m_lblk = start_blk;
> >> -       map.m_len = F2FS_BYTES_TO_BLK(len);
> >> +       map.m_len = blk_len;
> >>          map.m_next_pgofs = &next_pgofs;
> >>          map.m_seg_type = NO_CHECK_TYPE;
> >>
> >> @@ -1970,6 +1969,17 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> >>                  flags |= FIEMAP_EXTENT_LAST;
> >>          }
> >>
> >> +       /*
> >> +        * current extent may cross boundary of inquiry, increase len to
> >> +        * requery.
> >> +        */
> >> +       if (!compr_cluster && (map.m_flags & F2FS_MAP_MAPPED) &&
> >> +                               map.m_lblk + map.m_len - 1 == last_blk &&
> >> +                               blk_len != max_len) {
> >> +               blk_len = max_len;
> >> +               goto next;
> >> +       }
> >> +
> > it seems if user input the lenght which is less than the file size,
> > but return the whole fiemap?
> > such as:
> >   dd if=/dev/zero of=file bs=4k count=5
> >   xfs_io file -c "fiemap -v 0 16384"
> > will get extent with lenght = 0x5000? Is this expected?
> > Or did I understand it wrong?
>
> It's fine?
>
> Quoted from Documentation/filesystems/fiemap.rst
>
> "fm_start, and fm_length specify the logical range within the file
> which the process would like mappings for. Extents returned mirror
> those on disk - that is, the logical offset of the 1st returned extent
> may start before fm_start, and the range covered by the last returned
> extent may end after fm_length. All offsets and lengths are in bytes."
>
> Quoted from reply of Darrick:
>
> https://lore.kernel.org/fstests/20210224165057.GB7269@magnolia/
Hi Chao,
clear, and verfy thanks for you kindly explanations.
thanks again.
>
> Thanks,
>
> > thanks!
> >>          compr_appended = false;
> >>          /* In a case of compressed cluster, append this to the last extent */
> >>          if (compr_cluster && ((map.m_flags & F2FS_MAP_DELALLOC) ||
> >> --
> >> 2.40.1
> >>
> >>> 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,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>
> >>
>