Messages in this thread | | | Subject | Re: [f2fs-dev] [PATCH] f2fs: fix the panic in do_checkpoint() | From | Chao Yu <> | Date | Fri, 14 Feb 2020 15:35:18 +0800 |
| |
On 2020/2/14 15:16, 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.
I mean something like this:
- fsync() - shrink - f2fs_do_sync_file - __write_node_page - set_page_writeback(page#0) : remove DIRTY/TOWRITE flag - f2fs_fsync_node_pages : won't find page #0 as TOWRITE flag was removeD - f2fs_wait_on_node_pages_writeback : wont' wait page #0 writeback as it was not in fsync_node_list list. - f2fs_add_fsync_node_entry
Thanks,
> >> >>> >>>> >>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org> >>>> --- >>>> 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. > > 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); >>>> >>>> >> > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > . >
| |