RE: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[hebiao (G)]

Thanks Kim, it'll be very meaningful in block layer :-)

-Original Message-
From: Jaegeuk Kim [mailto:jaeg...@kernel.org] 
Sent: 2016年7月14日 10:40
To: hebiao (G) <hebi...@huawei.com>
Cc: Yuchao (T) <yuch...@huawei.com>; linux-kernel@vger.kernel.org; 
linux-fsde...@vger.kernel.org; linux-f2fs-de...@lists.sourceforge.net; CHEN 
CHUN YEN (IAN) <chen.chun@huawei.com>; Kongfei <kong...@hisilicon.com>
Subject: Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

Hi hebiao,

On Wed, Jul 13, 2016 at 01:21:15AM +, hebiao (G) wrote:
> Hi, Kim,
> 
>   You are right. If file system can merge bios as much as possible. It's 
> very helpful to block layer. But plugging mechanism has a precognition of IO 
> stream except merging bios. For example, we can out the low-power mode in 
> advance when you start a plug and we can in the low-power mode only when you 
> end a plug to avoid in-out low-power mode frequently. So I want to know if 
> there is any side effect in F2FS to reserve the plug mechanism ?

Oh, does android kernel act like that?
Nevertheless, I'll take a look at any performance regression if I add plugs in 
all the IO submission again.

Thanks,

> 
> -Original Message-
> From: Jaegeuk Kim [mailto:jaeg...@kernel.org]
> Sent: 2016年7月13日 1:08
> To: Yuchao (T) <yuch...@huawei.com>
> Cc: linux-kernel@vger.kernel.org; linux-fsde...@vger.kernel.org; 
> linux-f2fs-de...@lists.sourceforge.net; CHEN CHUN YEN (IAN) 
> <chen.chun....@huawei.com>; hebiao (G) <hebi...@huawei.com>
> Subject: Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
> 
> Hi Chao,
> 
> On Tue, Jul 12, 2016 at 09:38:11AM +0800, Chao Yu wrote:
> > On 2016/7/10 0:32, Jaegeuk Kim wrote:
> > > On Sat, Jul 09, 2016 at 10:28:49AM +0800, Chao Yu wrote:
> > >> Hi Jaegeuk,
> > >>
> > >> On 2016/6/9 1:24, Jaegeuk Kim wrote:
> > >>> In f2fs, we don't need to keep block plugging for NODE and DATA 
> > >>> writes, since we already merged bios as much as possible.
> > >>
> > >> IMO, we can not remove block plug, this is because there are 
> > >> still many conditions which stops us merging r/w IOs into one bio 
> > >> as we expect, theoretically, block plug can hold bios as much as 
> > >> possible, then submitting them into queue in batch, it will 
> > >> reduce racing of grabbing queue->lock during bio submitting, if 
> > >> we drop them, when syncing nodes or flushing datas, we will suffer more 
> > >> lock racing.
> > >>
> > >> Or there are something I am missing, do you suffer any 
> > >> performance issue on block plug?
> > > 
> > > In the latest patch, I've turned off plugging forcefully, only if 
> > > the underlying device is SMR drive.
> > 
> > Got it.
> > 
> > > And, still I removed other block plugging, since I couldn't see 
> > > any performance regression.
> > 
> > I suspect that in low-end machine with single-queue which is used in 
> > block layer, we will suffer regression.
> > 
> > > Even in some workloads, I could have seen some inverted IOs due to 
> > > race condition between plugged and unplugged IOs.
> > 
> > For data path, what about enabling block plug for IPU/SSR ?
> 
> Not sure. IPU and SSR will produce small (likely random) writes.
> What I'm seeing here is that we already try to merge bios as much as possible.
> Thus, I'm in doubt why we need to wait for merging them by block layer.
> If possible, could you check this in android?
> 
> Thanks,
> 
> > >>>
> > >>> Signed-off-by: Jaegeuk Kim <jaeg...@kernel.org>
> > >>> ---
> > >>>  fs/f2fs/checkpoint.c |  4 
> > >>>  fs/f2fs/data.c   | 17 ++---
> > >>>  fs/f2fs/gc.c |  5 -
> > >>>  fs/f2fs/segment.c|  7 +--
> > >>>  4 files changed, 11 insertions(+), 22 deletions(-)
> > >>>
> > >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 
> > >>> 5ddd15c..4179c7b 100644
> > >>> --- a/fs/f2fs/checkpoint.c
> > >>> +++ b/fs/f2fs/checkpoint.c
> > >>> @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info 
> > >>> *sbi)
> > >>> .nr_to_write = LONG_MAX,
> > >>> .for_reclaim = 0,
> > >>> };
> > >>> -   struct blk_plug plug;
> > >>> int err = 0;
> > >>>  
> > &

RE: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[hebiao (G)]

Thanks Kim, it'll be very meaningful in block layer :-)

-Original Message-
From: Jaegeuk Kim [mailto:jaeg...@kernel.org] 
Sent: 2016年7月14日 10:40
To: hebiao (G) 
Cc: Yuchao (T) ; linux-kernel@vger.kernel.org; 
linux-fsde...@vger.kernel.org; linux-f2fs-de...@lists.sourceforge.net; CHEN 
CHUN YEN (IAN) ; Kongfei 
Subject: Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

Hi hebiao,

On Wed, Jul 13, 2016 at 01:21:15AM +, hebiao (G) wrote:
> Hi, Kim,
> 
>   You are right. If file system can merge bios as much as possible. It's 
> very helpful to block layer. But plugging mechanism has a precognition of IO 
> stream except merging bios. For example, we can out the low-power mode in 
> advance when you start a plug and we can in the low-power mode only when you 
> end a plug to avoid in-out low-power mode frequently. So I want to know if 
> there is any side effect in F2FS to reserve the plug mechanism ?

Oh, does android kernel act like that?
Nevertheless, I'll take a look at any performance regression if I add plugs in 
all the IO submission again.

Thanks,

