RE: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
From: hebiao (G)
Date: Tue Jul 12 2016 - 21:21:44 EST
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:jaegeuk@xxxxxxxxxx]
Sent: 2016年7月13日 1:08
To: Yuchao (T) <yuchao0@xxxxxxxxxx>
Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx; linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx; CHEN CHUN YEN (IAN) <chen.chun.yen@xxxxxxxxxx>; hebiao (G) <hebiao6@xxxxxxxxxx>
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 <jaegeuk@xxxxxxxxxx>
> >>> ---
> >>> 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(&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(&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(&sbi->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(&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(&plug);
> >>> -
> >>> if (gc_type == FG_GC) {
> >>> while (start_segno < end_segno)
> >>> if (get_valid_blocks(sbi, start_segno++, 1) == 0) diff --git
> >>> a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 7b58bfb..eff046a
> >>> 100644
> >>> --- a/fs/f2fs/segment.c
> >>> +++ b/fs/f2fs/segment.c
> >>> @@ -379,13 +379,8 @@ void f2fs_balance_fs_bg(struct f2fs_sb_info *sbi)
> >>> excess_prefree_segs(sbi) ||
> >>> excess_dirty_nats(sbi) ||
> >>> (is_idle(sbi) && f2fs_time_over(sbi, CP_TIME))) {
> >>> - if (test_opt(sbi, DATA_FLUSH)) {
> >>> - struct blk_plug plug;
> >>> -
> >>> - blk_start_plug(&plug);
> >>> + if (test_opt(sbi, DATA_FLUSH))
> >>> sync_dirty_inodes(sbi, FILE_INODE);
> >>> - blk_finish_plug(&plug);
> >>> - }
> >>> f2fs_sync_fs(sbi->sb, true);
> >>> stat_inc_bg_cp_count(sbi->stat_info);
> >>> }
> >>>
> >
> > .
> >