[PATCH] f2fs: fix iostat parameter for discard

[PATCH] f2fs: fix iostat parameter for discard

[Yangtao Li]

Just like other data we count uses the number of bytes as the basic unit,
but discard uses the number of cmds as the statistical unit. In fact the
discard command contains the number of blocks, so let's change to the
number of bytes as the base unit.

Fixes: b0af6d491a6b ("f2fs: add app/fs io stat")

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/f2fs/segment.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 9486ca49ecb1..bc262e17b279 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1181,7 +1181,7 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
 
 		atomic_inc(&dcc->issued_discard);
 
-		f2fs_update_iostat(sbi, NULL, FS_DISCARD, 1);
+		f2fs_update_iostat(sbi, NULL, FS_DISCARD, len * F2FS_BLKSIZE);
 
 		lstart += len;
 		start += len;
-- 
2.25.1

Re: [PATCH] f2fs: fix iostat parameter for discard

[Chao Yu]

On 2022/12/5 22:56, Yangtao Li wrote:
> Just like other data we count uses the number of bytes as the basic unit,
> but discard uses the number of cmds as the statistical unit. In fact the
> discard command contains the number of blocks, so let's change to the
> number of bytes as the base unit.
> 
> Fixes: b0af6d491a6b ("f2fs: add app/fs io stat")
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>   fs/f2fs/segment.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 9486ca49ecb1..bc262e17b279 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1181,7 +1181,7 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>   
>   		atomic_inc(&dcc->issued_discard);
>   
> -		f2fs_update_iostat(sbi, NULL, FS_DISCARD, 1);
> +		f2fs_update_iostat(sbi, NULL, FS_DISCARD, len * F2FS_BLKSIZE);

In order to avoid breaking its usage of application, how about keeping
FS_DISCARD as it is, and add FS_DISCARD_IO to account discard bytes?

Thanks,

>   
>   		lstart += len;
>   		start += len;

Re: [PATCH] f2fs: fix iostat parameter for discard

[Jaegeuk Kim]

On 12/11, Chao Yu wrote:
> On 2022/12/5 22:56, Yangtao Li wrote:
> > Just like other data we count uses the number of bytes as the basic unit,
> > but discard uses the number of cmds as the statistical unit. In fact the
> > discard command contains the number of blocks, so let's change to the
> > number of bytes as the base unit.
> > 
> > Fixes: b0af6d491a6b ("f2fs: add app/fs io stat")
> > 
> > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > ---
> >   fs/f2fs/segment.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 9486ca49ecb1..bc262e17b279 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -1181,7 +1181,7 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
> >   		atomic_inc(&dcc->issued_discard);
> > -		f2fs_update_iostat(sbi, NULL, FS_DISCARD, 1);
> > +		f2fs_update_iostat(sbi, NULL, FS_DISCARD, len * F2FS_BLKSIZE);
> 
> In order to avoid breaking its usage of application, how about keeping
> FS_DISCARD as it is, and add FS_DISCARD_IO to account discard bytes?

I picked this to match the f2fs_update_iostat() definition. Do we know
how many applications will be affected? I don't have any at a glance in
Android tho.

void f2fs_update_iostat(struct f2fs_sb_info *sbi, struct inode *inode,
                        enum iostat_type type, unsigned long long io_bytes)


> 
> Thanks,
> 
> >   		lstart += len;
> >   		start += len;

Re: [PATCH] f2fs: fix iostat parameter for discard

[Chao Yu]

