From: Chao Yu <yuchao0@huawei.com>
To: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: <linux-kernel@vger.kernel.org>, <linux-fsdevel@vger.kernel.org>,
<linux-f2fs-devel@lists.sourceforge.net>,
<chen.chun.yen@huawei.com>, <hebiao6@huawei.com>
Subject: Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
Date: Tue, 12 Jul 2016 09:38:11 +0800 [thread overview]
Message-ID: <ead95e1c-9fe4-9895-0653-90868f865137@huawei.com> (raw)
In-Reply-To: <20160709163226.GB1837@jaegeuk>
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 <jaegeuk@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(&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);
>>> }
>>>
>
> .
>
next prev parent reply other threads:[~2016-07-12 1:38 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-08 17:24 [PATCH 1/7] f2fs: set mapping error for EIO Jaegeuk Kim
2016-06-08 17:24 ` [PATCH 2/7] f2fs: avoid reverse IO order for NODE and DATA Jaegeuk Kim
2016-06-08 17:24 ` [PATCH 3/7] f2fs: drop any block plugging Jaegeuk Kim
2016-07-09 2:28 ` [f2fs-dev] " Chao Yu
2016-07-09 16:32 ` Jaegeuk Kim
2016-07-12 1:38 ` Chao Yu [this message]
2016-07-12 17:08 ` Jaegeuk Kim
2016-07-13 1:21 ` hebiao (G)
2016-07-14 2:39 ` Jaegeuk Kim
2016-07-14 6:07 ` hebiao (G)
2016-06-08 17:24 ` [PATCH 4/7] f2fs: skip clean segment for gc Jaegeuk Kim
2016-06-08 17:24 ` [PATCH 5/7] f2fs: introduce force_lfs mount option Jaegeuk Kim
2016-06-08 17:24 ` [PATCH 6/7] f2fs: fix deadlock in add_link failure Jaegeuk Kim
2016-06-08 17:24 ` [PATCH 7/7] f2fs: don't need to flush unlinked dentry pages Jaegeuk Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ead95e1c-9fe4-9895-0653-90868f865137@huawei.com \
--to=yuchao0@huawei.com \
--cc=chen.chun.yen@huawei.com \
--cc=hebiao6@huawei.com \
--cc=jaegeuk@kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).