| Message ID | 20230831071011.56116-1-guochunhai@vivo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev,v2] f2fs: replace blk_finish_plug() with blk_flush_plug() | expand |
On 2023/8/31 15:10, Chunhai Guo wrote: > The commit 344150999b7f ("f2fs: fix to avoid potential deadlock") only > requires unplugging current->plug. Using blk_finish_plug() is unnecessary > as it sets current->plug as NULL and prevents wb_writeback() from using > plug in subsequent loops. Instead, use blk_flush_plug() as a replacement. > > Signed-off-by: Chunhai Guo <guochunhai@vivo.com> Reviewed-by: Chao Yu <chao@kernel.org> Thanks,
On 2023/8/31 15:10, Chunhai Guo wrote: > The commit 344150999b7f ("f2fs: fix to avoid potential deadlock") only > requires unplugging current->plug. Using blk_finish_plug() is unnecessary > as it sets current->plug as NULL and prevents wb_writeback() from using > plug in subsequent loops. Instead, use blk_flush_plug() as a replacement. > > Signed-off-by: Chunhai Guo <guochunhai@vivo.com> > --- > fs/f2fs/data.c | 3 +-- > fs/f2fs/node.c | 3 +-- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 916e317ac925..77b4a55d6d94 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -3346,8 +3346,7 @@ static int __f2fs_write_data_pages(struct address_space *mapping, > atomic_inc(&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); What about? if (current->plug) { struct blk_plug *plug = current->plug; blk_finish_plug(plug); blk_start_plug(plug); } Jaegeuk, thoughts? Thanks, > + blk_flush_plug(current->plug, false); > goto skip_write; > } > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > index ee2e1dd64f25..c4b5717a8c1a 100644 > --- a/fs/f2fs/node.c > +++ b/fs/f2fs/node.c > @@ -2126,8 +2126,7 @@ static int f2fs_write_node_pages(struct address_space *mapping, > atomic_inc(&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); > + blk_flush_plug(current->plug, false); > goto skip_write; > } >
On 09/04, Chao Yu wrote: > On 2023/8/31 15:10, Chunhai Guo wrote: > > The commit 344150999b7f ("f2fs: fix to avoid potential deadlock") only > > requires unplugging current->plug. Using blk_finish_plug() is unnecessary > > as it sets current->plug as NULL and prevents wb_writeback() from using > > plug in subsequent loops. Instead, use blk_flush_plug() as a replacement. > > > > Signed-off-by: Chunhai Guo <guochunhai@vivo.com> > > --- > > fs/f2fs/data.c | 3 +-- > > fs/f2fs/node.c | 3 +-- > > 2 files changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > index 916e317ac925..77b4a55d6d94 100644 > > --- a/fs/f2fs/data.c > > +++ b/fs/f2fs/data.c > > @@ -3346,8 +3346,7 @@ static int __f2fs_write_data_pages(struct address_space *mapping, > > atomic_inc(&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); > > What about? > > if (current->plug) { > struct blk_plug *plug = current->plug; > > blk_finish_plug(plug); > blk_start_plug(plug); > } > > Jaegeuk, thoughts? By the way, do we really need to reuse current->plug again? After checkpoint being done, we can use the plug in __f2fs_write_data_pages. Are there some numbers to show the benefit? > > Thanks, > > > + blk_flush_plug(current->plug, false); > > goto skip_write; > > } > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > > index ee2e1dd64f25..c4b5717a8c1a 100644 > > --- a/fs/f2fs/node.c > > +++ b/fs/f2fs/node.c > > @@ -2126,8 +2126,7 @@ static int f2fs_write_node_pages(struct address_space *mapping, > > atomic_inc(&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); > > + blk_flush_plug(current->plug, false); > > goto skip_write; > > }
On 2023/9/5 0:40, Jaegeuk Kim wrote: > On 09/04, Chao Yu wrote: >> On 2023/8/31 15:10, Chunhai Guo wrote: >>> The commit 344150999b7f ("f2fs: fix to avoid potential deadlock") only >>> requires unplugging current->plug. Using blk_finish_plug() is unnecessary >>> as it sets current->plug as NULL and prevents wb_writeback() from using >>> plug in subsequent loops. Instead, use blk_flush_plug() as a replacement. >>> >>> Signed-off-by: Chunhai Guo <guochunhai@vivo.com> >>> --- >>> fs/f2fs/data.c | 3 +-- >>> fs/f2fs/node.c | 3 +-- >>> 2 files changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>> index 916e317ac925..77b4a55d6d94 100644 >>> --- a/fs/f2fs/data.c >>> +++ b/fs/f2fs/data.c >>> @@ -3346,8 +3346,7 @@ static int __f2fs_write_data_pages(struct address_space *mapping, >>> atomic_inc(&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); >> >> What about? >> >> if (current->plug) { >> struct blk_plug *plug = current->plug; >> >> blk_finish_plug(plug); >> blk_start_plug(plug); >> } >> >> Jaegeuk, thoughts? > > By the way, do we really need to reuse current->plug again? After checkpoint > being done, we can use the plug in __f2fs_write_data_pages. Are there some > numbers to show the benefit? > I don't have a number on this, but I think that reusing current->plug can help maintain the original intention of wb_writeback(). Nevertheless, it can be a viable alternative to use the plug in __f2fs_write_data_pages(). Thanks. >> >> Thanks, >> >>> + blk_flush_plug(current->plug, false); >>> goto skip_write; >>> } >>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c >>> index ee2e1dd64f25..c4b5717a8c1a 100644 >>> --- a/fs/f2fs/node.c >>> +++ b/fs/f2fs/node.c >>> @@ -2126,8 +2126,7 @@ static int f2fs_write_node_pages(struct address_space *mapping, >>> atomic_inc(&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); >>> + blk_flush_plug(current->plug, false); >>> goto skip_write; >>> }
On 2023/9/5 21:42, Chunhai Guo wrote: > > > On 2023/9/5 0:40, Jaegeuk Kim wrote: >> On 09/04, Chao Yu wrote: >>> On 2023/8/31 15:10, Chunhai Guo wrote: >>>> The commit 344150999b7f ("f2fs: fix to avoid potential deadlock") only >>>> requires unplugging current->plug. Using blk_finish_plug() is unnecessary >>>> as it sets current->plug as NULL and prevents wb_writeback() from using >>>> plug in subsequent loops. Instead, use blk_flush_plug() as a replacement. >>>> >>>> Signed-off-by: Chunhai Guo <guochunhai@vivo.com> >>>> --- >>>> fs/f2fs/data.c | 3 +-- >>>> fs/f2fs/node.c | 3 +-- >>>> 2 files changed, 2 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>>> index 916e317ac925..77b4a55d6d94 100644 >>>> --- a/fs/f2fs/data.c >>>> +++ b/fs/f2fs/data.c >>>> @@ -3346,8 +3346,7 @@ static int __f2fs_write_data_pages(struct address_space *mapping, >>>> atomic_inc(&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); >>> >>> What about? >>> >>> if (current->plug) { >>> struct blk_plug *plug = current->plug; >>> >>> blk_finish_plug(plug); >>> blk_start_plug(plug); >>> } >>> >>> Jaegeuk, thoughts? >> >> By the way, do we really need to reuse current->plug again? After checkpoint >> being done, we can use the plug in __f2fs_write_data_pages. Are there some >> numbers to show the benefit? >> > > I don't have a number on this, but I think that reusing current->plug > can help maintain the original intention of wb_writeback(). > Nevertheless, it can be a viable alternative to use the plug in > __f2fs_write_data_pages(). I don't have numbers as well, but it can avoid break the intention of per-writeback-work plug, which tries to merge IOs from all dirty inode. Thanks, > > Thanks. >>> >>> Thanks, >>> >>>> + blk_flush_plug(current->plug, false); >>>> goto skip_write; >>>> } >>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c >>>> index ee2e1dd64f25..c4b5717a8c1a 100644 >>>> --- a/fs/f2fs/node.c >>>> +++ b/fs/f2fs/node.c >>>> @@ -2126,8 +2126,7 @@ static int f2fs_write_node_pages(struct address_space *mapping, >>>> atomic_inc(&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); >>>> + blk_flush_plug(current->plug, false); >>>> goto skip_write; >>>> }
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 916e317ac925..77b4a55d6d94 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -3346,8 +3346,7 @@ static int __f2fs_write_data_pages(struct address_space *mapping, atomic_inc(&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); + blk_flush_plug(current->plug, false); goto skip_write; } diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index ee2e1dd64f25..c4b5717a8c1a 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -2126,8 +2126,7 @@ static int f2fs_write_node_pages(struct address_space *mapping, atomic_inc(&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); + blk_flush_plug(current->plug, false); goto skip_write; }
The commit 344150999b7f ("f2fs: fix to avoid potential deadlock") only requires unplugging current->plug. Using blk_finish_plug() is unnecessary as it sets current->plug as NULL and prevents wb_writeback() from using plug in subsequent loops. Instead, use blk_flush_plug() as a replacement. Signed-off-by: Chunhai Guo <guochunhai@vivo.com> --- fs/f2fs/data.c | 3 +-- fs/f2fs/node.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)