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 <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
