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
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
