From: Jaegeuk Kim <jaegeuk@kernel.org> To: Chao Yu <chao@kernel.org> Cc: linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, Zhiguo Niu <zhiguo.niu@unisoc.com>, Jing Xia <jing.xia@unisoc.com> Subject: Re: [PATCH] f2fs: fix to avoid potential deadlock Date: Thu, 27 Jan 2022 13:59:35 -0800 [thread overview] Message-ID: <YfMVxzdhat01ca7m@google.com> (raw) In-Reply-To: <20220127054449.24711-1-chao@kernel.org> On 01/27, Chao Yu wrote: > Quoted from Jing Xia's report, there is a potential deadlock may happen > between kworker and checkpoint as below: > > [T:writeback] [T:checkpoint] > - wb_writeback > - blk_start_plug > bio contains NodeA was plugged in writeback threads I'm still trying to understand more precisely. So, how is it possible to have bio having node write in this current context? > - 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 > - inode_dec_dirty_pages > - 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 > - blk_finish_plug > > Let's try to avoid deadlock condition by forcing unplugging previous bio via > blk_finish_plug(current->plug) once we'v skipped writeback in writepages() > due to valid sbi->wb_sync_req[DATA/NODE]. > > Fixes: 687de7f1010c ("f2fs: avoid IO split due to mixed WB_SYNC_ALL and WB_SYNC_NONE") > Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com> > Signed-off-by: Jing Xia <jing.xia@unisoc.com> > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/data.c | 6 +++++- > fs/f2fs/node.c | 6 +++++- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 76d6fe7b0c8f..932a4c81acaf 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -3174,8 +3174,12 @@ static int __f2fs_write_data_pages(struct address_space *mapping, > /* to avoid spliting IOs due to mixed WB_SYNC_ALL and WB_SYNC_NONE */ > if (wbc->sync_mode == WB_SYNC_ALL) > atomic_inc(&sbi->wb_sync_req[DATA]); > - else if (atomic_read(&sbi->wb_sync_req[DATA])) > + else if (atomic_read(&sbi->wb_sync_req[DATA])) { > + /* to avoid potential deadlock */ > + if (current->plug) > + blk_finish_plug(current->plug); > goto skip_write; > + } > > if (__should_serialize_io(inode, wbc)) { > mutex_lock(&sbi->writepages); > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > index 556fcd8457f3..69c6bcaf5aae 100644 > --- a/fs/f2fs/node.c > +++ b/fs/f2fs/node.c > @@ -2106,8 +2106,12 @@ static int f2fs_write_node_pages(struct address_space *mapping, > > if (wbc->sync_mode == WB_SYNC_ALL) > atomic_inc(&sbi->wb_sync_req[NODE]); > - else if (atomic_read(&sbi->wb_sync_req[NODE])) > + else if (atomic_read(&sbi->wb_sync_req[NODE])) { > + /* to avoid potential deadlock */ > + if (current->plug) > + blk_finish_plug(current->plug); > goto skip_write; > + } > > trace_f2fs_writepages(mapping->host, wbc, NODE); > > -- > 2.32.0
WARNING: multiple messages have this Message-ID (diff)
From: Jaegeuk Kim <jaegeuk@kernel.org> To: Chao Yu <chao@kernel.org> Cc: Jing Xia <jing.xia@unisoc.com>, Zhiguo Niu <zhiguo.niu@unisoc.com>, linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to avoid potential deadlock Date: Thu, 27 Jan 2022 13:59:35 -0800 [thread overview] Message-ID: <YfMVxzdhat01ca7m@google.com> (raw) In-Reply-To: <20220127054449.24711-1-chao@kernel.org> On 01/27, Chao Yu wrote: > Quoted from Jing Xia's report, there is a potential deadlock may happen > between kworker and checkpoint as below: > > [T:writeback] [T:checkpoint] > - wb_writeback > - blk_start_plug > bio contains NodeA was plugged in writeback threads I'm still trying to understand more precisely. So, how is it possible to have bio having node write in this current context? > - 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 > - inode_dec_dirty_pages > - 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 > - blk_finish_plug > > Let's try to avoid deadlock condition by forcing unplugging previous bio via > blk_finish_plug(current->plug) once we'v skipped writeback in writepages() > due to valid sbi->wb_sync_req[DATA/NODE]. > > Fixes: 687de7f1010c ("f2fs: avoid IO split due to mixed WB_SYNC_ALL and WB_SYNC_NONE") > Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com> > Signed-off-by: Jing Xia <jing.xia@unisoc.com> > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/data.c | 6 +++++- > fs/f2fs/node.c | 6 +++++- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 76d6fe7b0c8f..932a4c81acaf 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -3174,8 +3174,12 @@ static int __f2fs_write_data_pages(struct address_space *mapping, > /* to avoid spliting IOs due to mixed WB_SYNC_ALL and WB_SYNC_NONE */ > if (wbc->sync_mode == WB_SYNC_ALL) > atomic_inc(&sbi->wb_sync_req[DATA]); > - else if (atomic_read(&sbi->wb_sync_req[DATA])) > + else if (atomic_read(&sbi->wb_sync_req[DATA])) { > + /* to avoid potential deadlock */ > + if (current->plug) > + blk_finish_plug(current->plug); > goto skip_write; > + } > > if (__should_serialize_io(inode, wbc)) { > mutex_lock(&sbi->writepages); > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > index 556fcd8457f3..69c6bcaf5aae 100644 > --- a/fs/f2fs/node.c > +++ b/fs/f2fs/node.c > @@ -2106,8 +2106,12 @@ static int f2fs_write_node_pages(struct address_space *mapping, > > if (wbc->sync_mode == WB_SYNC_ALL) > atomic_inc(&sbi->wb_sync_req[NODE]); > - else if (atomic_read(&sbi->wb_sync_req[NODE])) > + else if (atomic_read(&sbi->wb_sync_req[NODE])) { > + /* to avoid potential deadlock */ > + if (current->plug) > + blk_finish_plug(current->plug); > goto skip_write; > + } > > trace_f2fs_writepages(mapping->host, wbc, NODE); > > -- > 2.32.0 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2022-01-27 21:59 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-01-27 5:44 [PATCH] f2fs: fix to avoid potential deadlock Chao Yu 2022-01-27 5:44 ` [f2fs-dev] " Chao Yu 2022-01-27 21:59 ` Jaegeuk Kim [this message] 2022-01-27 21:59 ` Jaegeuk Kim 2022-01-28 1:43 ` Chao Yu 2022-01-28 1:43 ` [f2fs-dev] " Chao Yu 2022-01-29 0:37 ` Jaegeuk Kim 2022-01-29 0:37 ` [f2fs-dev] " Jaegeuk Kim 2022-01-29 1:48 ` Chao Yu 2022-01-29 1:48 ` [f2fs-dev] " Chao Yu 2022-02-03 1:51 ` Jaegeuk Kim 2022-02-03 1:51 ` [f2fs-dev] " Jaegeuk Kim 2022-02-03 14:57 ` Chao Yu 2022-02-03 14:57 ` [f2fs-dev] " Chao Yu 2022-02-25 3:02 ` Chao Yu 2022-02-25 3:02 ` Chao Yu 2022-03-02 3:32 ` Chao Yu 2022-03-02 3:32 ` Chao Yu 2022-03-02 5:26 ` Jaegeuk Kim 2022-03-02 5:26 ` Jaegeuk Kim 2022-03-02 8:14 ` Chao Yu 2022-03-02 8:14 ` Chao Yu 2022-03-02 19:45 ` Jaegeuk Kim 2022-03-02 19:45 ` Jaegeuk Kim 2022-03-03 2:32 ` Chao Yu 2022-03-03 2:32 ` Chao Yu 2022-03-03 21:30 ` Jaegeuk Kim 2022-03-03 21:30 ` Jaegeuk Kim 2022-12-16 15:50 Chao Yu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YfMVxzdhat01ca7m@google.com \ --to=jaegeuk@kernel.org \ --cc=chao@kernel.org \ --cc=jing.xia@unisoc.com \ --cc=linux-f2fs-devel@lists.sourceforge.net \ --cc=linux-kernel@vger.kernel.org \ --cc=zhiguo.niu@unisoc.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.