> 
> -Original Message-
> From: Jaegeuk Kim [mailto:jaeg...@kernel.org]
> Sent: 2016年7月13日 1:08
> To: Yuchao (T) 
> Cc: linux-kernel@vger.kernel.org; linux-fsde...@vger.kernel.org; 
> linux-f2fs-de...@lists.sourceforge.net; CHEN CHUN YEN (IAN) 
> ; hebiao (G) 
> Subject: Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
> 
> Hi Chao,
> 
> On Tue, Jul 12, 2016 at 09:38:11AM +0800, Chao Yu wrote:
> > On 2016/7/10 0:32, Jaegeuk Kim wrote:
> > > On Sat, Jul 09, 2016 at 10:28:49AM +0800, Chao Yu wrote:
> > >> Hi Jaegeuk,
> > >>
> > >> On 2016/6/9 1:24, Jaegeuk Kim wrote:
> > >>> In f2fs, we don't need to keep block plugging for NODE and DATA 
> > >>> writes, since we already merged bios as much as possible.
> > >>
> > >> IMO, we can not remove block plug, this is because there are 
> > >> still many conditions which stops us merging r/w IOs into one bio 
> > >> as we expect, theoretically, block plug can hold bios as much as 
> > >> possible, then submitting them into queue in batch, it will 
> > >> reduce racing of grabbing queue->lock during bio submitting, if 
> > >> we drop them, when syncing nodes or flushing datas, we will suffer more 
> > >> lock racing.
> > >>
> > >> Or there are something I am missing, do you suffer any 
> > >> performance issue on block plug?
> > > 
> > > In the latest patch, I've turned off plugging forcefully, only if 
> > > the underlying device is SMR drive.
> > 
> > Got it.
> > 
> > > And, still I removed other block plugging, since I couldn't see 
> > > any performance regression.
> > 
> > I suspect that in low-end machine with single-queue which is used in 
> > block layer, we will suffer regression.
> > 
> > > Even in some workloads, I could have seen some inverted IOs due to 
> > > race condition between plugged and unplugged IOs.
> > 
> > For data path, what about enabling block plug for IPU/SSR ?
> 
> Not sure. IPU and SSR will produce small (likely random) writes.
> What I'm seeing here is that we already try to merge bios as much as possible.
> Thus, I'm in doubt why we need to wait for merging them by block layer.
> If possible, could you check this in android?
> 
> Thanks,
> 
> > >>>
> > >>> Signed-off-by: Jaegeuk Kim 
> > >>> ---
> > >>>  fs/f2fs/checkpoint.c |  4 
> > >>>  fs/f2fs/data.c   | 17 ++---
> > >>>  fs/f2fs/gc.c |  5 -
> > >>>  fs/f2fs/segment.c|  7 +--
> > >>>  4 files changed, 11 insertions(+), 22 deletions(-)
> > >>>
> > >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 
> > >>> 5ddd15c..4179c7b 100644
> > >>> --- a/fs/f2fs/checkpoint.c
> > >>> +++ b/fs/f2fs/checkpoint.c
> > >>> @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info 
> > >>> *sbi)
> > >>> .nr_to_write = LONG_MAX,
> > >>> .for_reclaim = 0,
> > >>> };
> > >>> -   struct blk_plug plug;
> > >>> int err = 0;
> > >>>  
> > >>> -   blk_start_plug();
> > >>> -
> > >>>  retry_flush_dents:
> > >>> f2fs_lock_all(sbi);
> > >>> /* write all t

Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[Jaegeuk Kim]

Hi hebiao,

On Wed, Jul 13, 2016 at 01:21:15AM +, hebiao (G) wrote:
> Hi, Kim,
> 
>   You are right. If file system can merge bios as much as possible. It's 
> very helpful to block layer. But plugging mechanism has a precognition of IO 
> stream except merging bios. For example, we can out the low-power mode in 
> advance when you start a plug and we can in the low-power mode only when you 
> end a plug to avoid in-out low-power mode frequently. So I want to know if 
> there is any side effect in F2FS to reserve the plug mechanism ?

Oh, does android kernel act like that?
Nevertheless, I'll take a look at any performance regression if I add plugs in
all the IO submission again.

Thanks,

> 
> -Original Message-
> From: Jaegeuk Kim [mailto:jaeg...@kernel.org] 
> Sent: 2016年7月13日 1:08
> To: Yuchao (T) <yuch...@huawei.com>
> Cc: linux-kernel@vger.kernel.org; linux-fsde...@vger.kernel.org; 
> linux-f2fs-de...@lists.sourceforge.net; CHEN CHUN YEN (IAN) 
> <chen.chun@huawei.com>; hebiao (G) <hebi...@huawei.com>
> Subject: Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
> 
> Hi Chao,
> 
> On Tue, Jul 12, 2016 at 09:38:11AM +0800, Chao Yu wrote:
> > On 2016/7/10 0:32, Jaegeuk Kim wrote:
> > > On Sat, Jul 09, 2016 at 10:28:49AM +0800, Chao Yu wrote:
> > >> Hi Jaegeuk,
> > >>
> > >> On 2016/6/9 1:24, Jaegeuk Kim wrote:
> > >>> In f2fs, we don't need to keep block plugging for NODE and DATA 
> > >>> writes, since we already merged bios as much as possible.
> > >>
> > >> IMO, we can not remove block plug, this is because there are still 
> > >> many conditions which stops us merging r/w IOs into one bio as we 
> > >> expect, theoretically, block plug can hold bios as much as 
> > >> possible, then submitting them into queue in batch, it will reduce 
> > >> racing of grabbing queue->lock during bio submitting, if we drop 
> > >> them, when syncing nodes or flushing datas, we will suffer more lock 
> > >> racing.
> > >>
> > >> Or there are something I am missing, do you suffer any performance 
> > >> issue on block plug?
> > > 
> > > In the latest patch, I've turned off plugging forcefully, only if 
> > > the underlying device is SMR drive.
> > 
> > Got it.
> > 
> > > And, still I removed other block plugging, since I couldn't see any 
> > > performance regression.
> > 
> > I suspect that in low-end machine with single-queue which is used in 
> > block layer, we will suffer regression.
> > 
> > > Even in some workloads, I could have seen some inverted IOs due to 
> > > race condition between plugged and unplugged IOs.
> > 
> > For data path, what about enabling block plug for IPU/SSR ?
> 
> Not sure. IPU and SSR will produce small (likely random) writes.
> What I'm seeing here is that we already try to merge bios as much as possible.
> Thus, I'm in doubt why we need to wait for merging them by block layer.
> If possible, could you check this in android?
> 
> Thanks,
> 
> > >>>
> > >>> Signed-off-by: Jaegeuk Kim <jaeg...@kernel.org>
> > >>> ---
> > >>>  fs/f2fs/checkpoint.c |  4 
> > >>>  fs/f2fs/data.c   | 17 ++---
> > >>>  fs/f2fs/gc.c |  5 -
> > >>>  fs/f2fs/segment.c|  7 +--
> > >>>  4 files changed, 11 insertions(+), 22 deletions(-)
> > >>>
> > >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 
> > >>> 5ddd15c..4179c7b 100644
> > >>> --- a/fs/f2fs/checkpoint.c
> > >>> +++ b/fs/f2fs/checkpoint.c
> > >>> @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info 
> > >>> *sbi)
> > >>> .nr_to_write = LONG_MAX,
> > >>> .for_reclaim = 0,
> > >>> };
> > >>> -   struct blk_plug plug;
> > >>> int err = 0;
> > >>>  
> > >>> -   blk_start_plug();
> > >>> -
> > >>>  retry_flush_dents:
> > >>> f2fs_lock_all(sbi);
> > >>> /* write all the dirty dentry pages */ @@ -938,7 +935,6 @@ 
> > >>> retry_flush_nodes:
> > >>> goto retry_flush_nodes;
> > >>> }
> > >>>  out:
> > >>> -   blk_finish_plug();
> > >>> return err;
&g

Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[Jaegeuk Kim]

Hi hebiao,

On Wed, Jul 13, 2016 at 01:21:15AM +, hebiao (G) wrote:
> Hi, Kim,
> 
>   You are right. If file system can merge bios as much as possible. It's 
> very helpful to block layer. But plugging mechanism has a precognition of IO 
> stream except merging bios. For example, we can out the low-power mode in 
> advance when you start a plug and we can in the low-power mode only when you 
> end a plug to avoid in-out low-power mode frequently. So I want to know if 
> there is any side effect in F2FS to reserve the plug mechanism ?

Oh, does android kernel act like that?
Nevertheless, I'll take a look at any performance regression if I add plugs in
all the IO submission again.

Thanks,

> 
> -Original Message-
> From: Jaegeuk Kim [mailto:jaeg...@kernel.org] 
> Sent: 2016年7月13日 1:08
> To: Yuchao (T) 
> Cc: linux-kernel@vger.kernel.org; linux-fsde...@vger.kernel.org; 
> linux-f2fs-de...@lists.sourceforge.net; CHEN CHUN YEN (IAN) 
> ; hebiao (G) 
> Subject: Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
> 
> Hi Chao,
> 
> On Tue, Jul 12, 2016 at 09:38:11AM +0800, Chao Yu wrote:
> > On 2016/7/10 0:32, Jaegeuk Kim wrote:
> > > On Sat, Jul 09, 2016 at 10:28:49AM +0800, Chao Yu wrote:
> > >> Hi Jaegeuk,
> > >>
> > >> On 2016/6/9 1:24, Jaegeuk Kim wrote:
> > >>> In f2fs, we don't need to keep block plugging for NODE and DATA 
> > >>> writes, since we already merged bios as much as possible.
> > >>
> > >> IMO, we can not remove block plug, this is because there are still 
> > >> many conditions which stops us merging r/w IOs into one bio as we 
> > >> expect, theoretically, block plug can hold bios as much as 
> > >> possible, then submitting them into queue in batch, it will reduce 
> > >> racing of grabbing queue->lock during bio submitting, if we drop 
> > >> them, when syncing nodes or flushing datas, we will suffer more lock 
> > >> racing.
> > >>
> > >> Or there are something I am missing, do you suffer any performance 
> > >> issue on block plug?
> > > 
> > > In the latest patch, I've turned off plugging forcefully, only if 
> > > the underlying device is SMR drive.
> > 
> > Got it.
> > 
> > > And, still I removed other block plugging, since I couldn't see any 
> > > performance regression.
> > 
> > I suspect that in low-end machine with single-queue which is used in 
> > block layer, we will suffer regression.
> > 
> > > Even in some workloads, I could have seen some inverted IOs due to 
> > > race condition between plugged and unplugged IOs.
> > 
> > For data path, what about enabling block plug for IPU/SSR ?
> 
> Not sure. IPU and SSR will produce small (likely random) writes.
> What I'm seeing here is that we already try to merge bios as much as possible.
> Thus, I'm in doubt why we need to wait for merging them by block layer.
> If possible, could you check this in android?
> 
> Thanks,
> 
> > >>>
> > >>> Signed-off-by: Jaegeuk Kim 
> > >>> ---
> > >>>  fs/f2fs/checkpoint.c |  4 
> > >>>  fs/f2fs/data.c   | 17 ++---
> > >>>  fs/f2fs/gc.c |  5 -
> > >>>  fs/f2fs/segment.c|  7 +--
> > >>>  4 files changed, 11 insertions(+), 22 deletions(-)
> > >>>
> > >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 
> > >>> 5ddd15c..4179c7b 100644
> > >>> --- a/fs/f2fs/checkpoint.c
> > >>> +++ b/fs/f2fs/checkpoint.c
> > >>> @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info 
> > >>> *sbi)
> > >>> .nr_to_write = LONG_MAX,
> > >>> .for_reclaim = 0,
> > >>> };
> > >>> -   struct blk_plug plug;
> > >>> int err = 0;
> > >>>  
> > >>> -   blk_start_plug();
> > >>> -
> > >>>  retry_flush_dents:
> > >>> f2fs_lock_all(sbi);
> > >>> /* write all the dirty dentry pages */ @@ -938,7 +935,6 @@ 
> > >>> retry_flush_nodes:
> > >>> goto retry_flush_nodes;
> > >>> }
> > >>>  out:
> > >>> -   blk_finish_plug();
> > >>> return err;
> > >>>  }
> > >>>  
> > >>> diff --g

