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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux