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 <stumm...@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
> .
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
