2018-10-10 21:23:25

by Jaegeuk Kim

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

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

Signed-off-by: Jaegeuk Kim <[email protected]>
---
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

2018-10-15 12:39:04

by Chao Yu

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

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 <[email protected]>
> ---
> 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);
>

2018-10-15 23:08:45

by Jaegeuk Kim

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

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 <[email protected]>
> > ---
> > 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);
> >

2018-10-16 01:32:11

by Chao Yu

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

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 <[email protected]>
>>> ---
>>> 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);
>>>
>
> .
>

2018-10-16 03:11:47

by Jaegeuk Kim

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

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 <[email protected]>
> >>> ---
> >>> 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);
> >>>
> >
> > .
> >

2018-10-17 01:31:04

by Chao Yu

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

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 <[email protected]>
>>>>> ---
>>>>> 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);
>>>>>
>>>
>>> .
>>>
>
> .
>

2018-10-17 02:35:23

by Jaegeuk Kim

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

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: <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
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

2018-10-17 02:45:52

by Jaegeuk Kim

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

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 <[email protected]>
> >>>>> ---
> >>>>> 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);
> >>>>>
> >>>
> >>> .
> >>>
> >
> > .
> >

2018-10-17 02:51:37

by Chao Yu

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

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: <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>

Reviewed-by: Chao Yu <[email protected]>

Thanks,