* [PATCH] f2fs: clear cold data flag if IO is not counted @ 2018-10-10 21:22 Jaegeuk Kim 2018-10-15 12:38 ` [f2fs-dev] " Chao Yu 2018-10-17 2:34 ` [PATCH v2] " Jaegeuk Kim 0 siblings, 2 replies; 9+ messages in thread From: Jaegeuk Kim @ 2018-10-10 21:22 UTC (permalink / raw) To: linux-kernel, linux-f2fs-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: clear cold data flag if IO is not counted 2018-10-10 21:22 [PATCH] f2fs: clear cold data flag if IO is not counted Jaegeuk Kim @ 2018-10-15 12:38 ` Chao Yu 2018-10-15 23:08 ` Jaegeuk Kim 2018-10-17 2:34 ` [PATCH v2] " Jaegeuk Kim 1 sibling, 1 reply; 9+ messages in thread From: Chao Yu @ 2018-10-15 12:38 UTC (permalink / raw) To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel 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); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: clear cold data flag if IO is not counted 2018-10-15 12:38 ` [f2fs-dev] " Chao Yu @ 2018-10-15 23:08 ` Jaegeuk Kim 2018-10-16 1:31 ` Chao Yu 0 siblings, 1 reply; 9+ messages in thread From: Jaegeuk Kim @ 2018-10-15 23:08 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel 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); > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: clear cold data flag if IO is not counted 2018-10-15 23:08 ` Jaegeuk Kim @ 2018-10-16 1:31 ` Chao Yu 2018-10-16 3:10 ` Jaegeuk Kim 0 siblings, 1 reply; 9+ messages in thread From: Chao Yu @ 2018-10-16 1:31 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel 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); >>> > > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: clear cold data flag if IO is not counted 2018-10-16 1:31 ` Chao Yu @ 2018-10-16 3:10 ` Jaegeuk Kim 2018-10-17 1:30 ` Chao Yu 0 siblings, 1 reply; 9+ messages in thread From: Jaegeuk Kim @ 2018-10-16 3:10 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel 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); > >>> > > > > . > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: clear cold data flag if IO is not counted 2018-10-16 3:10 ` Jaegeuk Kim @ 2018-10-17 1:30 ` Chao Yu 2018-10-17 2:43 ` Jaegeuk Kim 0 siblings, 1 reply; 9+ messages in thread From: Chao Yu @ 2018-10-17 1:30 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel 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); >>>>> >>> >>> . >>> > > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: clear cold data flag if IO is not counted 2018-10-17 1:30 ` Chao Yu @ 2018-10-17 2:43 ` Jaegeuk Kim 0 siblings, 0 replies; 9+ messages in thread From: Jaegeuk Kim @ 2018-10-17 2:43 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel 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); > >>>>> > >>> > >>> . > >>> > > > > . > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] f2fs: clear cold data flag if IO is not counted 2018-10-10 21:22 [PATCH] f2fs: clear cold data flag if IO is not counted Jaegeuk Kim 2018-10-15 12:38 ` [f2fs-dev] " Chao Yu @ 2018-10-17 2:34 ` Jaegeuk Kim 2018-10-17 2:50 ` [f2fs-dev] " Chao Yu 1 sibling, 1 reply; 9+ messages in thread From: Jaegeuk Kim @ 2018-10-17 2:34 UTC (permalink / raw) To: linux-kernel, linux-f2fs-devel 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: clear cold data flag if IO is not counted 2018-10-17 2:34 ` [PATCH v2] " Jaegeuk Kim @ 2018-10-17 2:50 ` Chao Yu 0 siblings, 0 replies; 9+ messages in thread From: Chao Yu @ 2018-10-17 2:50 UTC (permalink / raw) To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel 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, ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-10-17 2:51 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-10 21:22 [PATCH] f2fs: clear cold data flag if IO is not counted Jaegeuk Kim 2018-10-15 12:38 ` [f2fs-dev] " Chao Yu 2018-10-15 23:08 ` Jaegeuk Kim 2018-10-16 1:31 ` Chao Yu 2018-10-16 3:10 ` Jaegeuk Kim 2018-10-17 1:30 ` Chao Yu 2018-10-17 2:43 ` Jaegeuk Kim 2018-10-17 2:34 ` [PATCH v2] " Jaegeuk Kim 2018-10-17 2:50 ` [f2fs-dev] " Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).