RE: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[hebiao (G)]

Hi, Kim,

You are right. If file system can merge bios as much as possible. It's 
very helpful to block layer. But plugging mechanism has a precognition of IO 
stream except merging bios. For example, we can out the low-power mode in 
advance when you start a plug and we can in the low-power mode only when you 
end a plug to avoid in-out low-power mode frequently. So I want to know if 
there is any side effect in F2FS to reserve the plug mechanism ?

-Original Message-
From: Jaegeuk Kim [mailto:jaeg...@kernel.org] 
Sent: 2016年7月13日 1:08
To: Yuchao (T) <yuch...@huawei.com>
Cc: linux-kernel@vger.kernel.org; linux-fsde...@vger.kernel.org; 
linux-f2fs-de...@lists.sourceforge.net; CHEN CHUN YEN (IAN) 
<chen.chun@huawei.com>; hebiao (G) <hebi...@huawei.com>
Subject: Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

Hi Chao,

On Tue, Jul 12, 2016 at 09:38:11AM +0800, Chao Yu wrote:
> On 2016/7/10 0:32, Jaegeuk Kim wrote:
> > On Sat, Jul 09, 2016 at 10:28:49AM +0800, Chao Yu wrote:
> >> Hi Jaegeuk,
> >>
> >> On 2016/6/9 1:24, Jaegeuk Kim wrote:
> >>> In f2fs, we don't need to keep block plugging for NODE and DATA 
> >>> writes, since we already merged bios as much as possible.
> >>
> >> IMO, we can not remove block plug, this is because there are still 
> >> many conditions which stops us merging r/w IOs into one bio as we 
> >> expect, theoretically, block plug can hold bios as much as 
> >> possible, then submitting them into queue in batch, it will reduce 
> >> racing of grabbing queue->lock during bio submitting, if we drop 
> >> them, when syncing nodes or flushing datas, we will suffer more lock 
> >> racing.
> >>
> >> Or there are something I am missing, do you suffer any performance 
> >> issue on block plug?
> > 
> > In the latest patch, I've turned off plugging forcefully, only if 
> > the underlying device is SMR drive.
> 
> Got it.
> 
> > And, still I removed other block plugging, since I couldn't see any 
> > performance regression.
> 
> I suspect that in low-end machine with single-queue which is used in 
> block layer, we will suffer regression.
> 
> > Even in some workloads, I could have seen some inverted IOs due to 
> > race condition between plugged and unplugged IOs.
> 
> For data path, what about enabling block plug for IPU/SSR ?

Not sure. IPU and SSR will produce small (likely random) writes.
What I'm seeing here is that we already try to merge bios as much as possible.
Thus, I'm in doubt why we need to wait for merging them by block layer.
If possible, could you check this in android?

Thanks,

