Re: [PATCH] f2fs: fix the panic in do_checkpoint()

From: Sahitya Tummala
Date: Sun Feb 16 2020 - 22:09:48 EST

Next message: Joakim Zhang: "RE: [PATCH v4] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events"
Previous message: Anson Huang: "[PATCH] clk: imx8mn: Fix incorrect clock defines"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Feb 14, 2020 at 03:16:59PM +0800, Chao Yu wrote:
> Hi Sahitya,
>
> On 2020/2/14 11:54, Sahitya Tummala wrote:
> > Hi Chao,
> >
> > On Fri, Feb 14, 2020 at 10:26:18AM +0800, Chao Yu wrote:
> >> Hi Sahitya,
> >>
> >> On 2020/2/12 18:34, Sahitya Tummala wrote:
> >>> There could be a scenario where f2fs_sync_meta_pages() will not
> >>> ensure that all F2FS_DIRTY_META pages are submitted for IO. Thus,
> >>> resulting in the below panic in do_checkpoint() -
> >>>
> >>> f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> >>> !f2fs_cp_error(sbi));
> >>>
> >>> This can happen in a low-memory condition, where shrinker could
> >>> also be doing the writepage operation (stack shown below)
> >>> at the same time when checkpoint is running on another core.
> >>>
> >>> schedule
> >>> down_write
> >>> f2fs_submit_page_write -> by this time, this page in page cache is tagged
> >>> as PAGECACHE_TAG_WRITEBACK and PAGECACHE_TAG_DIRTY
> >>> is cleared, due to which f2fs_sync_meta_pages()
> >>> cannot sync this page in do_checkpoint() path.
> >>> f2fs_do_write_meta_page
> >>> __f2fs_write_meta_page
> >>> f2fs_write_meta_page
> >>> shrink_page_list
> >>> shrink_inactive_list
> >>> shrink_node_memcg
> >>> shrink_node
> >>> kswapd
> >>
> >> IMO, there may be one more simple fix here:
> >>
> >> - f2fs_do_write_meta_page(sbi, page, io_type);
> >> dec_page_count(sbi, F2FS_DIRTY_META);
> >>
> >> + f2fs_do_write_meta_page(sbi, page, io_type);
> >>
> >> If we can remove F2FS_DIRTY_META reference count before we clear
> >> PAGECACHE_TAG_DIRTY, we can avoid this race condition.
> >>
> >> - dec_page_count(sbi, F2FS_DIRTY_META);
> >> - f2fs_do_write_meta_page
> >> - set_page_writeback
> >> - __test_set_page_writeback
> >> - xas_clear_mark(&xas, PAGECACHE_TAG_DIRTY);
> >>
> >> Thoughts?
> >
> > I believe it will be a problem because let's say the shrinker path is
> > still in the below stack -
> >
> > f2fs_submit_page_write();
> > ->down_write(&io->io_rwsem);
> >
> > Then, the current f2fs_wait_on_all_pages_writeback() will not wait for
> > that page as it is not yet tagged as F2FS_WB_CP_DATA. Hence, the checkpoint
> > may proceed without waiting for this meta page to be written to disk.
>
> Oh, you're right.
>
> It looks the race can happen for data/node pages? like in fsync() procedure.
>
Yes, it can happen. I see that you have already put a patch to fix that :).

