[PATCH] f2fs: fix double count on issued discard commands

[PATCH] f2fs: fix double count on issued discard commands

[Jaegeuk Kim]

If issue_cond is true, it does double count for # of issued commands.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 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 7fd742f747ce..25196ff5d587 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1087,7 +1087,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi, bool issue_cond)
 				issued++;
 				__submit_discard_cmd(sbi, dc);
 			}
-			if (issue_cond && iter++ > DISCARD_ISSUE_RATE)
+			if (issue_cond && iter > DISCARD_ISSUE_RATE)
 				goto out;
 		}
 		if (list_empty(pend_list) && dcc->pend_list_tag[i] & P_TRIM)
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

Re: [f2fs-dev] [PATCH] f2fs: fix double count on issued discard commands

[Chao Yu]

On 2017/9/11 11:38, Jaegeuk Kim wrote:
> If issue_cond is true, it does double count for # of issued commands.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <yuchao0@huawei.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 7fd742f747ce..25196ff5d587 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1087,7 +1087,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi, bool issue_cond)
>  				issued++;
>  				__submit_discard_cmd(sbi, dc);
>  			}
> -			if (issue_cond && iter++ > DISCARD_ISSUE_RATE)
> +			if (issue_cond && iter > DISCARD_ISSUE_RATE)
>  				goto out;
>  		}
>  		if (list_empty(pend_list) && dcc->pend_list_tag[i] & P_TRIM)
> 

Re: [f2fs-dev] [PATCH] f2fs: fix double count on issued discard commands

[Chao Yu]

On 2017/9/11 11:38, Jaegeuk Kim wrote:
> If issue_cond is true, it does double count for # of issued commands.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <yuchao0@huawei.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 7fd742f747ce..25196ff5d587 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1087,7 +1087,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi, bool issue_cond)
>  				issued++;
>  				__submit_discard_cmd(sbi, dc);
>  			}
> -			if (issue_cond && iter++ > DISCARD_ISSUE_RATE)
> +			if (issue_cond && iter > DISCARD_ISSUE_RATE)
>  				goto out;
>  		}
>  		if (list_empty(pend_list) && dcc->pend_list_tag[i] & P_TRIM)
> 

Re: [f2fs-dev] [PATCH] f2fs: fix double count on issued discard commands

[Chao Yu]

On 2017/9/12 9:53, Chao Yu wrote:
> On 2017/9/11 11:38, Jaegeuk Kim wrote:
>> If issue_cond is true, it does double count for # of issued commands.
>>
>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> 
> Reviewed-by: Chao Yu <yuchao0@huawei.com>

As Daeho Jeong mentioned, the change makes 'iter > DISCARD_ISSUE_RATE' dead
code, I just misread iter and issued variable, sorry. :(

Thanks,

> 
>> ---
>>  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 7fd742f747ce..25196ff5d587 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -1087,7 +1087,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi, bool issue_cond)
>>  				issued++;
>>  				__submit_discard_cmd(sbi, dc);
>>  			}
>> -			if (issue_cond && iter++ > DISCARD_ISSUE_RATE)
>> +			if (issue_cond && iter > DISCARD_ISSUE_RATE)
>>  				goto out;
>>  		}
>>  		if (list_empty(pend_list) && dcc->pend_list_tag[i] & P_TRIM)
>>
> 
> 
> 

Re: [f2fs-dev] [PATCH] f2fs: fix double count on issued discard commands

[Chao Yu]

On 2017/9/12 9:53, Chao Yu wrote:
> On 2017/9/11 11:38, Jaegeuk Kim wrote:
>> If issue_cond is true, it does double count for # of issued commands.
>>
>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> 
> Reviewed-by: Chao Yu <yuchao0@huawei.com>

As Daeho Jeong mentioned, the change makes 'iter > DISCARD_ISSUE_RATE' dead
code, I just misread iter and issued variable, sorry. :(

Thanks,

> 
>> ---
>>  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 7fd742f747ce..25196ff5d587 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -1087,7 +1087,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi, bool issue_cond)
>>  				issued++;
>>  				__submit_discard_cmd(sbi, dc);
>>  			}
>> -			if (issue_cond && iter++ > DISCARD_ISSUE_RATE)
>> +			if (issue_cond && iter > DISCARD_ISSUE_RATE)
>>  				goto out;
>>  		}
>>  		if (list_empty(pend_list) && dcc->pend_list_tag[i] & P_TRIM)
>>
> 
> 
> 