> >>>
> >>> Signed-off-by: Jaegeuk Kim <jaeg...@kernel.org>
> >>> ---
> >>>  fs/f2fs/checkpoint.c |  4 
> >>>  fs/f2fs/data.c   | 17 ++---
> >>>  fs/f2fs/gc.c |  5 -
> >>>  fs/f2fs/segment.c|  7 +--
> >>>  4 files changed, 11 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 
> >>> 5ddd15c..4179c7b 100644
> >>> --- a/fs/f2fs/checkpoint.c
> >>> +++ b/fs/f2fs/checkpoint.c
> >>> @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info *sbi)
> >>>   .nr_to_write = LONG_MAX,
> >>>   .for_reclaim = 0,
> >>>   };
> >>> - struct blk_plug plug;
> >>>   int err = 0;
> >>>  
> >>> - blk_start_plug();
> >>> -
> >>>  retry_flush_dents:
> >>>   f2fs_lock_all(sbi);
> >>>   /* write all the dirty dentry pages */ @@ -938,7 +935,6 @@ 
> >>> retry_flush_nodes:
> >>>   goto retry_flush_nodes;
> >>>   }
> >>>  out:
> >>> - blk_finish_plug();
> >>>   return err;
> >>>  }
> >>>  
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 
> >>> 30dc448..5f655d0 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -98,10 +98,13 @@ static struct bio *__bio_alloc(struct 
> >>> f2fs_sb_info *sbi, block_t blk_addr,  }
> >>>  
> >>>  static inline void __submit_bio(struct f2fs_sb_info *sbi, int rw,
> >>> - struct bio *bio)
> >>> + struct bio *bio, enum page_type type)
> >>>  {
> >>> - if (!is_read_io(rw))
> >>> + if (!is_read_io(rw)) {
> >>>   atomic_inc(>nr_wb

RE: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[hebiao (G)]

Hi, Kim,

You are right. If file system can merge bios as much as possible. It's 
very helpful to block layer. But plugging mechanism has a precognition of IO 
stream except merging bios. For example, we can out the low-power mode in 
advance when you start a plug and we can in the low-power mode only when you 
end a plug to avoid in-out low-power mode frequently. So I want to know if 
there is any side effect in F2FS to reserve the plug mechanism ?

-Original Message-
From: Jaegeuk Kim [mailto:jaeg...@kernel.org] 
Sent: 2016年7月13日 1:08
To: Yuchao (T) 
Cc: linux-kernel@vger.kernel.org; linux-fsde...@vger.kernel.org; 
linux-f2fs-de...@lists.sourceforge.net; CHEN CHUN YEN (IAN) 
; hebiao (G) 
Subject: Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

Hi Chao,

On Tue, Jul 12, 2016 at 09:38:11AM +0800, Chao Yu wrote:
> On 2016/7/10 0:32, Jaegeuk Kim wrote:
> > On Sat, Jul 09, 2016 at 10:28:49AM +0800, Chao Yu wrote:
> >> Hi Jaegeuk,
> >>
> >> On 2016/6/9 1:24, Jaegeuk Kim wrote:
> >>> In f2fs, we don't need to keep block plugging for NODE and DATA 
> >>> writes, since we already merged bios as much as possible.
> >>
> >> IMO, we can not remove block plug, this is because there are still 
> >> many conditions which stops us merging r/w IOs into one bio as we 
> >> expect, theoretically, block plug can hold bios as much as 
> >> possible, then submitting them into queue in batch, it will reduce 
> >> racing of grabbing queue->lock during bio submitting, if we drop 
> >> them, when syncing nodes or flushing datas, we will suffer more lock 
> >> racing.
> >>
> >> Or there are something I am missing, do you suffer any performance 
> >> issue on block plug?
> > 
> > In the latest patch, I've turned off plugging forcefully, only if 
> > the underlying device is SMR drive.
> 
> Got it.
> 
> > And, still I removed other block plugging, since I couldn't see any 
> > performance regression.
> 
> I suspect that in low-end machine with single-queue which is used in 
> block layer, we will suffer regression.
> 
> > Even in some workloads, I could have seen some inverted IOs due to 
> > race condition between plugged and unplugged IOs.
> 
> For data path, what about enabling block plug for IPU/SSR ?

Not sure. IPU and SSR will produce small (likely random) writes.
What I'm seeing here is that we already try to merge bios as much as possible.
Thus, I'm in doubt why we need to wait for merging them by block layer.
If possible, could you check this in android?

Thanks,

> >>>
> >>> Signed-off-by: Jaegeuk Kim 
> >>> ---
> >>>  fs/f2fs/checkpoint.c |  4 
> >>>  fs/f2fs/data.c   | 17 ++---
> >>>  fs/f2fs/gc.c |  5 -
> >>>  fs/f2fs/segment.c|  7 +--
> >>>  4 files changed, 11 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 
> >>> 5ddd15c..4179c7b 100644
> >>> --- a/fs/f2fs/checkpoint.c
> >>> +++ b/fs/f2fs/checkpoint.c
> >>> @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info *sbi)
> >>>   .nr_to_write = LONG_MAX,
> >>>   .for_reclaim = 0,
> >>>   };
> >>> - struct blk_plug plug;
> >>>   int err = 0;
> >>>  
> >>> - blk_start_plug();
> >>> -
> >>>  retry_flush_dents:
> >>>   f2fs_lock_all(sbi);
> >>>   /* write all the dirty dentry pages */ @@ -938,7 +935,6 @@ 
> >>> retry_flush_nodes:
> >>>   goto retry_flush_nodes;
> >>>   }
> >>>  out:
> >>> - blk_finish_plug();
> >>>   return err;
> >>>  }
> >>>  
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 
> >>> 30dc448..5f655d0 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -98,10 +98,13 @@ static struct bio *__bio_alloc(struct 
> >>> f2fs_sb_info *sbi, block_t blk_addr,  }
> >>>  
> >>>  static inline void __submit_bio(struct f2fs_sb_info *sbi, int rw,
> >>> - struct bio *bio)
> >>> + struct bio *bio, enum page_type type)
> >>>  {
> >>> - if (!is_read_io(rw))
> >>> + if (!is_read_io(rw)) {
> >>>   atomic_inc(>nr_wb_bios);
> >>> + if (current->plug && (type == DATA || type == NODE))
> >>> + b

Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[Jaegeuk Kim]

Hi Chao,

On Tue, Jul 12, 2016 at 09:38:11AM +0800, Chao Yu wrote:
> On 2016/7/10 0:32, Jaegeuk Kim wrote:
> > On Sat, Jul 09, 2016 at 10:28:49AM +0800, Chao Yu wrote:
> >> Hi Jaegeuk,
> >>
> >> On 2016/6/9 1:24, Jaegeuk Kim wrote:
> >>> In f2fs, we don't need to keep block plugging for NODE and DATA writes, 
> >>> since
> >>> we already merged bios as much as possible.
> >>
> >> IMO, we can not remove block plug, this is because there are still many
> >> conditions which stops us merging r/w IOs into one bio as we expect,
> >> theoretically, block plug can hold bios as much as possible, then 
> >> submitting
> >> them into queue in batch, it will reduce racing of grabbing queue->lock 
> >> during
> >> bio submitting, if we drop them, when syncing nodes or flushing datas, we 
> >> will
> >> suffer more lock racing.
> >>
> >> Or there are something I am missing, do you suffer any performance issue on
> >> block plug?
> > 
> > In the latest patch, I've turned off plugging forcefully, only if the 
> > underlying
> > device is SMR drive.
> 
> Got it.
> 
> > And, still I removed other block plugging, since I couldn't see any 
> > performance
> > regression.
> 
> I suspect that in low-end machine with single-queue which is used in block
> layer, we will suffer regression.
> 
> > Even in some workloads, I could have seen some inverted IOs due to
> > race condition between plugged and unplugged IOs.
> 
> For data path, what about enabling block plug for IPU/SSR ?

Not sure. IPU and SSR will produce small (likely random) writes.
What I'm seeing here is that we already try to merge bios as much as possible.
Thus, I'm in doubt why we need to wait for merging them by block layer.
If possible, could you check this in android?

Thanks,

> >>>
> >>> Signed-off-by: Jaegeuk Kim 
> >>> ---
> >>>  fs/f2fs/checkpoint.c |  4 
> >>>  fs/f2fs/data.c   | 17 ++---
> >>>  fs/f2fs/gc.c |  5 -
> >>>  fs/f2fs/segment.c|  7 +--
> >>>  4 files changed, 11 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>> index 5ddd15c..4179c7b 100644
> >>> --- a/fs/f2fs/checkpoint.c
> >>> +++ b/fs/f2fs/checkpoint.c
> >>> @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info *sbi)
> >>>   .nr_to_write = LONG_MAX,
> >>>   .for_reclaim = 0,
> >>>   };
> >>> - struct blk_plug plug;
> >>>   int err = 0;
> >>>  
> >>> - blk_start_plug();
> >>> -
> >>>  retry_flush_dents:
> >>>   f2fs_lock_all(sbi);
> >>>   /* write all the dirty dentry pages */
> >>> @@ -938,7 +935,6 @@ retry_flush_nodes:
> >>>   goto retry_flush_nodes;
> >>>   }
> >>>  out:
> >>> - blk_finish_plug();
> >>>   return err;
> >>>  }
> >>>  
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>> index 30dc448..5f655d0 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -98,10 +98,13 @@ static struct bio *__bio_alloc(struct f2fs_sb_info 
> >>> *sbi, block_t blk_addr,
> >>>  }
> >>>  
> >>>  static inline void __submit_bio(struct f2fs_sb_info *sbi, int rw,
> >>> - struct bio *bio)
> >>> + struct bio *bio, enum page_type type)
> >>>  {
> >>> - if (!is_read_io(rw))
> >>> + if (!is_read_io(rw)) {
> >>>   atomic_inc(>nr_wb_bios);
> >>> + if (current->plug && (type == DATA || type == NODE))
> >>> + blk_finish_plug(current->plug);
> >>> + }
> >>>   submit_bio(rw, bio);
> >>>  }
> >>>  
> >>> @@ -117,7 +120,7 @@ static void __submit_merged_bio(struct f2fs_bio_info 
> >>> *io)
> >>>   else
> >>>   trace_f2fs_submit_write_bio(io->sbi->sb, fio, io->bio);
> >>>  
> >>> - __submit_bio(io->sbi, fio->rw, io->bio);
> >>> + __submit_bio(io->sbi, fio->rw, io->bio, fio->type);
> >>>   io->bio = NULL;
> >>>  }
> >>>  
> >>> @@ -235,7 +238,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
> >>>   return -EFAULT;
> >>>   }
> >>>  
> >>> - __submit_bio(fio->sbi, fio->rw, bio);
> >>> + __submit_bio(fio->sbi, fio->rw, bio, fio->type);
> >>>   return 0;
> >>>  }
> >>>  
> >>> @@ -1040,7 +1043,7 @@ got_it:
> >>>*/
> >>>   if (bio && (last_block_in_bio != block_nr - 1)) {
> >>>  submit_and_realloc:
> >>> - __submit_bio(F2FS_I_SB(inode), READ, bio);
> >>> + __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
> >>>   bio = NULL;
> >>>   }
> >>>   if (bio == NULL) {
> >>> @@ -1083,7 +1086,7 @@ set_error_page:
> >>>   goto next_page;
> >>>  confused:
> >>>   if (bio) {
> >>> - __submit_bio(F2FS_I_SB(inode), READ, bio);
> >>> + __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
> >>>   bio = NULL;
> >>>   }
> >>>   unlock_page(page);
> >>> @@ -1093,7 +1096,7 @@ next_page:
> >>>   }
> >>>   BUG_ON(pages && !list_empty(pages));
> >>>   if (bio)
> >>> - __submit_bio(F2FS_I_SB(inode), 

Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[Jaegeuk Kim]

Hi Chao,

On Tue, Jul 12, 2016 at 09:38:11AM +0800, Chao Yu wrote:
> On 2016/7/10 0:32, Jaegeuk Kim wrote:
> > On Sat, Jul 09, 2016 at 10:28:49AM +0800, Chao Yu wrote:
> >> Hi Jaegeuk,
> >>
> >> On 2016/6/9 1:24, Jaegeuk Kim wrote:
> >>> In f2fs, we don't need to keep block plugging for NODE and DATA writes, 
> >>> since
> >>> we already merged bios as much as possible.
> >>
> >> IMO, we can not remove block plug, this is because there are still many
> >> conditions which stops us merging r/w IOs into one bio as we expect,
> >> theoretically, block plug can hold bios as much as possible, then 
> >> submitting
> >> them into queue in batch, it will reduce racing of grabbing queue->lock 
> >> during
> >> bio submitting, if we drop them, when syncing nodes or flushing datas, we 
> >> will
> >> suffer more lock racing.
> >>
> >> Or there are something I am missing, do you suffer any performance issue on
> >> block plug?
> > 
> > In the latest patch, I've turned off plugging forcefully, only if the 
> > underlying
> > device is SMR drive.
> 
> Got it.
> 
> > And, still I removed other block plugging, since I couldn't see any 
> > performance
> > regression.
> 
> I suspect that in low-end machine with single-queue which is used in block
> layer, we will suffer regression.
> 
> > Even in some workloads, I could have seen some inverted IOs due to
> > race condition between plugged and unplugged IOs.
> 
> For data path, what about enabling block plug for IPU/SSR ?

Not sure. IPU and SSR will produce small (likely random) writes.
What I'm seeing here is that we already try to merge bios as much as possible.
Thus, I'm in doubt why we need to wait for merging them by block layer.
If possible, could you check this in android?

Thanks,

> >>>
> >>> Signed-off-by: Jaegeuk Kim 
> >>> ---
> >>>  fs/f2fs/checkpoint.c |  4 
> >>>  fs/f2fs/data.c   | 17 ++---
> >>>  fs/f2fs/gc.c |  5 -
> >>>  fs/f2fs/segment.c|  7 +--
> >>>  4 files changed, 11 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>> index 5ddd15c..4179c7b 100644
> >>> --- a/fs/f2fs/checkpoint.c
> >>> +++ b/fs/f2fs/checkpoint.c
> >>> @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info *sbi)
> >>>   .nr_to_write = LONG_MAX,
> >>>   .for_reclaim = 0,
> >>>   };
> >>> - struct blk_plug plug;
> >>>   int err = 0;
> >>>  
> >>> - blk_start_plug();
> >>> -
> >>>  retry_flush_dents:
> >>>   f2fs_lock_all(sbi);
> >>>   /* write all the dirty dentry pages */
> >>> @@ -938,7 +935,6 @@ retry_flush_nodes:
> >>>   goto retry_flush_nodes;
> >>>   }
> >>>  out:
> >>> - blk_finish_plug();
> >>>   return err;
> >>>  }
> >>>  
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>> index 30dc448..5f655d0 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -98,10 +98,13 @@ static struct bio *__bio_alloc(struct f2fs_sb_info 
> >>> *sbi, block_t blk_addr,
> >>>  }
> >>>  
> >>>  static inline void __submit_bio(struct f2fs_sb_info *sbi, int rw,
> >>> - struct bio *bio)
> >>> + struct bio *bio, enum page_type type)
> >>>  {
> >>> - if (!is_read_io(rw))
> >>> + if (!is_read_io(rw)) {
> >>>   atomic_inc(>nr_wb_bios);
> >>> + if (current->plug && (type == DATA || type == NODE))
> >>> + blk_finish_plug(current->plug);
> >>> + }
> >>>   submit_bio(rw, bio);
> >>>  }
> >>>  
> >>> @@ -117,7 +120,7 @@ static void __submit_merged_bio(struct f2fs_bio_info 
> >>> *io)
> >>>   else
> >>>   trace_f2fs_submit_write_bio(io->sbi->sb, fio, io->bio);
> >>>  
> >>> - __submit_bio(io->sbi, fio->rw, io->bio);
> >>> + __submit_bio(io->sbi, fio->rw, io->bio, fio->type);
> >>>   io->bio = NULL;
> >>>  }
> >>>  
> >>> @@ -235,7 +238,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
> >>>   return -EFAULT;
> >>>   }
> >>>  
> >>> - __submit_bio(fio->sbi, fio->rw, bio);
> >>> + __submit_bio(fio->sbi, fio->rw, bio, fio->type);
> >>>   return 0;
> >>>  }
> >>>  
> >>> @@ -1040,7 +1043,7 @@ got_it:
> >>>*/
> >>>   if (bio && (last_block_in_bio != block_nr - 1)) {
> >>>  submit_and_realloc:
> >>> - __submit_bio(F2FS_I_SB(inode), READ, bio);
> >>> + __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
> >>>   bio = NULL;
> >>>   }
> >>>   if (bio == NULL) {
> >>> @@ -1083,7 +1086,7 @@ set_error_page:
> >>>   goto next_page;
> >>>  confused:
> >>>   if (bio) {
> >>> - __submit_bio(F2FS_I_SB(inode), READ, bio);
> >>> + __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
> >>>   bio = NULL;
> >>>   }
> >>>   unlock_page(page);
> >>> @@ -1093,7 +1096,7 @@ next_page:
> >>>   }
> >>>   BUG_ON(pages && !list_empty(pages));
> >>>   if (bio)
> >>> - __submit_bio(F2FS_I_SB(inode), READ, bio);
> >>> +  

Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[Chao Yu]

