Hi hebiao, On Wed, Jul 13, 2016 at 01:21:15AM +0000, 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: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); > > >>> } > > >>> > > > > > > . > > > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
