Bug 208565
| Summary: | There may be dead lock for cp_rwsem during checkpoint | ||
|---|---|---|---|
| Product: | File System | Reporter: | Zhiguo.Niu (Zhiguo.Niu) |
| Component: | f2fs | Assignee: | Default virtual assignee for f2fs (filesystem_f2fs) |
| Status: | CLOSED CODE_FIX | ||
| Severity: | high | CC: | chao, jaegeuk |
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | 4.14.181 | Subsystem: | |
| Regression: | No | Bisected commit-id: | |
|
Description
Zhiguo.Niu
2020-07-15 12:24:37 UTC
Thank you for the report.
I think this is valid point that we need to fix.
I'm testing a RFC patch like this. Thanks.
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1926,8 +1926,12 @@ int f2fs_sync_node_pages(struct f2fs_sb_info *sbi,
goto continue_unlock;
}
- /* flush inline_data, if it's async context. */
- if (do_balance && is_inline_node(page)) {
+ /* flush inline_data/inode, if it's async context. */
+ if (!do_balance)
+ goto write_node;
+
+ /* flush inline_data */
+ if (is_inline_node(page)) {
clear_inline_node(page);
unlock_page(page);
flush_inline_data(sbi, ino_of_node(page));
@@ -1940,7 +1944,7 @@ int f2fs_sync_node_pages(struct f2fs_sb_info *sbi,
if (flush_dirty_inode(page))
goto lock_node;
}
-
+write_node:
f2fs_wait_on_page_writeback(page, NODE, true, true);
if (!clear_page_dirty_for_io(page))
(In reply to Jaegeuk Kim from comment #1) > Thank you for the report. > > I think this is valid point that we need to fix. > I'm testing a RFC patch like this. Thanks. > Could you please check generic/204 testcase? as this fix diff skips codes added in commit 052a82d85a3b ("f2fs: fix to writeout dirty inode during node flush"). That passed. could you check your side? Passed, we should ask Eric to retest in his enviornment, to make sure there is actually no regression. (In reply to Jaegeuk Kim from comment #1) > Thank you for the report. > > I think this is valid point that we need to fix. > I'm testing a RFC patch like this. Thanks. > > --- a/fs/f2fs/node.c > +++ b/fs/f2fs/node.c > @@ -1926,8 +1926,12 @@ int f2fs_sync_node_pages(struct f2fs_sb_info *sbi, > goto continue_unlock; > } > > - /* flush inline_data, if it's async context. */ > - if (do_balance && is_inline_node(page)) { > + /* flush inline_data/inode, if it's async context. */ > + if (!do_balance) > + goto write_node; > + > + /* flush inline_data */ > + if (is_inline_node(page)) { > clear_inline_node(page); > unlock_page(page); > flush_inline_data(sbi, ino_of_node(page)); > @@ -1940,7 +1944,7 @@ int f2fs_sync_node_pages(struct f2fs_sb_info *sbi, > if (flush_dirty_inode(page)) > goto lock_node; > } > - > +write_node: > f2fs_wait_on_page_writeback(page, NODE, true, true); > > if (!clear_page_dirty_for_io(page)) Hi Jaegeuk Kim for comment 1, can you help provide a full diff for this patch, I can not apply it directly because of the following error: > fatal: patch fragment without header at line 3: @@ -1926,8 +1926,12 @@ int > f2fs_sync_node_pages(struct f2fs_sb_info *sbi, or Should I merge it by Manually? thanks a lot~ Hi Jaegeuk Kim Could I merge this patch in my code for fixing the deadlock issue? thanks! On 2020/7/17 1:15, bugzilla-daemon@bugzilla.kernel.org wrote: > https://bugzilla.kernel.org/show_bug.cgi?id=208565 > > Jaegeuk Kim (jaegeuk@kernel.org) changed: > > What |Removed |Added > ---------------------------------------------------------------------------- > CC| |jaegeuk@kernel.org > > --- Comment #1 from Jaegeuk Kim (jaegeuk@kernel.org) --- > Thank you for the report. > > I think this is valid point that we need to fix. > I'm testing a RFC patch like this. Thanks. Shouldn't we revert 34c061ad85a2 ("f2fs: Avoid double lock for cp_rwsem during checkpoint") at the same time? > > --- a/fs/f2fs/node.c > +++ b/fs/f2fs/node.c > @@ -1926,8 +1926,12 @@ int f2fs_sync_node_pages(struct f2fs_sb_info *sbi, > goto continue_unlock; > } > > - /* flush inline_data, if it's async context. */ > - if (do_balance && is_inline_node(page)) { > + /* flush inline_data/inode, if it's async context. */ > + if (!do_balance) > + goto write_node; > + > + /* flush inline_data */ > + if (is_inline_node(page)) { > clear_inline_node(page); > unlock_page(page); > flush_inline_data(sbi, ino_of_node(page)); > @@ -1940,7 +1944,7 @@ int f2fs_sync_node_pages(struct f2fs_sb_info *sbi, > if (flush_dirty_inode(page)) > goto lock_node; > } > - > +write_node: > f2fs_wait_on_page_writeback(page, NODE, true, true); > > if (!clear_page_dirty_for_io(page)) > Chao, why do we need to revert that patch? Zhiguo, You can see the patch here. https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=b0f3b87fb3abc42c81d76c6c5795f26dbdb2f04b On 2020/7/24 11:21, bugzilla-daemon@bugzilla.kernel.org wrote: > https://bugzilla.kernel.org/show_bug.cgi?id=208565 > > --- Comment #8 from Jaegeuk Kim (jaegeuk@kernel.org) --- > Chao, why do we need to revert that patch? After applying your new patch, below race condition should no longer happen, right? Thread A Thread B f2fs_write_checkpoint() - block_operations(sbi) - f2fs_lock_all(sbi); - down_write(&sbi->cp_rwsem); - open() - igrab() - write() write inline data - unlink() - f2fs_sync_node_pages() + if (!do_balance) + goto write_node; <---- this avoids running into iput(). - if (is_inline_node(page)) - flush_inline_data() - ilookup() page = f2fs_pagecache_get_page() if (!page) goto iput_out; iput_out: -close() -iput() iput(inode); - f2fs_evict_inode() - f2fs_truncate_blocks() - f2fs_lock_op() - down_read(&sbi->cp_rwsem); > > Zhiguo, > You can see the patch here. > > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=b0f3b87fb3abc42c81d76c6c5795f26dbdb2f04b > if we revert it, we're not going to call f2fs_flush_inline_data() in checkpoint path, which actually fixes generic/204? (In reply to Jaegeuk Kim from comment #10) > if we revert it, we're not going to call f2fs_flush_inline_data() in > checkpoint path, which actually fixes generic/204? Hi Jaeqeuk Kim, I still have a problem: As new patch modified, flush_dirty_inode will be skipped for avoiding dead lock, but when/where flush_dirty_inode would be done? thanks! It'll be called by node_inode->writepages() -> f2fs_write_node_pages(), which should be done by flusher in vfs. |