On 2016/7/10 0:32, Jaegeuk Kim wrote:
> On Sat, Jul 09, 2016 at 10:28:49AM +0800, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2016/6/9 1:24, Jaegeuk Kim wrote:
>>> In f2fs, we don't need to keep block plugging for NODE and DATA writes, 
>>> since
>>> we already merged bios as much as possible.
>>
>> IMO, we can not remove block plug, this is because there are still many
>> conditions which stops us merging r/w IOs into one bio as we expect,
>> theoretically, block plug can hold bios as much as possible, then submitting
>> them into queue in batch, it will reduce racing of grabbing queue->lock 
>> during
>> bio submitting, if we drop them, when syncing nodes or flushing datas, we 
>> will
>> suffer more lock racing.
>>
>> Or there are something I am missing, do you suffer any performance issue on
>> block plug?
> 
> In the latest patch, I've turned off plugging forcefully, only if the 
> underlying
> device is SMR drive.

Got it.

> And, still I removed other block plugging, since I couldn't see any 
> performance
> regression.

I suspect that in low-end machine with single-queue which is used in block
layer, we will suffer regression.

> Even in some workloads, I could have seen some inverted IOs due to
> race condition between plugged and unplugged IOs.

For data path, what about enabling block plug for IPU/SSR ?

Thanks,

