by Chao Yu

[permalink] [raw]

Subject: Re: [f2fs-dev] [PATCH Vx 1/1] f2fs: Avoid deadlock between writeback and checkpoint

On 2021/12/18 0:59, Jaegeuk Kim wrote:
> On 12/17, Chao Yu wrote:
>> On 2021/11/20 9:50, Chao Yu wrote:
>>> On 2021/11/18 14:46, Chao Yu wrote:
>>>> On 2021/11/18 0:56, Jaegeuk Kim wrote:
>>>>> On 11/09, [email protected] wrote:
>>>>>> From: Zhiguo Niu <[email protected]>
>>>>>>
>>>>>> There could be a scenario as following:
>>>>>> The inodeA and inodeB are in b_io queue of writeback
>>>>>> inodeA : f2fs's node inode
>>>>>> inodeB : a dir inode with only one dirty pages, and the node page
>>>>>> of inodeB cached into inodeA
>>>>>>
>>>>>> writeback:
>>>>>>
>>>>>> wb_workfn
>>>>>> wb_writeback
>>>>>> blk_start_plug
>>>>>> loop {
>>>>>> queue_io
>>>>>> progress=__writeback_inodes_wb
>>>>>> __writeback_single_inode
>>>>>> do_writepages
>>>>>> f2fs_write_data_pages
>>>>>> wbc->pages_skipped +=get_dirty_pages
>>>>>> inode->i_state &= ~dirty
>>>>>> wrote++
>>>>>> requeue_inode
>>>>>> }
>>>>>> blk_finish_plug
>>>>>>
>>>>>> checkpoint:
>>>>>>
>>>>>> f2fs_write_checkpoint
>>>>>> f2fs_sync_dirty_inodes
>>>>>> filemap_fdatawrite
>>>>>> do_writepages
>>>>>> f2fs_write_data_pages
>>>>>> f2fs_write_single_data_page
>>>>>> f2fs_do_write_data_page
>>>>>> set_page_writeback
>>>>>> f2fs_outplace_write_data
>>>>>> f2fs_update_data_blkaddr
>>>>>> f2fs_wait_on_page_writeback
>>>>>> inode_dec_dirty_pages
>>>>>>
>>>>>> 1. Writeback thread flush inodeA, and push it's bio request in task's plug;
>>>>>> 2. Checkpoint thread writes inodeB's dirty page, and then wait its node
>>>>>> page writeback cached into inodeA which is in writeback task's plug
>>>>>> 3. Writeback thread flush inodeB and skip writing the dirty page as
>>>>>> wb_sync_req[DATA] > 0.
>>>>>> 4. As none of the inodeB's page is marked as PAGECACHE_TAG_DIRTY, writeback
>>>>>> thread clear inodeB's dirty state.
>>>>>> 5. Then inodeB is moved from b_io to b_dirty because of pages_skipped > 0
>>>>>> as checkpoint thread is stuck before dec dirty_pages.
>>>>>>
>>>>>> This patch collect correct pages_skipped according to the tag state in
>>>>>> page tree of inode
>>>>>>
>>>>>> Signed-off-by: Zhiguo Niu <[email protected]>
>>>>>> Signed-off-by: Jing Xia <[email protected]>
>>>>>> ---
>>>>>> fs/f2fs/data.c | 4 +++-
>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>>> index f4fd6c246c9a..e98628e3868c 100644
>>>>>> --- a/fs/f2fs/data.c
>>>>>> +++ b/fs/f2fs/data.c
>>>>>> @@ -3237,7 +3237,9 @@ static int __f2fs_write_data_pages(struct address_space *mapping,
>>>>>> return ret;
>>>>>> skip_write:
>>>>>> - wbc->pages_skipped += get_dirty_pages(inode);
>>>>>> + wbc->pages_skipped +=
>>>>>> + mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY) ?
>>>>>
>>>>> Is there any race condition to get 0, if there's any dirty page? IOWs, it
>>>>
>>>> Quoted from Jing Xia's explanation:
>>>>
>>>> [T:writeback] [T:checkpoint]
>>>
>>> My bad, [1] should be here:
>>>
>>> bio contains NodeA was plugged in writeback threads
>>>
>>> Thanks,
>>>
>>>> - do_writepages -- sync write inodeB, inc wb_sync_req[DATA]
>>>> - f2fs_write_data_pages
>>>> - f2fs_write_single_data_page -- write last dirty page
>>>> - f2fs_do_write_data_page
>>>> - set_page_writeback -- clear page dirty flag and
>>>> PAGECACHE_TAG_DIRTY tag in radix tree
>>>> - f2fs_outplace_write_data
>>>> - f2fs_update_data_blkaddr
>>>> - f2fs_wait_on_page_writeback -- wait NodeA to writeback here
>>
>> Jaegeuk,
>>
>> checkpoint() will stuck on waiting NodeA to writeback.
>>
>>>> - inode_dec_dirty_pages
>>>
>>>> bio contains NodeA was plugged in writeback threads
>>>
>>> [1]
>>>
>>> Thanks,
>>>
>>>> - writeback_sb_inodes
>>>> - writeback_single_inode
>>>> - do_writepages
>>>> - f2fs_write_data_pages -- skip writepages due to wb_sync_req[DATA]
>>>> - wbc->pages_skipped += get_dirty_pages() -- PAGECACHE_TAG_DIRTY is not set but get_dirty_pages() returns one
>>>> - requeue_inode -- requeue inode to wb->b_dirty queue due to non-zero.pages_skipped
>>
>> Meanwhile, kworker plugged NodeA, and then loop writebacking inode due to
>> inode will be requeued all the time.
>
> I'm thinking that this will be released after finishing node writes in a
> different context. Then, this loop can happen in that time period which
> is NOT forever. But, if we want to avoid this temporary loop, we may be