Re: [f2fs-dev] [PATCH] f2fs: fix double count on issued discard commands

[Jaegeuk Kim]

On 09/12, Chao Yu wrote:
> On 2017/9/12 9:53, Chao Yu wrote:
> > On 2017/9/11 11:38, Jaegeuk Kim wrote:
> >> If issue_cond is true, it does double count for # of issued commands.
> >>
> >> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > 
> > Reviewed-by: Chao Yu <yuchao0@huawei.com>
> 
> As Daeho Jeong mentioned, the change makes 'iter > DISCARD_ISSUE_RATE' dead
> code, I just misread iter and issued variable, sorry. :(

Yeah, that's exactly like what I made a mistake before.
I should have mentioned that earlier. :)

> 
> Thanks,
> 
> > 
> >> ---
> >>  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 7fd742f747ce..25196ff5d587 100644
> >> --- a/fs/f2fs/segment.c
> >> +++ b/fs/f2fs/segment.c
> >> @@ -1087,7 +1087,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi, bool issue_cond)
> >>  				issued++;
> >>  				__submit_discard_cmd(sbi, dc);
> >>  			}
> >> -			if (issue_cond && iter++ > DISCARD_ISSUE_RATE)
> >> +			if (issue_cond && iter > DISCARD_ISSUE_RATE)
> >>  				goto out;
> >>  		}
> >>  		if (list_empty(pend_list) && dcc->pend_list_tag[i] & P_TRIM)
> >>
> > 
> > 
> > 

Re: [f2fs-dev] [PATCH] f2fs: fix double count on issued discard commands

[Daeho Jeong]

> Yeah, that's exactly like what I made a mistake before.

> I should have mentioned that earlier. :)



Or I think the previous code which used "iter++" might be right.

You might just want to check the fixed number of small discards, DISCARD_ISSUE_RATE,

when issue_cond is "true".



Anyways, I have another question about this function.

How about just issuing, not checking whether it is idle, the fixed number of small

discards, DISCARD_ISSUE_RATE, when issue_cond is "true".

Actually, the discard commands will be issued as "asynchronous" requests,

which has a low priority in the I/O scheduler,

so the performance degradation of other threads by doing this will not be much severe,

but we can make the performance of the storage device better even if there is no idle.



I am just worried about the storage device I/O performance gets worse 

under I/O intensive senario where there is no idle



Thanks,

Re: [f2fs-dev] [PATCH] f2fs: fix double count on issued discard commands

[Daeho Jeong]

> Yeah, that's exactly like what I made a mistake before.

> I should have mentioned that earlier. :)



Or I think the previous code which used "iter++" might be right.

You might just want to check the fixed number of small discards, DISCARD_ISSUE_RATE,

when issue_cond is "true".



Anyways, I have another question about this function.

How about just issuing, not checking whether it is idle, the fixed number of small

discards, DISCARD_ISSUE_RATE, when issue_cond is "true".

Actually, the discard commands will be issued as "asynchronous" requests,

which has a low priority in the I/O scheduler,

so the performance degradation of other threads by doing this will not be much severe,

but we can make the performance of the storage device better even if there is no idle.



I am just worried about the storage device I/O performance gets worse 

under I/O intensive senario where there is no idle



Thanks,

Re: [f2fs-dev] [PATCH] f2fs: fix double count on issued discard commands

[Jaegeuk Kim]

On 09/12, Daeho Jeong wrote:
> > Yeah, that's exactly like what I made a mistake before.
> > I should have mentioned that earlier. :)
> 
> Or I think the previous code which used "iter++" might be right.
> You might just want to check the fixed number of small discards, DISCARD_ISSUE_RATE,
> when issue_cond is "true".
> 
> Anyways, I have another question about this function.
> How about just issuing, not checking whether it is idle, the fixed number of small
> discards, DISCARD_ISSUE_RATE, when issue_cond is "true".
> Actually, the discard commands will be issued as "asynchronous" requests,
> which has a low priority in the I/O scheduler,
> so the performance degradation of other threads by doing this will not be much severe,
> but we can make the performance of the storage device better even if there is no idle.

I don't think I/O scheduler can efficiently prioritize discard commands and user
requests. The proper way that we can do would be waiting for idle time at this
moment.

Thanks,

> 
> I am just worried about the storage device I/O performance gets worse 
> under I/O intensive senario where there is no idle
> 
> Thanks,

