diff mbox series

f2fs: fix iostat parameter for discard

Message ID 20221205145603.70779-1-frank.li@vivo.com
State New
Headers
Series f2fs: fix iostat parameter for discard |

Commit Message

李扬韬 Dec. 5, 2022, 2:56 p.m. UTC 
  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(-)

Comments

Chao Yu Dec. 11, 2022, 2:23 a.m. UTC | #1 
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;
Jaegeuk Kim Dec. 12, 2022, 10:59 p.m. UTC | #2 
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;
Chao Yu Dec. 13, 2022, 1:46 a.m. UTC | #3 
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;
李扬韬 Dec. 13, 2022, 11:54 a.m. UTC | #4 
> 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
Jaegeuk Kim Dec. 13, 2022, 7:11 p.m. UTC | #5 
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
Chao Yu Dec. 15, 2022, 2:21 a.m. UTC | #6 
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
diff mbox series

Patch

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;