I doubt it may break due to below condition in writeback_sb_inodes():

if (need_resched()) {
/*
* We're trying to balance between building up a nice
* long list of IOs to improve our merge rate, and
* getting those IOs out quickly for anyone throttling
* in balance_dirty_pages(). cond_resched() doesn't
* unplug, so get our IOs out the door before we
* give up the CPU.
*/
if (current->plug)
blk_flush_plug(current->plug, false);
cond_resched();
}

But as Zhiguo Niu and Jing Xia reported, blk_flush_plug was not executed...

> able to decrease the dirty count before f2fs_update_data_blkaddr().

I guess there is one simple way, we can force flushing plugged bio via
blk_flush_plug(current->plug, false) once we skip writeback in writepages()
due to sbi->wb_sync_req[DATA/NODE] is valid.

Thoughts?

Thanks,

>
>>
>> Thanks,
>>
>>>>
>>>>> seems the current condition is just requeuing the inode as dirty, but next
>>>>> flushing time will remove it from dirty list. Is this giving too much overheads?
>>>>
>>>> I prefer to let writeback thread call blk_flush_plug() after skipping
>>>> writepages() due to wb_sync_req[DATA/NODE] check condition, thoughts?
>>>>
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index 9f754aaef558..b6e1ed73f8f5 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -3087,6 +3087,8 @@ static int f2fs_write_cache_pages(struct address_space *mapping,
>>>> /* give a priority to WB_SYNC threads */
>>>> if (atomic_read(&sbi->wb_sync_req[DATA]) &&
>>>> wbc->sync_mode == WB_SYNC_NONE) {
>>>> + if (current->plug)
>>>> + blk_flush_plug(current->plug, false);
>>>> done = 1;
>>>> break;
>>>> }
>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>>> index 556fcd8457f3..dd9a817d8dab 100644
>>>> --- a/fs/f2fs/node.c
>>>> +++ b/fs/f2fs/node.c
>>>> @@ -1946,6 +1946,8 @@ int f2fs_sync_node_pages(struct f2fs_sb_info *sbi,
>>>> if (atomic_read(&sbi->wb_sync_req[NODE]) &&
>>>> wbc->sync_mode == WB_SYNC_NONE) {
>>>> done = 1;
>>>> + if (current->plug)
>>>> + blk_flush_plug(current->plug, false);
>>>> break;
>>>> }
>>>>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>> + get_dirty_pages(inode) : 0;
>>>>>> trace_f2fs_writepages(mapping->host, wbc, DATA);
>>>>>> return 0;
>>>>>> }
>>>>>> --
>>>>>> 2.28.0
>>>>
>>>>
>>>> _______________________________________________
>>>> Linux-f2fs-devel mailing list
>>>> [email protected]
>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>>
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> [email protected]
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel