[PATCH] f2fs: clear cold data flag if IO is not counted

[PATCH] f2fs: clear cold data flag if IO is not counted

[Jaegeuk Kim]

If we clear the cold data flag out of the writeback flow, we can miscount
-1 by end_io.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/data.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 29a9d3b8f709..4102799b5558 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2636,10 +2636,6 @@ static int f2fs_set_data_page_dirty(struct page *page)
 	if (!PageUptodate(page))
 		SetPageUptodate(page);
 
-	/* don't remain PG_checked flag which was set during GC */
-	if (is_cold_data(page))
-		clear_cold_data(page);
-
 	if (f2fs_is_atomic_file(inode) && !f2fs_is_commit_atomic_write(inode)) {
 		if (!IS_ATOMIC_WRITTEN_PAGE(page)) {
 			f2fs_register_inmem_page(inode, page);
-- 
2.19.0.605.g01d371f741-goog

Re: [f2fs-dev] [PATCH] f2fs: clear cold data flag if IO is not counted

[Chao Yu]

On 2018/10/11 5:22, Jaegeuk Kim wrote:
> If we clear the cold data flag out of the writeback flow, we can miscount
> -1 by end_io.

I didn't get it, which count do you mean?

Thanks,

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/data.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 29a9d3b8f709..4102799b5558 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2636,10 +2636,6 @@ static int f2fs_set_data_page_dirty(struct page *page)
>  	if (!PageUptodate(page))
>  		SetPageUptodate(page);
>  
> -	/* don't remain PG_checked flag which was set during GC */
> -	if (is_cold_data(page))
> -		clear_cold_data(page);
> -
>  	if (f2fs_is_atomic_file(inode) && !f2fs_is_commit_atomic_write(inode)) {
>  		if (!IS_ATOMIC_WRITTEN_PAGE(page)) {
>  			f2fs_register_inmem_page(inode, page);
> 

Re: [f2fs-dev] [PATCH] f2fs: clear cold data flag if IO is not counted

[Jaegeuk Kim]

On 10/15, Chao Yu wrote:
> On 2018/10/11 5:22, Jaegeuk Kim wrote:
> > If we clear the cold data flag out of the writeback flow, we can miscount
> > -1 by end_io.
> 
> I didn't get it, which count do you mean?

It's the number of dirty pages.

Balancing F2FS Async:
  - IO (CP:    1, Data:   -1, Flush: (   0    0    1), Discard: (   0 129304)) cmd:    0 undiscard:   0

> 
> Thanks,
> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/data.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 29a9d3b8f709..4102799b5558 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -2636,10 +2636,6 @@ static int f2fs_set_data_page_dirty(struct page *page)
> >  	if (!PageUptodate(page))
> >  		SetPageUptodate(page);
> >  
> > -	/* don't remain PG_checked flag which was set during GC */
> > -	if (is_cold_data(page))
> > -		clear_cold_data(page);
> > -
> >  	if (f2fs_is_atomic_file(inode) && !f2fs_is_commit_atomic_write(inode)) {
> >  		if (!IS_ATOMIC_WRITTEN_PAGE(page)) {
> >  			f2fs_register_inmem_page(inode, page);
> > 

Re: [f2fs-dev] [PATCH] f2fs: clear cold data flag if IO is not counted

[Chao Yu]

On 2018/10/16 7:08, Jaegeuk Kim wrote:
> On 10/15, Chao Yu wrote:
>> On 2018/10/11 5:22, Jaegeuk Kim wrote:
>>> If we clear the cold data flag out of the writeback flow, we can miscount
>>> -1 by end_io.
>>
>> I didn't get it, which count do you mean?
> 
> It's the number of dirty pages.
> 
> Balancing F2FS Async:
>   - IO (CP:    1, Data:   -1, Flush: (   0    0    1), Discard: (   0 129304)) cmd:    0 undiscard:   0
> 

So I guess the race should be:

GC thread:				IRQ
- move_data_page()
 - set_page_dirty()
  - clear_cold_data()
					- f2fs_write_end_io()
					 - type = WB_DATA_TYPE(page);
					   here, we get wrong type
					 - dec_page_count(sbi, type);
 - f2fs_wait_on_page_writeback()

How about relocate wait_writeback & set_page_dirty to avoid above race:

move_data_page()

retry:
		f2fs_wait_on_page_writeback(page, DATA, true);
		set_page_dirty(page);

Thanks,

>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>>  fs/f2fs/data.c | 4 ----
>>>  1 file changed, 4 deletions(-)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 29a9d3b8f709..4102799b5558 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -2636,10 +2636,6 @@ static int f2fs_set_data_page_dirty(struct page *page)
>>>  	if (!PageUptodate(page))
>>>  		SetPageUptodate(page);
>>>  
>>> -	/* don't remain PG_checked flag which was set during GC */
>>> -	if (is_cold_data(page))
>>> -		clear_cold_data(page);
>>> -
>>>  	if (f2fs_is_atomic_file(inode) && !f2fs_is_commit_atomic_write(inode)) {
>>>  		if (!IS_ATOMIC_WRITTEN_PAGE(page)) {
>>>  			f2fs_register_inmem_page(inode, page);
>>>
> 
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: clear cold data flag if IO is not counted

[Jaegeuk Kim]

On 10/16, Chao Yu wrote:
> On 2018/10/16 7:08, Jaegeuk Kim wrote:
> > On 10/15, Chao Yu wrote:
> >> On 2018/10/11 5:22, Jaegeuk Kim wrote:
> >>> If we clear the cold data flag out of the writeback flow, we can miscount
> >>> -1 by end_io.
> >>
> >> I didn't get it, which count do you mean?
> > 
> > It's the number of dirty pages.
> > 
> > Balancing F2FS Async:
> >   - IO (CP:    1, Data:   -1, Flush: (   0    0    1), Discard: (   0 129304)) cmd:    0 undiscard:   0
> > 
> 
> So I guess the race should be:
> 
> GC thread:				IRQ
> - move_data_page()
>  - set_page_dirty()
>   - clear_cold_data()
> 					- f2fs_write_end_io()
> 					 - type = WB_DATA_TYPE(page);
> 					   here, we get wrong type
> 					 - dec_page_count(sbi, type);
>  - f2fs_wait_on_page_writeback()
> 
> How about relocate wait_writeback & set_page_dirty to avoid above race:

Well, wait_on_stable_page() doesn't guarantee its end_io. So, I'm not sure
this is the only case.

> 
> move_data_page()
> 
> retry:
> 		f2fs_wait_on_page_writeback(page, DATA, true);
> 		set_page_dirty(page);
> 
> Thanks,
> 
> >>
> >> Thanks,
> >>
> >>>
> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>> ---
> >>>  fs/f2fs/data.c | 4 ----
> >>>  1 file changed, 4 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>> index 29a9d3b8f709..4102799b5558 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -2636,10 +2636,6 @@ static int f2fs_set_data_page_dirty(struct page *page)
> >>>  	if (!PageUptodate(page))
> >>>  		SetPageUptodate(page);
> >>>  
> >>> -	/* don't remain PG_checked flag which was set during GC */
> >>> -	if (is_cold_data(page))
> >>> -		clear_cold_data(page);
> >>> -
> >>>  	if (f2fs_is_atomic_file(inode) && !f2fs_is_commit_atomic_write(inode)) {
> >>>  		if (!IS_ATOMIC_WRITTEN_PAGE(page)) {
> >>>  			f2fs_register_inmem_page(inode, page);
> >>>
> > 
> > .
> > 

Re: [f2fs-dev] [PATCH] f2fs: clear cold data flag if IO is not counted

[Chao Yu]

On 2018/10/16 11:10, Jaegeuk Kim wrote:
> On 10/16, Chao Yu wrote:
>> On 2018/10/16 7:08, Jaegeuk Kim wrote:
>>> On 10/15, Chao Yu wrote:
>>>> On 2018/10/11 5:22, Jaegeuk Kim wrote:
>>>>> If we clear the cold data flag out of the writeback flow, we can miscount
>>>>> -1 by end_io.
>>>>
>>>> I didn't get it, which count do you mean?
>>>
>>> It's the number of dirty pages.
>>>
>>> Balancing F2FS Async:
>>>   - IO (CP:    1, Data:   -1, Flush: (   0    0    1), Discard: (   0 129304)) cmd:    0 undiscard:   0

Better to add such info in commit message. :)

>>>
>>
>> So I guess the race should be:
>>
>> GC thread:				IRQ
>> - move_data_page()
>>  - set_page_dirty()
>>   - clear_cold_data()
>> 					- f2fs_write_end_io()
>> 					 - type = WB_DATA_TYPE(page);
>> 					   here, we get wrong type
>> 					 - dec_page_count(sbi, type);
>>  - f2fs_wait_on_page_writeback()
>>
>> How about relocate wait_writeback & set_page_dirty to avoid above race:
> 
> Well, wait_on_stable_page() doesn't guarantee its end_io. So, I'm not sure
> this is the only case.

Yes, you're right, I missed that case.

Can you use git-revert to generate the patch? so we can remain original
commit info for better backward tracking.

How do you think pick up original patch I submitted:

https://lkml.org/lkml/2018/7/27/415

Thanks,

> 
>>
>> move_data_page()
>>
>> retry:
>> 		f2fs_wait_on_page_writeback(page, DATA, true);
>> 		set_page_dirty(page);
>>
>> Thanks,
>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>> ---
>>>>>  fs/f2fs/data.c | 4 ----
>>>>>  1 file changed, 4 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>> index 29a9d3b8f709..4102799b5558 100644
>>>>> --- a/fs/f2fs/data.c
>>>>> +++ b/fs/f2fs/data.c
>>>>> @@ -2636,10 +2636,6 @@ static int f2fs_set_data_page_dirty(struct page *page)
>>>>>  	if (!PageUptodate(page))
>>>>>  		SetPageUptodate(page);
>>>>>  
>>>>> -	/* don't remain PG_checked flag which was set during GC */
>>>>> -	if (is_cold_data(page))
>>>>> -		clear_cold_data(page);
>>>>> -
>>>>>  	if (f2fs_is_atomic_file(inode) && !f2fs_is_commit_atomic_write(inode)) {
>>>>>  		if (!IS_ATOMIC_WRITTEN_PAGE(page)) {
>>>>>  			f2fs_register_inmem_page(inode, page);
>>>>>
>>>
>>> .
>>>
> 
> .
> 

Re: [PATCH v2] f2fs: clear cold data flag if IO is not counted

[Jaegeuk Kim]

This reverts commit 66110abc4c931f879d70e83e1281f891699364bf.

If we clear the cold data flag out of the writeback flow, we can miscount
-1 by end_io.

Balancing F2FS Async:
 - IO (CP:    1, Data:   -1, Flush: (   0    0    1), Discard: (   ...

GC thread:                              IRQ
- move_data_page()
 - set_page_dirty()
  - clear_cold_data()
                                        - f2fs_write_end_io()
                                         - type = WB_DATA_TYPE(page);
                                           here, we get wrong type
                                         - dec_page_count(sbi, type);
 - f2fs_wait_on_page_writeback()

Cc: <stable@vger.kernel.org>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/data.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 3f272c18fb61..0d0b4dd55b04 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2650,10 +2650,6 @@ static int f2fs_set_data_page_dirty(struct page *page)
 	if (!PageUptodate(page))
 		SetPageUptodate(page);
 
-	/* don't remain PG_checked flag which was set during GC */
-	if (is_cold_data(page))
-		clear_cold_data(page);
-
 	if (f2fs_is_atomic_file(inode) && !f2fs_is_commit_atomic_write(inode)) {
 		if (!IS_ATOMIC_WRITTEN_PAGE(page)) {
 			f2fs_register_inmem_page(inode, page);
-- 
2.19.0.605.g01d371f741-goog

Re: [f2fs-dev] [PATCH] f2fs: clear cold data flag if IO is not counted

[Jaegeuk Kim]

On 10/17, Chao Yu wrote:
> On 2018/10/16 11:10, Jaegeuk Kim wrote:
> > On 10/16, Chao Yu wrote:
> >> On 2018/10/16 7:08, Jaegeuk Kim wrote:
> >>> On 10/15, Chao Yu wrote:
> >>>> On 2018/10/11 5:22, Jaegeuk Kim wrote:
> >>>>> If we clear the cold data flag out of the writeback flow, we can miscount
> >>>>> -1 by end_io.
> >>>>
> >>>> I didn't get it, which count do you mean?
> >>>
> >>> It's the number of dirty pages.
> >>>
> >>> Balancing F2FS Async:
> >>>   - IO (CP:    1, Data:   -1, Flush: (   0    0    1), Discard: (   0 129304)) cmd:    0 undiscard:   0
> 
> Better to add such info in commit message. :)
> 
> >>>
> >>
> >> So I guess the race should be:
> >>
> >> GC thread:				IRQ
> >> - move_data_page()
> >>  - set_page_dirty()
> >>   - clear_cold_data()
> >> 					- f2fs_write_end_io()
> >> 					 - type = WB_DATA_TYPE(page);
> >> 					   here, we get wrong type
> >> 					 - dec_page_count(sbi, type);
> >>  - f2fs_wait_on_page_writeback()
> >>
> >> How about relocate wait_writeback & set_page_dirty to avoid above race:
> > 
> > Well, wait_on_stable_page() doesn't guarantee its end_io. So, I'm not sure
> > this is the only case.
> 
> Yes, you're right, I missed that case.
> 
> Can you use git-revert to generate the patch? so we can remain original
> commit info for better backward tracking.
> 
> How do you think pick up original patch I submitted:
> 
> https://lkml.org/lkml/2018/7/27/415

Seems that works.

Thanks,

> 
> Thanks,
> 
> > 
> >>
> >> move_data_page()
> >>
> >> retry:
> >> 		f2fs_wait_on_page_writeback(page, DATA, true);
> >> 		set_page_dirty(page);
> >>
> >> Thanks,
> >>
> >>>>
> >>>> Thanks,
> >>>>
> >>>>>
> >>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>>>> ---
> >>>>>  fs/f2fs/data.c | 4 ----
> >>>>>  1 file changed, 4 deletions(-)
> >>>>>
> >>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>>>> index 29a9d3b8f709..4102799b5558 100644
> >>>>> --- a/fs/f2fs/data.c
> >>>>> +++ b/fs/f2fs/data.c
> >>>>> @@ -2636,10 +2636,6 @@ static int f2fs_set_data_page_dirty(struct page *page)
> >>>>>  	if (!PageUptodate(page))
> >>>>>  		SetPageUptodate(page);
> >>>>>  
> >>>>> -	/* don't remain PG_checked flag which was set during GC */
> >>>>> -	if (is_cold_data(page))
> >>>>> -		clear_cold_data(page);
> >>>>> -
> >>>>>  	if (f2fs_is_atomic_file(inode) && !f2fs_is_commit_atomic_write(inode)) {
> >>>>>  		if (!IS_ATOMIC_WRITTEN_PAGE(page)) {
> >>>>>  			f2fs_register_inmem_page(inode, page);
> >>>>>
> >>>
> >>> .
> >>>
> > 
> > .
> > 

Re: [f2fs-dev] [PATCH v2] f2fs: clear cold data flag if IO is not counted

[Chao Yu]

On 2018/10/17 10:34, Jaegeuk Kim wrote:
> This reverts commit 66110abc4c931f879d70e83e1281f891699364bf.
> 
> If we clear the cold data flag out of the writeback flow, we can miscount
> -1 by end_io.
> 
> Balancing F2FS Async:
>  - IO (CP:    1, Data:   -1, Flush: (   0    0    1), Discard: (   ...
> 
> GC thread:                              IRQ
> - move_data_page()
>  - set_page_dirty()
>   - clear_cold_data()
>                                         - f2fs_write_end_io()
>                                          - type = WB_DATA_TYPE(page);
>                                            here, we get wrong type
>                                          - dec_page_count(sbi, type);
>  - f2fs_wait_on_page_writeback()
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,