* [f2fs-dev] [PATCH] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error @ 2020-05-22 14:47 Jaegeuk Kim 2020-05-22 23:32 ` Jaegeuk Kim 2020-05-25 2:16 ` [f2fs-dev] [PATCH] " Chao Yu 0 siblings, 2 replies; 12+ messages in thread From: Jaegeuk Kim @ 2020-05-22 14:47 UTC (permalink / raw) To: linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Jaegeuk Kim Shutdown test is somtime hung, since dirty node pages weren't flushed out. Let's drop dirty pages at umount after shutdown. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/node.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index e632de10aedab..8c63964a82fd0 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1520,8 +1520,15 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted, trace_f2fs_writepage(page, NODE); - if (unlikely(f2fs_cp_error(sbi))) + if (unlikely(f2fs_cp_error(sbi))) { + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { + dec_page_count(sbi, F2FS_DIRTY_NODES); + up_read(&sbi->node_write); + unlock_page(page); + return 0; + } goto redirty_out; + } if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) goto redirty_out; -- 2.27.0.rc0.183.gde8f92d652-goog _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error 2020-05-22 14:47 [f2fs-dev] [PATCH] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error Jaegeuk Kim @ 2020-05-22 23:32 ` Jaegeuk Kim 2020-05-25 3:56 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim 2020-05-25 2:16 ` [f2fs-dev] [PATCH] " Chao Yu 1 sibling, 1 reply; 12+ messages in thread From: Jaegeuk Kim @ 2020-05-22 23:32 UTC (permalink / raw) To: linux-kernel, linux-f2fs-devel, kernel-team Shutdown test is somtimes hung, since it keeps trying to flush dirty node pages in an inifinite loop. Let's drop dirty pages at umount in that case. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- v2: - fix typos fs/f2fs/node.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index e632de10aedab..8c63964a82fd0 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1520,8 +1520,15 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted, trace_f2fs_writepage(page, NODE); - if (unlikely(f2fs_cp_error(sbi))) + if (unlikely(f2fs_cp_error(sbi))) { + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { + dec_page_count(sbi, F2FS_DIRTY_NODES); + up_read(&sbi->node_write); + unlock_page(page); + return 0; + } goto redirty_out; + } if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) goto redirty_out; -- 2.27.0.rc0.183.gde8f92d652-goog _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error 2020-05-22 23:32 ` Jaegeuk Kim @ 2020-05-25 3:56 ` Jaegeuk Kim 2020-05-25 6:30 ` Chao Yu 0 siblings, 1 reply; 12+ messages in thread From: Jaegeuk Kim @ 2020-05-25 3:56 UTC (permalink / raw) To: linux-kernel, linux-f2fs-devel, kernel-team Shutdown test is somtimes hung, since it keeps trying to flush dirty node pages in an inifinite loop. Let's drop dirty pages at umount in that case. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- v3: - fix wrong unlock v2: - fix typos fs/f2fs/node.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index e632de10aedab..e0bb0f7e0506e 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1520,8 +1520,15 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted, trace_f2fs_writepage(page, NODE); - if (unlikely(f2fs_cp_error(sbi))) + if (unlikely(f2fs_cp_error(sbi))) { + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { + ClearPageUptodate(page); + dec_page_count(sbi, F2FS_DIRTY_NODES); + unlock_page(page); + return 0; + } goto redirty_out; + } if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) goto redirty_out; -- 2.27.0.rc0.183.gde8f92d652-goog _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error 2020-05-25 3:56 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim @ 2020-05-25 6:30 ` Chao Yu 2020-05-25 15:06 ` Jaegeuk Kim 0 siblings, 1 reply; 12+ messages in thread From: Chao Yu @ 2020-05-25 6:30 UTC (permalink / raw) To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel, kernel-team On 2020/5/25 11:56, Jaegeuk Kim wrote: > Shutdown test is somtimes hung, since it keeps trying to flush dirty node pages IMO, for umount case, we should drop dirty reference and dirty pages on meta/data pages like we change for node pages to avoid potential dead loop... Thanks, > in an inifinite loop. Let's drop dirty pages at umount in that case. > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > v3: > - fix wrong unlock > > v2: > - fix typos > > fs/f2fs/node.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > index e632de10aedab..e0bb0f7e0506e 100644 > --- a/fs/f2fs/node.c > +++ b/fs/f2fs/node.c > @@ -1520,8 +1520,15 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted, > > trace_f2fs_writepage(page, NODE); > > - if (unlikely(f2fs_cp_error(sbi))) > + if (unlikely(f2fs_cp_error(sbi))) { > + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { > + ClearPageUptodate(page); > + dec_page_count(sbi, F2FS_DIRTY_NODES); > + unlock_page(page); > + return 0; > + } > goto redirty_out; > + } > > if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) > goto redirty_out; > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error 2020-05-25 6:30 ` Chao Yu @ 2020-05-25 15:06 ` Jaegeuk Kim 2020-05-26 1:11 ` Chao Yu 0 siblings, 1 reply; 12+ messages in thread From: Jaegeuk Kim @ 2020-05-25 15:06 UTC (permalink / raw) To: Chao Yu; +Cc: kernel-team, linux-kernel, linux-f2fs-devel On 05/25, Chao Yu wrote: > On 2020/5/25 11:56, Jaegeuk Kim wrote: > > Shutdown test is somtimes hung, since it keeps trying to flush dirty node pages > > IMO, for umount case, we should drop dirty reference and dirty pages on meta/data > pages like we change for node pages to avoid potential dead loop... I believe we're doing for them. :P > > Thanks, > > > in an inifinite loop. Let's drop dirty pages at umount in that case. > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > v3: > > - fix wrong unlock > > > > v2: > > - fix typos > > > > fs/f2fs/node.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > > index e632de10aedab..e0bb0f7e0506e 100644 > > --- a/fs/f2fs/node.c > > +++ b/fs/f2fs/node.c > > @@ -1520,8 +1520,15 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted, > > > > trace_f2fs_writepage(page, NODE); > > > > - if (unlikely(f2fs_cp_error(sbi))) > > + if (unlikely(f2fs_cp_error(sbi))) { > > + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { > > + ClearPageUptodate(page); > > + dec_page_count(sbi, F2FS_DIRTY_NODES); > > + unlock_page(page); > > + return 0; > > + } > > goto redirty_out; > > + } > > > > if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) > > goto redirty_out; > > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error 2020-05-25 15:06 ` Jaegeuk Kim @ 2020-05-26 1:11 ` Chao Yu 2020-05-26 1:34 ` Chao Yu 0 siblings, 1 reply; 12+ messages in thread From: Chao Yu @ 2020-05-26 1:11 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: kernel-team, linux-kernel, linux-f2fs-devel On 2020/5/25 23:06, Jaegeuk Kim wrote: > On 05/25, Chao Yu wrote: >> On 2020/5/25 11:56, Jaegeuk Kim wrote: >>> Shutdown test is somtimes hung, since it keeps trying to flush dirty node pages >> >> IMO, for umount case, we should drop dirty reference and dirty pages on meta/data >> pages like we change for node pages to avoid potential dead loop... > > I believe we're doing for them. :P Actually, I mean do we need to drop dirty meta/data pages explicitly as below: diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 3dc3ac6fe143..4c08fd0a680a 100644 --- a/fs/f2fs/checkpoint.c +++ b/fs/f2fs/checkpoint.c @@ -299,8 +299,15 @@ static int __f2fs_write_meta_page(struct page *page, trace_f2fs_writepage(page, META); - if (unlikely(f2fs_cp_error(sbi))) + if (unlikely(f2fs_cp_error(sbi))) { + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { + ClearPageUptodate(page); + dec_page_count(sbi, F2FS_DIRTY_META); + unlock_page(page); + return 0; + } goto redirty_out; + } if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) goto redirty_out; if (wbc->for_reclaim && page->index < GET_SUM_BLOCK(sbi, 0)) diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 48a622b95b76..94b342802513 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -2682,6 +2682,12 @@ int f2fs_write_single_data_page(struct page *page, int *submitted, /* we should bypass data pages to proceed the kworkder jobs */ if (unlikely(f2fs_cp_error(sbi))) { + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { + ClearPageUptodate(page); + inode_dec_dirty_pages(inode); + unlock_page(page); + return 0; + } mapping_set_error(page->mapping, -EIO); /* * don't drop any dirty dentry pages for keeping lastest > >> >> Thanks, >> >>> in an inifinite loop. Let's drop dirty pages at umount in that case. >>> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>> --- >>> v3: >>> - fix wrong unlock >>> >>> v2: >>> - fix typos >>> >>> fs/f2fs/node.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c >>> index e632de10aedab..e0bb0f7e0506e 100644 >>> --- a/fs/f2fs/node.c >>> +++ b/fs/f2fs/node.c >>> @@ -1520,8 +1520,15 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted, >>> >>> trace_f2fs_writepage(page, NODE); >>> >>> - if (unlikely(f2fs_cp_error(sbi))) >>> + if (unlikely(f2fs_cp_error(sbi))) { >>> + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { >>> + ClearPageUptodate(page); >>> + dec_page_count(sbi, F2FS_DIRTY_NODES); >>> + unlock_page(page); >>> + return 0; >>> + } >>> goto redirty_out; >>> + } >>> >>> if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) >>> goto redirty_out; >>> > . > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error 2020-05-26 1:11 ` Chao Yu @ 2020-05-26 1:34 ` Chao Yu 2020-05-26 1:56 ` Jaegeuk Kim 0 siblings, 1 reply; 12+ messages in thread From: Chao Yu @ 2020-05-26 1:34 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: kernel-team, linux-kernel, linux-f2fs-devel On 2020/5/26 9:11, Chao Yu wrote: > On 2020/5/25 23:06, Jaegeuk Kim wrote: >> On 05/25, Chao Yu wrote: >>> On 2020/5/25 11:56, Jaegeuk Kim wrote: >>>> Shutdown test is somtimes hung, since it keeps trying to flush dirty node pages >>> >>> IMO, for umount case, we should drop dirty reference and dirty pages on meta/data >>> pages like we change for node pages to avoid potential dead loop... >> >> I believe we're doing for them. :P > > Actually, I mean do we need to drop dirty meta/data pages explicitly as below: > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > index 3dc3ac6fe143..4c08fd0a680a 100644 > --- a/fs/f2fs/checkpoint.c > +++ b/fs/f2fs/checkpoint.c > @@ -299,8 +299,15 @@ static int __f2fs_write_meta_page(struct page *page, > > trace_f2fs_writepage(page, META); > > - if (unlikely(f2fs_cp_error(sbi))) > + if (unlikely(f2fs_cp_error(sbi))) { > + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { > + ClearPageUptodate(page); > + dec_page_count(sbi, F2FS_DIRTY_META); > + unlock_page(page); > + return 0; > + } > goto redirty_out; > + } > if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) > goto redirty_out; > if (wbc->for_reclaim && page->index < GET_SUM_BLOCK(sbi, 0)) > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 48a622b95b76..94b342802513 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -2682,6 +2682,12 @@ int f2fs_write_single_data_page(struct page *page, int *submitted, > > /* we should bypass data pages to proceed the kworkder jobs */ > if (unlikely(f2fs_cp_error(sbi))) { > + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { > + ClearPageUptodate(page); > + inode_dec_dirty_pages(inode); > + unlock_page(page); > + return 0; > + } Oh, I notice previously, we will drop non-directory inode's dirty pages directly, however, during umount, we'd better drop directory inode's dirty pages as well, right? > mapping_set_error(page->mapping, -EIO); > /* > * don't drop any dirty dentry pages for keeping lastest > >> >>> >>> Thanks, >>> >>>> in an inifinite loop. Let's drop dirty pages at umount in that case. >>>> >>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>>> --- >>>> v3: >>>> - fix wrong unlock >>>> >>>> v2: >>>> - fix typos >>>> >>>> fs/f2fs/node.c | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c >>>> index e632de10aedab..e0bb0f7e0506e 100644 >>>> --- a/fs/f2fs/node.c >>>> +++ b/fs/f2fs/node.c >>>> @@ -1520,8 +1520,15 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted, >>>> >>>> trace_f2fs_writepage(page, NODE); >>>> >>>> - if (unlikely(f2fs_cp_error(sbi))) >>>> + if (unlikely(f2fs_cp_error(sbi))) { >>>> + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { >>>> + ClearPageUptodate(page); >>>> + dec_page_count(sbi, F2FS_DIRTY_NODES); >>>> + unlock_page(page); >>>> + return 0; >>>> + } >>>> goto redirty_out; >>>> + } >>>> >>>> if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) >>>> goto redirty_out; >>>> >> . >> > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > . > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error 2020-05-26 1:34 ` Chao Yu @ 2020-05-26 1:56 ` Jaegeuk Kim 2020-05-27 2:35 ` Chao Yu 0 siblings, 1 reply; 12+ messages in thread From: Jaegeuk Kim @ 2020-05-26 1:56 UTC (permalink / raw) To: Chao Yu; +Cc: kernel-team, linux-kernel, linux-f2fs-devel On 05/26, Chao Yu wrote: > On 2020/5/26 9:11, Chao Yu wrote: > > On 2020/5/25 23:06, Jaegeuk Kim wrote: > >> On 05/25, Chao Yu wrote: > >>> On 2020/5/25 11:56, Jaegeuk Kim wrote: > >>>> Shutdown test is somtimes hung, since it keeps trying to flush dirty node pages > >>> > >>> IMO, for umount case, we should drop dirty reference and dirty pages on meta/data > >>> pages like we change for node pages to avoid potential dead loop... > >> > >> I believe we're doing for them. :P > > > > Actually, I mean do we need to drop dirty meta/data pages explicitly as below: > > > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > > index 3dc3ac6fe143..4c08fd0a680a 100644 > > --- a/fs/f2fs/checkpoint.c > > +++ b/fs/f2fs/checkpoint.c > > @@ -299,8 +299,15 @@ static int __f2fs_write_meta_page(struct page *page, > > > > trace_f2fs_writepage(page, META); > > > > - if (unlikely(f2fs_cp_error(sbi))) > > + if (unlikely(f2fs_cp_error(sbi))) { > > + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { > > + ClearPageUptodate(page); > > + dec_page_count(sbi, F2FS_DIRTY_META); > > + unlock_page(page); > > + return 0; > > + } > > goto redirty_out; > > + } > > if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) > > goto redirty_out; > > if (wbc->for_reclaim && page->index < GET_SUM_BLOCK(sbi, 0)) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > index 48a622b95b76..94b342802513 100644 > > --- a/fs/f2fs/data.c > > +++ b/fs/f2fs/data.c > > @@ -2682,6 +2682,12 @@ int f2fs_write_single_data_page(struct page *page, int *submitted, > > > > /* we should bypass data pages to proceed the kworkder jobs */ > > if (unlikely(f2fs_cp_error(sbi))) { > > + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { > > + ClearPageUptodate(page); > > + inode_dec_dirty_pages(inode); > > + unlock_page(page); > > + return 0; > > + } > > Oh, I notice previously, we will drop non-directory inode's dirty pages directly, > however, during umount, we'd better drop directory inode's dirty pages as well, right? Hmm, I remember I dropped them before. Need to double check. > > > mapping_set_error(page->mapping, -EIO); > > /* > > * don't drop any dirty dentry pages for keeping lastest > > > >> > >>> > >>> Thanks, > >>> > >>>> in an inifinite loop. Let's drop dirty pages at umount in that case. > >>>> > >>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > >>>> --- > >>>> v3: > >>>> - fix wrong unlock > >>>> > >>>> v2: > >>>> - fix typos > >>>> > >>>> fs/f2fs/node.c | 9 ++++++++- > >>>> 1 file changed, 8 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > >>>> index e632de10aedab..e0bb0f7e0506e 100644 > >>>> --- a/fs/f2fs/node.c > >>>> +++ b/fs/f2fs/node.c > >>>> @@ -1520,8 +1520,15 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted, > >>>> > >>>> trace_f2fs_writepage(page, NODE); > >>>> > >>>> - if (unlikely(f2fs_cp_error(sbi))) > >>>> + if (unlikely(f2fs_cp_error(sbi))) { > >>>> + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { > >>>> + ClearPageUptodate(page); > >>>> + dec_page_count(sbi, F2FS_DIRTY_NODES); > >>>> + unlock_page(page); > >>>> + return 0; > >>>> + } > >>>> goto redirty_out; > >>>> + } > >>>> > >>>> if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) > >>>> goto redirty_out; > >>>> > >> . > >> > > > > > > _______________________________________________ > > Linux-f2fs-devel mailing list > > Linux-f2fs-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > . > > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error 2020-05-26 1:56 ` Jaegeuk Kim @ 2020-05-27 2:35 ` Chao Yu 2020-05-27 20:56 ` Jaegeuk Kim 0 siblings, 1 reply; 12+ messages in thread From: Chao Yu @ 2020-05-27 2:35 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: kernel-team, linux-kernel, linux-f2fs-devel On 2020/5/26 9:56, Jaegeuk Kim wrote: > On 05/26, Chao Yu wrote: >> On 2020/5/26 9:11, Chao Yu wrote: >>> On 2020/5/25 23:06, Jaegeuk Kim wrote: >>>> On 05/25, Chao Yu wrote: >>>>> On 2020/5/25 11:56, Jaegeuk Kim wrote: >>>>>> Shutdown test is somtimes hung, since it keeps trying to flush dirty node pages 71.07% 0.01% kworker/u256:1+ [kernel.kallsyms] [k] wb_writeback | --71.06%--wb_writeback | |--68.96%--__writeback_inodes_wb | | | --68.95%--writeback_sb_inodes | | | |--65.08%--__writeback_single_inode | | | | | --64.35%--do_writepages | | | | | |--59.83%--f2fs_write_node_pages | | | | | | | --59.74%--f2fs_sync_node_pages | | | | | | | |--27.91%--pagevec_lookup_range_tag | | | | | | | | | --27.90%--find_get_pages_range_tag Before umount, kworker will always hold one core, that looks not reasonable, to avoid that, could we just allow node write, since it's out-place-update, and cp is not allowed, we don't need to worry about its effect on data on previous checkpoint, and it can decrease memory footprint cost by node pages. Thanks, >>>>> >>>>> IMO, for umount case, we should drop dirty reference and dirty pages on meta/data >>>>> pages like we change for node pages to avoid potential dead loop... >>>> >>>> I believe we're doing for them. :P >>> >>> Actually, I mean do we need to drop dirty meta/data pages explicitly as below: >>> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c >>> index 3dc3ac6fe143..4c08fd0a680a 100644 >>> --- a/fs/f2fs/checkpoint.c >>> +++ b/fs/f2fs/checkpoint.c >>> @@ -299,8 +299,15 @@ static int __f2fs_write_meta_page(struct page *page, >>> >>> trace_f2fs_writepage(page, META); >>> >>> - if (unlikely(f2fs_cp_error(sbi))) >>> + if (unlikely(f2fs_cp_error(sbi))) { >>> + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { >>> + ClearPageUptodate(page); >>> + dec_page_count(sbi, F2FS_DIRTY_META); >>> + unlock_page(page); >>> + return 0; >>> + } >>> goto redirty_out; >>> + } >>> if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) >>> goto redirty_out; >>> if (wbc->for_reclaim && page->index < GET_SUM_BLOCK(sbi, 0)) >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>> index 48a622b95b76..94b342802513 100644 >>> --- a/fs/f2fs/data.c >>> +++ b/fs/f2fs/data.c >>> @@ -2682,6 +2682,12 @@ int f2fs_write_single_data_page(struct page *page, int *submitted, >>> >>> /* we should bypass data pages to proceed the kworkder jobs */ >>> if (unlikely(f2fs_cp_error(sbi))) { >>> + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { >>> + ClearPageUptodate(page); >>> + inode_dec_dirty_pages(inode); >>> + unlock_page(page); >>> + return 0; >>> + } >> >> Oh, I notice previously, we will drop non-directory inode's dirty pages directly, >> however, during umount, we'd better drop directory inode's dirty pages as well, right? > > Hmm, I remember I dropped them before. Need to double check. > >> >>> mapping_set_error(page->mapping, -EIO); >>> /* >>> * don't drop any dirty dentry pages for keeping lastest >>> >>>> >>>>> >>>>> Thanks, >>>>> >>>>>> in an inifinite loop. Let's drop dirty pages at umount in that case. >>>>>> >>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>>>>> --- >>>>>> v3: >>>>>> - fix wrong unlock >>>>>> >>>>>> v2: >>>>>> - fix typos >>>>>> >>>>>> fs/f2fs/node.c | 9 ++++++++- >>>>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c >>>>>> index e632de10aedab..e0bb0f7e0506e 100644 >>>>>> --- a/fs/f2fs/node.c >>>>>> +++ b/fs/f2fs/node.c >>>>>> @@ -1520,8 +1520,15 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted, >>>>>> >>>>>> trace_f2fs_writepage(page, NODE); >>>>>> >>>>>> - if (unlikely(f2fs_cp_error(sbi))) >>>>>> + if (unlikely(f2fs_cp_error(sbi))) { >>>>>> + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { >>>>>> + ClearPageUptodate(page); >>>>>> + dec_page_count(sbi, F2FS_DIRTY_NODES); >>>>>> + unlock_page(page); >>>>>> + return 0; >>>>>> + } >>>>>> goto redirty_out; >>>>>> + } >>>>>> >>>>>> if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) >>>>>> goto redirty_out; >>>>>> >>>> . >>>> >>> >>> >>> _______________________________________________ >>> Linux-f2fs-devel mailing list >>> Linux-f2fs-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >>> . >>> > . > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error 2020-05-27 2:35 ` Chao Yu @ 2020-05-27 20:56 ` Jaegeuk Kim 2020-05-28 1:20 ` Chao Yu 0 siblings, 1 reply; 12+ messages in thread From: Jaegeuk Kim @ 2020-05-27 20:56 UTC (permalink / raw) To: Chao Yu; +Cc: kernel-team, linux-kernel, linux-f2fs-devel On 05/27, Chao Yu wrote: > On 2020/5/26 9:56, Jaegeuk Kim wrote: > > On 05/26, Chao Yu wrote: > >> On 2020/5/26 9:11, Chao Yu wrote: > >>> On 2020/5/25 23:06, Jaegeuk Kim wrote: > >>>> On 05/25, Chao Yu wrote: > >>>>> On 2020/5/25 11:56, Jaegeuk Kim wrote: > >>>>>> Shutdown test is somtimes hung, since it keeps trying to flush dirty node pages > > 71.07% 0.01% kworker/u256:1+ [kernel.kallsyms] [k] wb_writeback > | > --71.06%--wb_writeback > | > |--68.96%--__writeback_inodes_wb > | | > | --68.95%--writeback_sb_inodes > | | > | |--65.08%--__writeback_single_inode > | | | > | | --64.35%--do_writepages > | | | > | | |--59.83%--f2fs_write_node_pages > | | | | > | | | --59.74%--f2fs_sync_node_pages > | | | | > | | | |--27.91%--pagevec_lookup_range_tag > | | | | | > | | | | --27.90%--find_get_pages_range_tag > > Before umount, kworker will always hold one core, that looks not reasonable, > to avoid that, could we just allow node write, since it's out-place-update, > and cp is not allowed, we don't need to worry about its effect on data on > previous checkpoint, and it can decrease memory footprint cost by node pages. It can cause some roll-forward recovery? > > Thanks, > > >>>>> > >>>>> IMO, for umount case, we should drop dirty reference and dirty pages on meta/data > >>>>> pages like we change for node pages to avoid potential dead loop... > >>>> > >>>> I believe we're doing for them. :P > >>> > >>> Actually, I mean do we need to drop dirty meta/data pages explicitly as below: > >>> > >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > >>> index 3dc3ac6fe143..4c08fd0a680a 100644 > >>> --- a/fs/f2fs/checkpoint.c > >>> +++ b/fs/f2fs/checkpoint.c > >>> @@ -299,8 +299,15 @@ static int __f2fs_write_meta_page(struct page *page, > >>> > >>> trace_f2fs_writepage(page, META); > >>> > >>> - if (unlikely(f2fs_cp_error(sbi))) > >>> + if (unlikely(f2fs_cp_error(sbi))) { > >>> + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { > >>> + ClearPageUptodate(page); > >>> + dec_page_count(sbi, F2FS_DIRTY_META); > >>> + unlock_page(page); > >>> + return 0; > >>> + } > >>> goto redirty_out; > >>> + } > >>> if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) > >>> goto redirty_out; > >>> if (wbc->for_reclaim && page->index < GET_SUM_BLOCK(sbi, 0)) > >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > >>> index 48a622b95b76..94b342802513 100644 > >>> --- a/fs/f2fs/data.c > >>> +++ b/fs/f2fs/data.c > >>> @@ -2682,6 +2682,12 @@ int f2fs_write_single_data_page(struct page *page, int *submitted, > >>> > >>> /* we should bypass data pages to proceed the kworkder jobs */ > >>> if (unlikely(f2fs_cp_error(sbi))) { > >>> + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { > >>> + ClearPageUptodate(page); > >>> + inode_dec_dirty_pages(inode); > >>> + unlock_page(page); > >>> + return 0; > >>> + } > >> > >> Oh, I notice previously, we will drop non-directory inode's dirty pages directly, > >> however, during umount, we'd better drop directory inode's dirty pages as well, right? > > > > Hmm, I remember I dropped them before. Need to double check. > > > >> > >>> mapping_set_error(page->mapping, -EIO); > >>> /* > >>> * don't drop any dirty dentry pages for keeping lastest > >>> > >>>> > >>>>> > >>>>> Thanks, > >>>>> > >>>>>> in an inifinite loop. Let's drop dirty pages at umount in that case. > >>>>>> > >>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > >>>>>> --- > >>>>>> v3: > >>>>>> - fix wrong unlock > >>>>>> > >>>>>> v2: > >>>>>> - fix typos > >>>>>> > >>>>>> fs/f2fs/node.c | 9 ++++++++- > >>>>>> 1 file changed, 8 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > >>>>>> index e632de10aedab..e0bb0f7e0506e 100644 > >>>>>> --- a/fs/f2fs/node.c > >>>>>> +++ b/fs/f2fs/node.c > >>>>>> @@ -1520,8 +1520,15 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted, > >>>>>> > >>>>>> trace_f2fs_writepage(page, NODE); > >>>>>> > >>>>>> - if (unlikely(f2fs_cp_error(sbi))) > >>>>>> + if (unlikely(f2fs_cp_error(sbi))) { > >>>>>> + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { > >>>>>> + ClearPageUptodate(page); > >>>>>> + dec_page_count(sbi, F2FS_DIRTY_NODES); > >>>>>> + unlock_page(page); > >>>>>> + return 0; > >>>>>> + } > >>>>>> goto redirty_out; > >>>>>> + } > >>>>>> > >>>>>> if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) > >>>>>> goto redirty_out; > >>>>>> > >>>> . > >>>> > >>> > >>> > >>> _______________________________________________ > >>> Linux-f2fs-devel mailing list > >>> Linux-f2fs-devel@lists.sourceforge.net > >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > >>> . > >>> > > . > > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error 2020-05-27 20:56 ` Jaegeuk Kim @ 2020-05-28 1:20 ` Chao Yu 0 siblings, 0 replies; 12+ messages in thread From: Chao Yu @ 2020-05-28 1:20 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: kernel-team, linux-kernel, linux-f2fs-devel On 2020/5/28 4:56, Jaegeuk Kim wrote: > On 05/27, Chao Yu wrote: >> On 2020/5/26 9:56, Jaegeuk Kim wrote: >>> On 05/26, Chao Yu wrote: >>>> On 2020/5/26 9:11, Chao Yu wrote: >>>>> On 2020/5/25 23:06, Jaegeuk Kim wrote: >>>>>> On 05/25, Chao Yu wrote: >>>>>>> On 2020/5/25 11:56, Jaegeuk Kim wrote: >>>>>>>> Shutdown test is somtimes hung, since it keeps trying to flush dirty node pages >> >> 71.07% 0.01% kworker/u256:1+ [kernel.kallsyms] [k] wb_writeback >> | >> --71.06%--wb_writeback >> | >> |--68.96%--__writeback_inodes_wb >> | | >> | --68.95%--writeback_sb_inodes >> | | >> | |--65.08%--__writeback_single_inode >> | | | >> | | --64.35%--do_writepages >> | | | >> | | |--59.83%--f2fs_write_node_pages >> | | | | >> | | | --59.74%--f2fs_sync_node_pages >> | | | | >> | | | |--27.91%--pagevec_lookup_range_tag >> | | | | | >> | | | | --27.90%--find_get_pages_range_tag >> >> Before umount, kworker will always hold one core, that looks not reasonable, >> to avoid that, could we just allow node write, since it's out-place-update, >> and cp is not allowed, we don't need to worry about its effect on data on >> previous checkpoint, and it can decrease memory footprint cost by node pages. > > It can cause some roll-forward recovery? Yup, recovery should be considered, Later fsync() will fail due to: int f2fs_sync_file(struct file *file, loff_t start, loff_t end, int datasync) { if (unlikely(f2fs_cp_error(F2FS_I_SB(file_inode(file))))) return -EIO; And we need to adjust f2fs_fsync_node_pages() to handle in-process fsyncing node pages as well: if (f2fs_cp_error()) { set_fsync_mark(page, 0); set_dentry_mark(page, 0); if (atomic) { unlock & put page; ret = -EIO; break; } } ret = __write_node_page(); Thanks, > >> >> Thanks, >> >>>>>>> >>>>>>> IMO, for umount case, we should drop dirty reference and dirty pages on meta/data >>>>>>> pages like we change for node pages to avoid potential dead loop... >>>>>> >>>>>> I believe we're doing for them. :P >>>>> >>>>> Actually, I mean do we need to drop dirty meta/data pages explicitly as below: >>>>> >>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c >>>>> index 3dc3ac6fe143..4c08fd0a680a 100644 >>>>> --- a/fs/f2fs/checkpoint.c >>>>> +++ b/fs/f2fs/checkpoint.c >>>>> @@ -299,8 +299,15 @@ static int __f2fs_write_meta_page(struct page *page, >>>>> >>>>> trace_f2fs_writepage(page, META); >>>>> >>>>> - if (unlikely(f2fs_cp_error(sbi))) >>>>> + if (unlikely(f2fs_cp_error(sbi))) { >>>>> + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { >>>>> + ClearPageUptodate(page); >>>>> + dec_page_count(sbi, F2FS_DIRTY_META); >>>>> + unlock_page(page); >>>>> + return 0; >>>>> + } >>>>> goto redirty_out; >>>>> + } >>>>> if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) >>>>> goto redirty_out; >>>>> if (wbc->for_reclaim && page->index < GET_SUM_BLOCK(sbi, 0)) >>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>>>> index 48a622b95b76..94b342802513 100644 >>>>> --- a/fs/f2fs/data.c >>>>> +++ b/fs/f2fs/data.c >>>>> @@ -2682,6 +2682,12 @@ int f2fs_write_single_data_page(struct page *page, int *submitted, >>>>> >>>>> /* we should bypass data pages to proceed the kworkder jobs */ >>>>> if (unlikely(f2fs_cp_error(sbi))) { >>>>> + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { >>>>> + ClearPageUptodate(page); >>>>> + inode_dec_dirty_pages(inode); >>>>> + unlock_page(page); >>>>> + return 0; >>>>> + } >>>> >>>> Oh, I notice previously, we will drop non-directory inode's dirty pages directly, >>>> however, during umount, we'd better drop directory inode's dirty pages as well, right? >>> >>> Hmm, I remember I dropped them before. Need to double check. >>> >>>> >>>>> mapping_set_error(page->mapping, -EIO); >>>>> /* >>>>> * don't drop any dirty dentry pages for keeping lastest >>>>> >>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>>> in an inifinite loop. Let's drop dirty pages at umount in that case. >>>>>>>> >>>>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>>>>>>> --- >>>>>>>> v3: >>>>>>>> - fix wrong unlock >>>>>>>> >>>>>>>> v2: >>>>>>>> - fix typos >>>>>>>> >>>>>>>> fs/f2fs/node.c | 9 ++++++++- >>>>>>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c >>>>>>>> index e632de10aedab..e0bb0f7e0506e 100644 >>>>>>>> --- a/fs/f2fs/node.c >>>>>>>> +++ b/fs/f2fs/node.c >>>>>>>> @@ -1520,8 +1520,15 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted, >>>>>>>> >>>>>>>> trace_f2fs_writepage(page, NODE); >>>>>>>> >>>>>>>> - if (unlikely(f2fs_cp_error(sbi))) >>>>>>>> + if (unlikely(f2fs_cp_error(sbi))) { >>>>>>>> + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { >>>>>>>> + ClearPageUptodate(page); >>>>>>>> + dec_page_count(sbi, F2FS_DIRTY_NODES); >>>>>>>> + unlock_page(page); >>>>>>>> + return 0; >>>>>>>> + } >>>>>>>> goto redirty_out; >>>>>>>> + } >>>>>>>> >>>>>>>> if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) >>>>>>>> goto redirty_out; >>>>>>>> >>>>>> . >>>>>> >>>>> >>>>> >>>>> _______________________________________________ >>>>> Linux-f2fs-devel mailing list >>>>> Linux-f2fs-devel@lists.sourceforge.net >>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >>>>> . >>>>> >>> . >>> > . > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error 2020-05-22 14:47 [f2fs-dev] [PATCH] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error Jaegeuk Kim 2020-05-22 23:32 ` Jaegeuk Kim @ 2020-05-25 2:16 ` Chao Yu 1 sibling, 0 replies; 12+ messages in thread From: Chao Yu @ 2020-05-25 2:16 UTC (permalink / raw) To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel, kernel-team On 2020/5/22 22:47, Jaegeuk Kim wrote: > Shutdown test is somtime hung, since dirty node pages weren't flushed out. > Let's drop dirty pages at umount after shutdown. > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > fs/f2fs/node.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > index e632de10aedab..8c63964a82fd0 100644 > --- a/fs/f2fs/node.c > +++ b/fs/f2fs/node.c > @@ -1520,8 +1520,15 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted, > > trace_f2fs_writepage(page, NODE); > > - if (unlikely(f2fs_cp_error(sbi))) > + if (unlikely(f2fs_cp_error(sbi))) { > + if (is_sbi_flag_set(sbi, SBI_IS_CLOSE)) { > + dec_page_count(sbi, F2FS_DIRTY_NODES); > + up_read(&sbi->node_write); We don't need to release node_write lock. > + unlock_page(page); > + return 0; > + } > goto redirty_out; > + } > > if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) > goto redirty_out; > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-05-28 1:20 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-22 14:47 [f2fs-dev] [PATCH] f2fs: avoid inifinite loop to wait for flushing node pages at cp_error Jaegeuk Kim 2020-05-22 23:32 ` Jaegeuk Kim 2020-05-25 3:56 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim 2020-05-25 6:30 ` Chao Yu 2020-05-25 15:06 ` Jaegeuk Kim 2020-05-26 1:11 ` Chao Yu 2020-05-26 1:34 ` Chao Yu 2020-05-26 1:56 ` Jaegeuk Kim 2020-05-27 2:35 ` Chao Yu 2020-05-27 20:56 ` Jaegeuk Kim 2020-05-28 1:20 ` Chao Yu 2020-05-25 2:16 ` [f2fs-dev] [PATCH] " 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).