Re: [f2fs-dev] [PATCH] f2fs: fix double count on issued discard commands

[Jaegeuk Kim]

On 09/12, Daeho Jeong wrote:
> > Yeah,�that's�exactly�like�what�I�made�a�mistake�before.
> > I�should�have�mentioned�that�earlier.�:)
> 
> Or I think the previous code which used "iter++" might be right.
> You might just want to check the fixed number of small discards, DISCARD_ISSUE_RATE,
> when issue_cond is "true".
> 
> Anyways, I have another question about this function.
> How about just issuing, not checking whether it is idle, the fixed number of small
> discards, DISCARD_ISSUE_RATE, when issue_cond is "true".
> Actually, the discard commands will be issued as "asynchronous" requests,
> which has a low priority in the I/O scheduler,
> so the performance degradation of other threads by doing this will not be much severe,
> but we can make the performance of the storage device better even if there is no idle.

I don't think I/O scheduler can efficiently prioritize discard commands and user
requests. The proper way that we can do would be waiting for idle time at this
moment.

Thanks,

> 
> I am just worried about the storage device I/O performance gets worse 
> under I/O intensive senario where there is no idle
> 
> Thanks,

Re: [PATCH] f2fs: fix double count on issued discard commands

[Jaegeuk Kim]

On 09/12, Daeho Jeong wrote:
> > Yeah, that's exactly like what I made a mistake before.
> > I should have mentioned that earlier. :)
> 
> Or I think the previous code which used "iter++" might be right.
> You might just want to check the fixed number of small discards, DISCARD_ISSUE_RATE,
> when issue_cond is "true".
> 
> Anyways, I have another question about this function.
> How about just issuing, not checking whether it is idle, the fixed number of small
> discards, DISCARD_ISSUE_RATE, when issue_cond is "true".
> Actually, the discard commands will be issued as "asynchronous" requests,
> which has a low priority in the I/O scheduler,
> so the performance degradation of other threads by doing this will not be much severe,
> but we can make the performance of the storage device better even if there is no idle.

I don't think I/O scheduler can efficiently prioritize discard commands and user
requests. The proper way that we can do would be waiting for idle time at this
moment.

Thanks,

> 
> I am just worried about the storage device I/O performance gets worse 
> under I/O intensive senario where there is no idle
> 
> Thanks,

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [f2fs-dev] [PATCH] f2fs: fix double count on issued discard commands

[Chao Yu]

On 2017/9/12 12:34, Daeho Jeong wrote:
>> Yeah, that's exactly like what I made a mistake before.
>> I should have mentioned that earlier. :)
> 
> Or I think the previous code which used "iter++" might be right.
> You might just want to check the fixed number of small discards, DISCARD_ISSUE_RATE,
> when issue_cond is "true".
> 
> Anyways, I have another question about this function.
> How about just issuing, not checking whether it is idle, the fixed number of small
> discards, DISCARD_ISSUE_RATE, when issue_cond is "true".
> Actually, the discard commands will be issued as "asynchronous" requests,
> which has a low priority in the I/O scheduler,
> so the performance degradation of other threads by doing this will not be much severe,
> but we can make the performance of the storage device better even if there is no idle.

Actually, we didn't change priority of discard command, so that it is still
synchronous IO for I/O scheduler, hence I/O interference will still exist if we
try to issue discard without IO aware ability.

Of course we can change the priority of discard command to lower, but potential
issue is that with ROW I/O scheduler in kernel or FTL, async I/O will handle
very slowly in heavy load scenario, if we are going to trigger sync write IO in
place in where we're doing async discard, we will face long latency.

Still I think it is worth to build the ability to issue async discard as a part
of discard policy and later we can adjust policy based on different scenario.

Thanks,

> 
> I am just worried about the storage device I/O performance gets worse 
> under I/O intensive senario where there is no idle
> 
> Thanks,
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 

Re: [f2fs-dev] [PATCH] f2fs: fix double count on issued discard commands

[Chao Yu]