> 
> Thanks,
> 
>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Jaegeuk Kim 
>>> ---
>>>  fs/f2fs/checkpoint.c |  4 
>>>  fs/f2fs/data.c   | 17 ++---
>>>  fs/f2fs/gc.c |  5 -
>>>  fs/f2fs/segment.c|  7 +--
>>>  4 files changed, 11 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index 5ddd15c..4179c7b 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info *sbi)
>>> .nr_to_write = LONG_MAX,
>>> .for_reclaim = 0,
>>> };
>>> -   struct blk_plug plug;
>>> int err = 0;
>>>  
>>> -   blk_start_plug();
>>> -
>>>  retry_flush_dents:
>>> f2fs_lock_all(sbi);
>>> /* write all the dirty dentry pages */
>>> @@ -938,7 +935,6 @@ retry_flush_nodes:
>>> goto retry_flush_nodes;
>>> }
>>>  out:
>>> -   blk_finish_plug();
>>> return err;
>>>  }
>>>  
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 30dc448..5f655d0 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -98,10 +98,13 @@ static struct bio *__bio_alloc(struct f2fs_sb_info 
>>> *sbi, block_t blk_addr,
>>>  }
>>>  
>>>  static inline void __submit_bio(struct f2fs_sb_info *sbi, int rw,
>>> -   struct bio *bio)
>>> +   struct bio *bio, enum page_type type)
>>>  {
>>> -   if (!is_read_io(rw))
>>> +   if (!is_read_io(rw)) {
>>> atomic_inc(>nr_wb_bios);
>>> +   if (current->plug && (type == DATA || type == NODE))
>>> +   blk_finish_plug(current->plug);
>>> +   }
>>> submit_bio(rw, bio);
>>>  }
>>>  
>>> @@ -117,7 +120,7 @@ static void __submit_merged_bio(struct f2fs_bio_info 
>>> *io)
>>> else
>>> trace_f2fs_submit_write_bio(io->sbi->sb, fio, io->bio);
>>>  
>>> -   __submit_bio(io->sbi, fio->rw, io->bio);
>>> +   __submit_bio(io->sbi, fio->rw, io->bio, fio->type);
>>> io->bio = NULL;
>>>  }
>>>  
>>> @@ -235,7 +238,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>>> return -EFAULT;
>>> }
>>>  
>>> -   __submit_bio(fio->sbi, fio->rw, bio);
>>> +   __submit_bio(fio->sbi, fio->rw, bio, fio->type);
>>> return 0;
>>>  }
>>>  
>>> @@ -1040,7 +1043,7 @@ got_it:
>>>  */
>>> if (bio && (last_block_in_bio != block_nr - 1)) {
>>>  submit_and_realloc:
>>> -   __submit_bio(F2FS_I_SB(inode), READ, bio);
>>> +   __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
>>> bio = NULL;
>>> }
>>> if (bio == NULL) {
>>> @@ -1083,7 +1086,7 @@ set_error_page:
>>> goto next_page;
>>>  confused:
>>> if (bio) {
>>> -   __submit_bio(F2FS_I_SB(inode), READ, bio);
>>> +   __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
>>> bio = NULL;
>>> }
>>> unlock_page(page);
>>> @@ -1093,7 +1096,7 @@ next_page:
>>> }
>>> BUG_ON(pages && !list_empty(pages));
>>> if (bio)
>>> -   __submit_bio(F2FS_I_SB(inode), READ, bio);
>>> +   __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
>>> return 0;
>>>  }
>>>  
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index 4a03076..67fd285 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -777,7 +777,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>>>  {
>>> struct page *sum_page;
>>> struct f2fs_summary_block *sum;
>>> -   struct blk_plug plug;
>>> unsigned int segno = start_segno;
>>> 

Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[Chao Yu]

On 2016/7/10 0:32, Jaegeuk Kim wrote:
> On Sat, Jul 09, 2016 at 10:28:49AM +0800, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2016/6/9 1:24, Jaegeuk Kim wrote:
>>> In f2fs, we don't need to keep block plugging for NODE and DATA writes, 
>>> since
>>> we already merged bios as much as possible.
>>
>> IMO, we can not remove block plug, this is because there are still many
>> conditions which stops us merging r/w IOs into one bio as we expect,
>> theoretically, block plug can hold bios as much as possible, then submitting
>> them into queue in batch, it will reduce racing of grabbing queue->lock 
>> during
>> bio submitting, if we drop them, when syncing nodes or flushing datas, we 
>> will
>> suffer more lock racing.
>>
>> Or there are something I am missing, do you suffer any performance issue on
>> block plug?
> 
> In the latest patch, I've turned off plugging forcefully, only if the 
> underlying
> device is SMR drive.

Got it.

> And, still I removed other block plugging, since I couldn't see any 
> performance
> regression.

I suspect that in low-end machine with single-queue which is used in block
layer, we will suffer regression.

> Even in some workloads, I could have seen some inverted IOs due to
> race condition between plugged and unplugged IOs.

For data path, what about enabling block plug for IPU/SSR ?

Thanks,

> 
> Thanks,
> 
>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Jaegeuk Kim 
>>> ---
>>>  fs/f2fs/checkpoint.c |  4 
>>>  fs/f2fs/data.c   | 17 ++---
>>>  fs/f2fs/gc.c |  5 -
>>>  fs/f2fs/segment.c|  7 +--
>>>  4 files changed, 11 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index 5ddd15c..4179c7b 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info *sbi)
>>> .nr_to_write = LONG_MAX,
>>> .for_reclaim = 0,
>>> };
>>> -   struct blk_plug plug;
>>> int err = 0;
>>>  
>>> -   blk_start_plug();
>>> -
>>>  retry_flush_dents:
>>> f2fs_lock_all(sbi);
>>> /* write all the dirty dentry pages */
>>> @@ -938,7 +935,6 @@ retry_flush_nodes:
>>> goto retry_flush_nodes;
>>> }
>>>  out:
>>> -   blk_finish_plug();
>>> return err;
>>>  }
>>>  
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 30dc448..5f655d0 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -98,10 +98,13 @@ static struct bio *__bio_alloc(struct f2fs_sb_info 
>>> *sbi, block_t blk_addr,
>>>  }
>>>  
>>>  static inline void __submit_bio(struct f2fs_sb_info *sbi, int rw,
>>> -   struct bio *bio)
>>> +   struct bio *bio, enum page_type type)
>>>  {
>>> -   if (!is_read_io(rw))
>>> +   if (!is_read_io(rw)) {
>>> atomic_inc(>nr_wb_bios);
>>> +   if (current->plug && (type == DATA || type == NODE))
>>> +   blk_finish_plug(current->plug);
>>> +   }
>>> submit_bio(rw, bio);
>>>  }
>>>  
>>> @@ -117,7 +120,7 @@ static void __submit_merged_bio(struct f2fs_bio_info 
>>> *io)
>>> else
>>> trace_f2fs_submit_write_bio(io->sbi->sb, fio, io->bio);
>>>  
>>> -   __submit_bio(io->sbi, fio->rw, io->bio);
>>> +   __submit_bio(io->sbi, fio->rw, io->bio, fio->type);
>>> io->bio = NULL;
>>>  }
>>>  
>>> @@ -235,7 +238,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>>> return -EFAULT;
>>> }
>>>  
>>> -   __submit_bio(fio->sbi, fio->rw, bio);
>>> +   __submit_bio(fio->sbi, fio->rw, bio, fio->type);
>>> return 0;
>>>  }
>>>  
>>> @@ -1040,7 +1043,7 @@ got_it:
>>>  */
>>> if (bio && (last_block_in_bio != block_nr - 1)) {
>>>  submit_and_realloc:
>>> -   __submit_bio(F2FS_I_SB(inode), READ, bio);
>>> +   __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
>>> bio = NULL;
>>> }
>>> if (bio == NULL) {
>>> @@ -1083,7 +1086,7 @@ set_error_page:
>>> goto next_page;
>>>  confused:
>>> if (bio) {
>>> -   __submit_bio(F2FS_I_SB(inode), READ, bio);
>>> +   __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
>>> bio = NULL;
>>> }
>>> unlock_page(page);
>>> @@ -1093,7 +1096,7 @@ next_page:
>>> }
>>> BUG_ON(pages && !list_empty(pages));
>>> if (bio)
>>> -   __submit_bio(F2FS_I_SB(inode), READ, bio);
>>> +   __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
>>> return 0;
>>>  }
>>>  
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index 4a03076..67fd285 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -777,7 +777,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>>>  {
>>> struct page *sum_page;
>>> struct f2fs_summary_block *sum;
>>> -   struct blk_plug plug;
>>> unsigned int segno = start_segno;
>>> unsigned int end_segno = 

Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[Jaegeuk Kim]

On Sat, Jul 09, 2016 at 10:28:49AM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> On 2016/6/9 1:24, Jaegeuk Kim wrote:
> > In f2fs, we don't need to keep block plugging for NODE and DATA writes, 
> > since
> > we already merged bios as much as possible.
> 
> IMO, we can not remove block plug, this is because there are still many
> conditions which stops us merging r/w IOs into one bio as we expect,
> theoretically, block plug can hold bios as much as possible, then submitting
> them into queue in batch, it will reduce racing of grabbing queue->lock during
> bio submitting, if we drop them, when syncing nodes or flushing datas, we will
> suffer more lock racing.
> 
> Or there are something I am missing, do you suffer any performance issue on
> block plug?

In the latest patch, I've turned off plugging forcefully, only if the underlying
device is SMR drive.
And, still I removed other block plugging, since I couldn't see any performance
regression. Even in some workloads, I could have seen some inverted IOs due to
race condition between plugged and unplugged IOs.

Thanks,

> 
> Thanks,
> 
> > 
> > Signed-off-by: Jaegeuk Kim 
> > ---
> >  fs/f2fs/checkpoint.c |  4 
> >  fs/f2fs/data.c   | 17 ++---
> >  fs/f2fs/gc.c |  5 -
> >  fs/f2fs/segment.c|  7 +--
> >  4 files changed, 11 insertions(+), 22 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 5ddd15c..4179c7b 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info *sbi)
> > .nr_to_write = LONG_MAX,
> > .for_reclaim = 0,
> > };
> > -   struct blk_plug plug;
> > int err = 0;
> >  
> > -   blk_start_plug();
> > -
> >  retry_flush_dents:
> > f2fs_lock_all(sbi);
> > /* write all the dirty dentry pages */
> > @@ -938,7 +935,6 @@ retry_flush_nodes:
> > goto retry_flush_nodes;
> > }
> >  out:
> > -   blk_finish_plug();
> > return err;
> >  }
> >  
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 30dc448..5f655d0 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -98,10 +98,13 @@ static struct bio *__bio_alloc(struct f2fs_sb_info 
> > *sbi, block_t blk_addr,
> >  }
> >  
> >  static inline void __submit_bio(struct f2fs_sb_info *sbi, int rw,
> > -   struct bio *bio)
> > +   struct bio *bio, enum page_type type)
> >  {
> > -   if (!is_read_io(rw))
> > +   if (!is_read_io(rw)) {
> > atomic_inc(>nr_wb_bios);
> > +   if (current->plug && (type == DATA || type == NODE))
> > +   blk_finish_plug(current->plug);
> > +   }
> > submit_bio(rw, bio);
> >  }
> >  
> > @@ -117,7 +120,7 @@ static void __submit_merged_bio(struct f2fs_bio_info 
> > *io)
> > else
> > trace_f2fs_submit_write_bio(io->sbi->sb, fio, io->bio);
> >  
> > -   __submit_bio(io->sbi, fio->rw, io->bio);
> > +   __submit_bio(io->sbi, fio->rw, io->bio, fio->type);
> > io->bio = NULL;
> >  }
> >  
> > @@ -235,7 +238,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
> > return -EFAULT;
> > }
> >  
> > -   __submit_bio(fio->sbi, fio->rw, bio);
> > +   __submit_bio(fio->sbi, fio->rw, bio, fio->type);
> > return 0;
> >  }
> >  
> > @@ -1040,7 +1043,7 @@ got_it:
> >  */
> > if (bio && (last_block_in_bio != block_nr - 1)) {
> >  submit_and_realloc:
> > -   __submit_bio(F2FS_I_SB(inode), READ, bio);
> > +   __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
> > bio = NULL;
> > }
> > if (bio == NULL) {
> > @@ -1083,7 +1086,7 @@ set_error_page:
> > goto next_page;
> >  confused:
> > if (bio) {
> > -   __submit_bio(F2FS_I_SB(inode), READ, bio);
> > +   __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
> > bio = NULL;
> > }
> > unlock_page(page);
> > @@ -1093,7 +1096,7 @@ next_page:
> > }
> > BUG_ON(pages && !list_empty(pages));
> > if (bio)
> > -   __submit_bio(F2FS_I_SB(inode), READ, bio);
> > +   __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
> > return 0;
> >  }
> >  
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index 4a03076..67fd285 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -777,7 +777,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> >  {
> > struct page *sum_page;
> > struct f2fs_summary_block *sum;
> > -   struct blk_plug plug;
> > unsigned int segno = start_segno;
> > unsigned int end_segno = start_segno + sbi->segs_per_sec;
> > int seg_freed = 0;
> > @@ -795,8 +794,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> > unlock_page(sum_page);
> > }
> >  
> > -   blk_start_plug();
> > -
> > for (segno = 

Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[Jaegeuk Kim]

On Sat, Jul 09, 2016 at 10:28:49AM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> On 2016/6/9 1:24, Jaegeuk Kim wrote:
> > In f2fs, we don't need to keep block plugging for NODE and DATA writes, 
> > since
> > we already merged bios as much as possible.
> 
> IMO, we can not remove block plug, this is because there are still many
> conditions which stops us merging r/w IOs into one bio as we expect,
> theoretically, block plug can hold bios as much as possible, then submitting
> them into queue in batch, it will reduce racing of grabbing queue->lock during
> bio submitting, if we drop them, when syncing nodes or flushing datas, we will
> suffer more lock racing.
> 
> Or there are something I am missing, do you suffer any performance issue on
> block plug?

In the latest patch, I've turned off plugging forcefully, only if the underlying
device is SMR drive.
And, still I removed other block plugging, since I couldn't see any performance
regression. Even in some workloads, I could have seen some inverted IOs due to
race condition between plugged and unplugged IOs.

Thanks,

> 
> Thanks,
> 
> > 
> > Signed-off-by: Jaegeuk Kim 
> > ---
> >  fs/f2fs/checkpoint.c |  4 
> >  fs/f2fs/data.c   | 17 ++---
> >  fs/f2fs/gc.c |  5 -
> >  fs/f2fs/segment.c|  7 +--
> >  4 files changed, 11 insertions(+), 22 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 5ddd15c..4179c7b 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info *sbi)
> > .nr_to_write = LONG_MAX,
> > .for_reclaim = 0,
> > };
> > -   struct blk_plug plug;
> > int err = 0;
> >  
> > -   blk_start_plug();
> > -
> >  retry_flush_dents:
> > f2fs_lock_all(sbi);
> > /* write all the dirty dentry pages */
> > @@ -938,7 +935,6 @@ retry_flush_nodes:
> > goto retry_flush_nodes;
> > }
> >  out:
> > -   blk_finish_plug();
> > return err;
> >  }
> >  
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 30dc448..5f655d0 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -98,10 +98,13 @@ static struct bio *__bio_alloc(struct f2fs_sb_info 
> > *sbi, block_t blk_addr,
> >  }
> >  
> >  static inline void __submit_bio(struct f2fs_sb_info *sbi, int rw,
> > -   struct bio *bio)
> > +   struct bio *bio, enum page_type type)
> >  {
> > -   if (!is_read_io(rw))
> > +   if (!is_read_io(rw)) {
> > atomic_inc(>nr_wb_bios);
> > +   if (current->plug && (type == DATA || type == NODE))
> > +   blk_finish_plug(current->plug);
> > +   }
> > submit_bio(rw, bio);
> >  }
> >  
> > @@ -117,7 +120,7 @@ static void __submit_merged_bio(struct f2fs_bio_info 
> > *io)
> > else
> > trace_f2fs_submit_write_bio(io->sbi->sb, fio, io->bio);
> >  
> > -   __submit_bio(io->sbi, fio->rw, io->bio);
> > +   __submit_bio(io->sbi, fio->rw, io->bio, fio->type);
> > io->bio = NULL;
> >  }
> >  
> > @@ -235,7 +238,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
> > return -EFAULT;
> > }
> >  
> > -   __submit_bio(fio->sbi, fio->rw, bio);
> > +   __submit_bio(fio->sbi, fio->rw, bio, fio->type);
> > return 0;
> >  }
> >  
> > @@ -1040,7 +1043,7 @@ got_it:
> >  */
> > if (bio && (last_block_in_bio != block_nr - 1)) {
> >  submit_and_realloc:
> > -   __submit_bio(F2FS_I_SB(inode), READ, bio);
> > +   __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
> > bio = NULL;
> > }
> > if (bio == NULL) {
> > @@ -1083,7 +1086,7 @@ set_error_page:
> > goto next_page;
> >  confused:
> > if (bio) {
> > -   __submit_bio(F2FS_I_SB(inode), READ, bio);
> > +   __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
> > bio = NULL;
> > }
> > unlock_page(page);
> > @@ -1093,7 +1096,7 @@ next_page:
> > }
> > BUG_ON(pages && !list_empty(pages));
> > if (bio)
> > -   __submit_bio(F2FS_I_SB(inode), READ, bio);
> > +   __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
> > return 0;
> >  }
> >  
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index 4a03076..67fd285 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -777,7 +777,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> >  {
> > struct page *sum_page;
> > struct f2fs_summary_block *sum;
> > -   struct blk_plug plug;
> > unsigned int segno = start_segno;
> > unsigned int end_segno = start_segno + sbi->segs_per_sec;
> > int seg_freed = 0;
> > @@ -795,8 +794,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> > unlock_page(sum_page);
> > }
> >  
> > -   blk_start_plug();
> > -
> > for (segno = start_segno; segno < 

Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[Chao Yu]

Hi Jaegeuk,

On 2016/6/9 1:24, Jaegeuk Kim wrote:
> In f2fs, we don't need to keep block plugging for NODE and DATA writes, since
> we already merged bios as much as possible.

IMO, we can not remove block plug, this is because there are still many
conditions which stops us merging r/w IOs into one bio as we expect,
theoretically, block plug can hold bios as much as possible, then submitting
them into queue in batch, it will reduce racing of grabbing queue->lock during
bio submitting, if we drop them, when syncing nodes or flushing datas, we will
suffer more lock racing.

Or there are something I am missing, do you suffer any performance issue on
block plug?

Thanks,

> 
> Signed-off-by: Jaegeuk Kim 
> ---
>  fs/f2fs/checkpoint.c |  4 
>  fs/f2fs/data.c   | 17 ++---
>  fs/f2fs/gc.c |  5 -
>  fs/f2fs/segment.c|  7 +--
>  4 files changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 5ddd15c..4179c7b 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info *sbi)
>   .nr_to_write = LONG_MAX,
>   .for_reclaim = 0,
>   };
> - struct blk_plug plug;
>   int err = 0;
>  
> - blk_start_plug();
> -
>  retry_flush_dents:
>   f2fs_lock_all(sbi);
>   /* write all the dirty dentry pages */
> @@ -938,7 +935,6 @@ retry_flush_nodes:
>   goto retry_flush_nodes;
>   }
>  out:
> - blk_finish_plug();
>   return err;
>  }
>  
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 30dc448..5f655d0 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -98,10 +98,13 @@ static struct bio *__bio_alloc(struct f2fs_sb_info *sbi, 
> block_t blk_addr,
>  }
>  
>  static inline void __submit_bio(struct f2fs_sb_info *sbi, int rw,
> - struct bio *bio)
> + struct bio *bio, enum page_type type)
>  {
> - if (!is_read_io(rw))
> + if (!is_read_io(rw)) {
>   atomic_inc(>nr_wb_bios);
> + if (current->plug && (type == DATA || type == NODE))
> + blk_finish_plug(current->plug);
> + }
>   submit_bio(rw, bio);
>  }
>  
> @@ -117,7 +120,7 @@ static void __submit_merged_bio(struct f2fs_bio_info *io)
>   else
>   trace_f2fs_submit_write_bio(io->sbi->sb, fio, io->bio);
>  
> - __submit_bio(io->sbi, fio->rw, io->bio);
> + __submit_bio(io->sbi, fio->rw, io->bio, fio->type);
>   io->bio = NULL;
>  }
>  
> @@ -235,7 +238,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>   return -EFAULT;
>   }
>  
> - __submit_bio(fio->sbi, fio->rw, bio);
> + __submit_bio(fio->sbi, fio->rw, bio, fio->type);
>   return 0;
>  }
>  
> @@ -1040,7 +1043,7 @@ got_it:
>*/
>   if (bio && (last_block_in_bio != block_nr - 1)) {
>  submit_and_realloc:
> - __submit_bio(F2FS_I_SB(inode), READ, bio);
> + __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
>   bio = NULL;
>   }
>   if (bio == NULL) {
> @@ -1083,7 +1086,7 @@ set_error_page:
>   goto next_page;
>  confused:
>   if (bio) {
> - __submit_bio(F2FS_I_SB(inode), READ, bio);
> + __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
>   bio = NULL;
>   }
>   unlock_page(page);
> @@ -1093,7 +1096,7 @@ next_page:
>   }
>   BUG_ON(pages && !list_empty(pages));
>   if (bio)
> - __submit_bio(F2FS_I_SB(inode), READ, bio);
> + __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
>   return 0;
>  }
>  
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 4a03076..67fd285 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -777,7 +777,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>  {
>   struct page *sum_page;
>   struct f2fs_summary_block *sum;
> - struct blk_plug plug;
>   unsigned int segno = start_segno;
>   unsigned int end_segno = start_segno + sbi->segs_per_sec;
>   int seg_freed = 0;
> @@ -795,8 +794,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>   unlock_page(sum_page);
>   }
>  
> - blk_start_plug();
> -
>   for (segno = start_segno; segno < end_segno; segno++) {
>   /* find segment summary of victim */
>   sum_page = find_get_page(META_MAPPING(sbi),
> @@ -830,8 +827,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>   f2fs_submit_merged_bio(sbi,
>   (type == SUM_TYPE_NODE) ? NODE : DATA, WRITE);
>  
> - blk_finish_plug();
> -
>   if (gc_type == FG_GC) {
>   while (start_segno < end_segno)
>   if (get_valid_blocks(sbi, start_segno++, 1) == 0)
> 

Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging

[Chao Yu]

Hi Jaegeuk,

On 2016/6/9 1:24, Jaegeuk Kim wrote:
> In f2fs, we don't need to keep block plugging for NODE and DATA writes, since
> we already merged bios as much as possible.

IMO, we can not remove block plug, this is because there are still many
conditions which stops us merging r/w IOs into one bio as we expect,
theoretically, block plug can hold bios as much as possible, then submitting
them into queue in batch, it will reduce racing of grabbing queue->lock during
bio submitting, if we drop them, when syncing nodes or flushing datas, we will
suffer more lock racing.

Or there are something I am missing, do you suffer any performance issue on
block plug?

Thanks,

> 
> Signed-off-by: Jaegeuk Kim 
> ---
>  fs/f2fs/checkpoint.c |  4 
>  fs/f2fs/data.c   | 17 ++---
>  fs/f2fs/gc.c |  5 -
>  fs/f2fs/segment.c|  7 +--
>  4 files changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 5ddd15c..4179c7b 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -897,11 +897,8 @@ static int block_operations(struct f2fs_sb_info *sbi)
>   .nr_to_write = LONG_MAX,
>   .for_reclaim = 0,
>   };
> - struct blk_plug plug;
>   int err = 0;
>  
> - blk_start_plug();
> -
>  retry_flush_dents:
>   f2fs_lock_all(sbi);
>   /* write all the dirty dentry pages */
> @@ -938,7 +935,6 @@ retry_flush_nodes:
>   goto retry_flush_nodes;
>   }
>  out:
> - blk_finish_plug();
>   return err;
>  }
>  
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 30dc448..5f655d0 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -98,10 +98,13 @@ static struct bio *__bio_alloc(struct f2fs_sb_info *sbi, 
> block_t blk_addr,
>  }
>  
>  static inline void __submit_bio(struct f2fs_sb_info *sbi, int rw,
> - struct bio *bio)
> + struct bio *bio, enum page_type type)
>  {
> - if (!is_read_io(rw))
> + if (!is_read_io(rw)) {
>   atomic_inc(>nr_wb_bios);
> + if (current->plug && (type == DATA || type == NODE))
> + blk_finish_plug(current->plug);
> + }
>   submit_bio(rw, bio);
>  }
>  
> @@ -117,7 +120,7 @@ static void __submit_merged_bio(struct f2fs_bio_info *io)
>   else
>   trace_f2fs_submit_write_bio(io->sbi->sb, fio, io->bio);
>  
> - __submit_bio(io->sbi, fio->rw, io->bio);
> + __submit_bio(io->sbi, fio->rw, io->bio, fio->type);
>   io->bio = NULL;
>  }
>  
> @@ -235,7 +238,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>   return -EFAULT;
>   }
>  
> - __submit_bio(fio->sbi, fio->rw, bio);
> + __submit_bio(fio->sbi, fio->rw, bio, fio->type);
>   return 0;
>  }
>  
> @@ -1040,7 +1043,7 @@ got_it:
>*/
>   if (bio && (last_block_in_bio != block_nr - 1)) {
>  submit_and_realloc:
> - __submit_bio(F2FS_I_SB(inode), READ, bio);
> + __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
>   bio = NULL;
>   }
>   if (bio == NULL) {
> @@ -1083,7 +1086,7 @@ set_error_page:
>   goto next_page;
>  confused:
>   if (bio) {
> - __submit_bio(F2FS_I_SB(inode), READ, bio);
> + __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
>   bio = NULL;
>   }
>   unlock_page(page);
> @@ -1093,7 +1096,7 @@ next_page:
>   }
>   BUG_ON(pages && !list_empty(pages));
>   if (bio)
> - __submit_bio(F2FS_I_SB(inode), READ, bio);
> + __submit_bio(F2FS_I_SB(inode), READ, bio, DATA);
>   return 0;
>  }
>  
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 4a03076..67fd285 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -777,7 +777,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>  {
>   struct page *sum_page;
>   struct f2fs_summary_block *sum;
> - struct blk_plug plug;
>   unsigned int segno = start_segno;
>   unsigned int end_segno = start_segno + sbi->segs_per_sec;
>   int seg_freed = 0;
> @@ -795,8 +794,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>   unlock_page(sum_page);
>   }
>  
> - blk_start_plug();
> -
>   for (segno = start_segno; segno < end_segno; segno++) {
>   /* find segment summary of victim */
>   sum_page = find_get_page(META_MAPPING(sbi),
> @@ -830,8 +827,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>   f2fs_submit_merged_bio(sbi,
>   (type == SUM_TYPE_NODE) ? NODE : DATA, WRITE);
>  
> - blk_finish_plug();
> -
>   if (gc_type == FG_GC) {
>   while (start_segno < end_segno)
>   if (get_valid_blocks(sbi, start_segno++, 1) == 0)
> diff --git