> >
> >>
> >>>
> >>> Signed-off-by: Sahitya Tummala <stummala@xxxxxxxxxxxxxx>
> >>> ---
> >>> fs/f2fs/checkpoint.c | 16 ++++++++--------
> >>> fs/f2fs/f2fs.h | 2 +-
> >>> fs/f2fs/super.c | 2 +-
> >>> 3 files changed, 10 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>> index ffdaba0..2b651a3 100644
> >>> --- a/fs/f2fs/checkpoint.c
> >>> +++ b/fs/f2fs/checkpoint.c
> >>> @@ -1250,14 +1250,14 @@ static void unblock_operations(struct f2fs_sb_info *sbi)
> >>> f2fs_unlock_all(sbi);
> >>> }
> >>>
> >>> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi)
> >>> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type)
> >>> {
> >>> DEFINE_WAIT(wait);
> >>>
> >>> for (;;) {
> >>> prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);
> >>>
> >>> - if (!get_pages(sbi, F2FS_WB_CP_DATA))
> >>> + if (!get_pages(sbi, type))
> >>> break;
> >>>
> >>> if (unlikely(f2fs_cp_error(sbi)))
> >>> @@ -1384,8 +1384,8 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>
> >>> /* Flush all the NAT/SIT pages */
> >>> f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
> >>> - f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> >>> - !f2fs_cp_error(sbi));
> >>> + /* Wait for all dirty meta pages to be submitted for IO */
> >>> + f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
> >>
> >> I'm afraid calling f2fs_wait_on_all_pages() after we call
> >> f2fs_sync_meta_pages() is low efficient, as we only want to write out
> >> dirty meta pages instead of wait for writebacking them to device cache.
> >>
> >
> > I have modified the existing function f2fs_wait_on_all_pages_writeback() to
> > a generic one f2fs_wait_on_all_pages(), where it will wait according to the
> > requested type. In this case, it will only wait for dirty F2FS_DIRTY_META pages
> > but not for the writeback to device cache.
>
> Oh, I see, as last dirty reference count decreaser won't wake up waiter, we need
> to wait for timeout, right? Can we decrease timeout count, maybe HZ/50 as we did
> for congestion.

Yes, I agree that it is better to decrease the timeout to HZ/50.

Thanks,

>
> io_schedule_timeout(5*HZ);
>
> Thanks,
>
> >
> > Thanks,
> >
> >> Thanks,
> >>
> >>>
> >>> /*
> >>> * modify checkpoint
> >>> @@ -1493,11 +1493,11 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>
> >>> /* Here, we have one bio having CP pack except cp pack 2 page */
> >>> f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
> >>> - f2fs_bug_on(sbi, get_pages(sbi, F2FS_DIRTY_META) &&
> >>> - !f2fs_cp_error(sbi));
> >>> + /* Wait for all dirty meta pages to be submitted for IO */
> >>> + f2fs_wait_on_all_pages(sbi, F2FS_DIRTY_META);
> >>>
> >>> /* wait for previous submitted meta pages writeback */
> >>> - f2fs_wait_on_all_pages_writeback(sbi);
> >>> + f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
> >>>
> >>> /* flush all device cache */
> >>> err = f2fs_flush_device_cache(sbi);
> >>> @@ -1506,7 +1506,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>
> >>> /* barrier and flush checkpoint cp pack 2 page if it can */
> >>> commit_checkpoint(sbi, ckpt, start_blk);
> >>> - f2fs_wait_on_all_pages_writeback(sbi);
> >>> + f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
> >>>
> >>> /*
> >>> * invalidate intermediate page cache borrowed from meta inode
> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >>> index 5a888a0..b0e0535 100644
> >>> --- a/fs/f2fs/f2fs.h
> >>> +++ b/fs/f2fs/f2fs.h
> >>> @@ -3196,7 +3196,7 @@ bool f2fs_is_dirty_device(struct f2fs_sb_info *sbi, nid_t ino,
> >>> void f2fs_update_dirty_page(struct inode *inode, struct page *page);
> >>> void f2fs_remove_dirty_inode(struct inode *inode);
> >>> int f2fs_sync_dirty_inodes(struct f2fs_sb_info *sbi, enum inode_type type);
> >>> -void f2fs_wait_on_all_pages_writeback(struct f2fs_sb_info *sbi);
> >>> +void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type);
> >>> int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc);
> >>> void f2fs_init_ino_entry_info(struct f2fs_sb_info *sbi);
> >>> int __init f2fs_create_checkpoint_caches(void);
> >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >>> index 5111e1f..084633b 100644
> >>> --- a/fs/f2fs/super.c
> >>> +++ b/fs/f2fs/super.c
> >>> @@ -1105,7 +1105,7 @@ static void f2fs_put_super(struct super_block *sb)
> >>> /* our cp_error case, we can wait for any writeback page */
> >>> f2fs_flush_merged_writes(sbi);
> >>>
> >>> - f2fs_wait_on_all_pages_writeback(sbi);
> >>> + f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);
> >>>
> >>> f2fs_bug_on(sbi, sbi->fsync_node_num);
> >>>
> >>>
> >

--
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Next message: Joakim Zhang: "RE: [PATCH v4] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events"
Previous message: Anson Huang: "[PATCH] clk: imx8mn: Fix incorrect clock defines"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]