On 2017/9/12 12:34, Daeho Jeong wrote:
>> Yeah, that's exactly like what I made a mistake before.
>> I should have mentioned that earlier. :)
> 
> Or I think the previous code which used "iter++" might be right.
> You might just want to check the fixed number of small discards, DISCARD_ISSUE_RATE,
> when issue_cond is "true".
> 
> Anyways, I have another question about this function.
> How about just issuing, not checking whether it is idle, the fixed number of small
> discards, DISCARD_ISSUE_RATE, when issue_cond is "true".
> Actually, the discard commands will be issued as "asynchronous" requests,
> which has a low priority in the I/O scheduler,
> so the performance degradation of other threads by doing this will not be much severe,
> but we can make the performance of the storage device better even if there is no idle.

Actually, we didn't change priority of discard command, so that it is still
synchronous IO for I/O scheduler, hence I/O interference will still exist if we
try to issue discard without IO aware ability.

Of course we can change the priority of discard command to lower, but potential
issue is that with ROW I/O scheduler in kernel or FTL, async I/O will handle
very slowly in heavy load scenario, if we are going to trigger sync write IO in
place in where we're doing async discard, we will face long latency.

Still I think it is worth to build the ability to issue async discard as a part
of discard policy and later we can adjust policy based on different scenario.

Thanks,

> 
> I am just worried about the storage device I/O performance gets worse 
> under I/O intensive senario where there is no idle
> 
> Thanks,
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 

Re: [PATCH] f2fs: fix double count on issued discard commands

[Chao Yu]

On 2017/9/12 12:34, Daeho Jeong wrote:
>> Yeah, that's exactly like what I made a mistake before.
>> I should have mentioned that earlier. :)
> 
> Or I think the previous code which used "iter++" might be right.
> You might just want to check the fixed number of small discards, DISCARD_ISSUE_RATE,
> when issue_cond is "true".
> 
> Anyways, I have another question about this function.
> How about just issuing, not checking whether it is idle, the fixed number of small
> discards, DISCARD_ISSUE_RATE, when issue_cond is "true".
> Actually, the discard commands will be issued as "asynchronous" requests,
> which has a low priority in the I/O scheduler,
> so the performance degradation of other threads by doing this will not be much severe,
> but we can make the performance of the storage device better even if there is no idle.

Actually, we didn't change priority of discard command, so that it is still
synchronous IO for I/O scheduler, hence I/O interference will still exist if we
try to issue discard without IO aware ability.

Of course we can change the priority of discard command to lower, but potential
issue is that with ROW I/O scheduler in kernel or FTL, async I/O will handle
very slowly in heavy load scenario, if we are going to trigger sync write IO in
place in where we're doing async discard, we will face long latency.

Still I think it is worth to build the ability to issue async discard as a part
of discard policy and later we can adjust policy based on different scenario.

Thanks,

> 
> I am just worried about the storage device I/O performance gets worse 
> under I/O intensive senario where there is no idle
> 
> Thanks,
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: fix double count on issued discard commands

[Daeho Jeong]

> Actually, we didn't change priority of discard command, so that it is still
> synchronous IO for I/O scheduler, hence I/O interference will still exist if we
> try to issue discard without IO aware ability.
 
> Of course we can change the priority of discard command to lower, but potential
> issue is that with ROW I/O scheduler in kernel or FTL, async I/O will handle
> very slowly in heavy load scenario, if we are going to trigger sync write IO in
> place in where we're doing async discard, we will face long latency.
 
> Still I think it is worth to build the ability to issue async discard as a part
> of discard policy and later we can adjust policy based on different scenario.
 
> Thanks,

Oh, I see.

f2fs is sending discard requests as "sync" requests, I didn't know that.
Right, I just though in case of CFQ I/O scheduler, but f2fs has to consider the other
schedulers, but CFQ.

Thanks, :)

Re: [f2fs-dev] [PATCH] f2fs: fix double count on issued discard commands

[Daeho Jeong]

> Actually, we didn't change priority of discard command, so that it is still
> synchronous IO for I/O scheduler, hence I/O interference will still exist if we
> try to issue discard without IO aware ability.
 
> Of course we can change the priority of discard command to lower, but potential
> issue is that with ROW I/O scheduler in kernel or FTL, async I/O will handle
> very slowly in heavy load scenario, if we are going to trigger sync write IO in
> place in where we're doing async discard, we will face long latency.
 
> Still I think it is worth to build the ability to issue async discard as a part
> of discard policy and later we can adjust policy based on different scenario.
 
> Thanks,

Oh, I see.

f2fs is sending discard requests as "sync" requests, I didn't know that.
Right, I just though in case of CFQ I/O scheduler, but f2fs has to consider the other
schedulers, but CFQ.

Thanks, :)