On 2022/12/13 6:59, Jaegeuk Kim wrote:
> On 12/11, Chao Yu wrote:
>> On 2022/12/5 22:56, Yangtao Li wrote:
>>> Just like other data we count uses the number of bytes as the basic unit,
>>> but discard uses the number of cmds as the statistical unit. In fact the
>>> discard command contains the number of blocks, so let's change to the
>>> number of bytes as the base unit.
>>>
>>> Fixes: b0af6d491a6b ("f2fs: add app/fs io stat")
>>>
>>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>>> ---
>>>    fs/f2fs/segment.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index 9486ca49ecb1..bc262e17b279 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -1181,7 +1181,7 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>>>    		atomic_inc(&dcc->issued_discard);
>>> -		f2fs_update_iostat(sbi, NULL, FS_DISCARD, 1);
>>> +		f2fs_update_iostat(sbi, NULL, FS_DISCARD, len * F2FS_BLKSIZE);
>>
>> In order to avoid breaking its usage of application, how about keeping
>> FS_DISCARD as it is, and add FS_DISCARD_IO to account discard bytes?
> 
> I picked this to match the f2fs_update_iostat() definition. Do we know
> how many applications will be affected? I don't have any at a glance in
> Android tho.

I guess there is some private scripts in OEM rely on this, and I don't
see any non-Android users using this.

> 
> void f2fs_update_iostat(struct f2fs_sb_info *sbi, struct inode *inode,
>                          enum iostat_type type, unsigned long long io_bytes)

What do you think of extending this function to support io_counts?

void f2fs_update_iostat(struct f2fs_sb_info *sbi, struct inode *inode,
			enum iostat_type type, unsigned long long io_bytes,
			unsigned long long io_counts)

Thanks,

> 
> 
>>
>> Thanks,
>>
>>>    		lstart += len;
>>>    		start += len;

Re: [PATCH] f2fs: fix iostat parameter for discard

[Yangtao Li]

> What do you think of extending this function to support io_counts?
> 
> void f2fs_update_iostat(struct f2fs_sb_info *sbi, struct inode *inode,
> 			enum iostat_type type, unsigned long long io_bytes,
> 			unsigned long long io_counts)

Support to have extra io_count.

But I don't think there is any need to add additional parameters to f2fs_update_iostat.
IIUC, each call to f2fs_update_iostat means that the corresponding count increases by 1,
so only the internal processing of the function is required.

BTW, let's type out the iocount of the additional record in the following way?

time:           1670930162
[WRITE]
app buffered data:      4096(1)

Thx,
Yangtao

Re: [PATCH] f2fs: fix iostat parameter for discard

[Jaegeuk Kim]

On 12/13, Yangtao Li wrote:
> > What do you think of extending this function to support io_counts?
> > 
> > void f2fs_update_iostat(struct f2fs_sb_info *sbi, struct inode *inode,
> > 			enum iostat_type type, unsigned long long io_bytes,
> > 			unsigned long long io_counts)
> 
> Support to have extra io_count.
> 
> But I don't think there is any need to add additional parameters to f2fs_update_iostat.
> IIUC, each call to f2fs_update_iostat means that the corresponding count increases by 1,
> so only the internal processing of the function is required.
> 
> BTW, let's type out the iocount of the additional record in the following way?
> 
> time:           1670930162
> [WRITE]
> app buffered data:      4096(1)

How about giving in another columns with additional stats like avg. len/call or max. len?

app buffered data:      4096	1

> 
> Thx,
> Yangtao

Re: [PATCH] f2fs: fix iostat parameter for discard

[Chao Yu]

On 2022/12/14 3:11, Jaegeuk Kim wrote:
> On 12/13, Yangtao Li wrote:
>>> What do you think of extending this function to support io_counts?
>>>
>>> void f2fs_update_iostat(struct f2fs_sb_info *sbi, struct inode *inode,
>>> 			enum iostat_type type, unsigned long long io_bytes,
>>> 			unsigned long long io_counts)
>>
>> Support to have extra io_count.
>>
>> But I don't think there is any need to add additional parameters to f2fs_update_iostat.
>> IIUC, each call to f2fs_update_iostat means that the corresponding count increases by 1,
>> so only the internal processing of the function is required.
>>
>> BTW, let's type out the iocount of the additional record in the following way?
>>
>> time:           1670930162
>> [WRITE]
>> app buffered data:      4096(1)
> 
> How about giving in another columns with additional stats like avg. len/call or max. len?

Maybe call is better? w/ it we can calculate avg. len/call.

Thanks,

> 
> app buffered data:      4096	1
> 
>>
>> Thx,
>> Yangtao