* [PATCH 2/7] f2fs: avoid reverse IO order for NODE and DATA
2016-06-08 17:24 [PATCH 1/7] f2fs: set mapping error for EIO Jaegeuk Kim
@ 2016-06-08 17:24 ` Jaegeuk Kim
2016-06-08 17:24 ` [PATCH 3/7] f2fs: drop any block plugging Jaegeuk Kim
` (4 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Jaegeuk Kim @ 2016-06-08 17:24 UTC (permalink / raw)
To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim
There is a data race between allocate_data_block() and f2fs_sbumit_page_mbio(),
which incur unnecessary reversed bio submission.
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
fs/f2fs/f2fs.h | 1 +
fs/f2fs/segment.c | 6 ++++++
fs/f2fs/super.c | 2 ++
3 files changed, 9 insertions(+)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index d1702ee..b35b68b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -786,6 +786,7 @@ struct f2fs_sb_info {
/* for bio operations */
struct f2fs_bio_info read_io; /* for read bios */
struct f2fs_bio_info write_io[NR_PAGE_TYPE]; /* for write bios */
+ struct mutex wio_mutex[NODE + 1]; /* bio ordering for NODE/DATA */
/* for checkpoint */
struct f2fs_checkpoint *ckpt; /* raw checkpoint pointer */
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 9011bffd..7b58bfb 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1399,11 +1399,17 @@ static void do_write_page(struct f2fs_summary *sum, struct f2fs_io_info *fio)
{
int type = __get_segment_type(fio->page, fio->type);
+ if (fio->type == NODE || fio->type == DATA)
+ mutex_lock(&fio->sbi->wio_mutex[fio->type]);
+
allocate_data_block(fio->sbi, fio->page, fio->old_blkaddr,
&fio->new_blkaddr, sum, type);
/* writeout dirty page into bdev */
f2fs_submit_page_mbio(fio);
+
+ if (fio->type == NODE || fio->type == DATA)
+ mutex_unlock(&fio->sbi->wio_mutex[fio->type]);
}
void write_meta_page(struct f2fs_sb_info *sbi, struct page *page)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 94bb87d..dc66f16 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1436,6 +1436,8 @@ static void init_sb_info(struct f2fs_sb_info *sbi)
INIT_LIST_HEAD(&sbi->s_list);
mutex_init(&sbi->umount_mutex);
+ mutex_init(&sbi->wio_mutex[NODE]);
+ mutex_init(&sbi->wio_mutex[DATA]);
#ifdef CONFIG_F2FS_FS_ENCRYPTION
memcpy(sbi->key_prefix, F2FS_KEY_DESC_PREFIX,
--
2.8.3
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/7] f2fs: drop any block plugging
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 ` Jaegeuk Kim
2016-07-09 2:28 ` [f2fs-dev] " Chao Yu
2016-06-08 17:24 ` [PATCH 4/7] f2fs: skip clean segment for gc Jaegeuk Kim
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Jaegeuk Kim @ 2016-06-08 17:24 UTC (permalink / raw)
To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim
In f2fs, we don't need to keep block plugging for NODE and DATA writes, since
we already merged bios as much as possible.
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);
}
--
2.8.3
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
2016-06-08 17:24 ` [PATCH 3/7] f2fs: drop any block plugging Jaegeuk Kim
@ 2016-07-09 2:28 ` Chao Yu
2016-07-09 16:32 ` Jaegeuk Kim
0 siblings, 1 reply; 14+ messages in thread
From: Chao Yu @ 2016-07-09 2:28 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel, linux-fsdevel, linux-f2fs-devel
Cc: chen.chun.yen, hebiao6
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?
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);
> }
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
2016-07-09 2:28 ` [f2fs-dev] " Chao Yu
@ 2016-07-09 16:32 ` Jaegeuk Kim
2016-07-12 1:38 ` Chao Yu
0 siblings, 1 reply; 14+ messages in thread
From: Jaegeuk Kim @ 2016-07-09 16:32 UTC (permalink / raw)
To: Chao Yu
Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel, chen.chun.yen, hebiao6
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@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);
> > }
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
2016-07-09 16:32 ` Jaegeuk Kim
@ 2016-07-12 1:38 ` Chao Yu
2016-07-12 17:08 ` Jaegeuk Kim
0 siblings, 1 reply; 14+ messages in thread
From: Chao Yu @ 2016-07-12 1:38 UTC (permalink / raw)
To: Jaegeuk Kim
Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel, chen.chun.yen, hebiao6
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);
>>> }
>>>
>
> .
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
2016-07-12 1:38 ` Chao Yu
@ 2016-07-12 17:08 ` Jaegeuk Kim
2016-07-13 1:21 ` hebiao (G)
0 siblings, 1 reply; 14+ messages in thread
From: Jaegeuk Kim @ 2016-07-12 17:08 UTC (permalink / raw)
To: Chao Yu
Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel, chen.chun.yen, hebiao6
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@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);
> >>> }
> >>>
> >
> > .
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
2016-07-12 17:08 ` Jaegeuk Kim
@ 2016-07-13 1:21 ` hebiao (G)
2016-07-14 2:39 ` Jaegeuk Kim
0 siblings, 1 reply; 14+ messages in thread
From: hebiao (G) @ 2016-07-13 1:21 UTC (permalink / raw)
To: Jaegeuk Kim, Yuchao (T)
Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel,
CHEN CHUN YEN (IAN),
Kongfei
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@kernel.org]
Sent: 2016年7月13日 1:08
To: Yuchao (T) <yuchao0@huawei.com>
Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net; CHEN CHUN YEN (IAN) <chen.chun.yen@huawei.com>; hebiao (G) <hebiao6@huawei.com>
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@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);
> >>> }
> >>>
> >
> > .
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
2016-07-13 1:21 ` hebiao (G)
@ 2016-07-14 2:39 ` Jaegeuk Kim
2016-07-14 6:07 ` hebiao (G)
0 siblings, 1 reply; 14+ messages in thread
From: Jaegeuk Kim @ 2016-07-14 2:39 UTC (permalink / raw)
To: hebiao (G)
Cc: Yuchao (T),
linux-kernel, linux-fsdevel, linux-f2fs-devel,
CHEN CHUN YEN (IAN),
Kongfei
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@kernel.org]
> Sent: 2016年7月13日 1:08
> To: Yuchao (T) <yuchao0@huawei.com>
> Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net; CHEN CHUN YEN (IAN) <chen.chun.yen@huawei.com>; hebiao (G) <hebiao6@huawei.com>
> 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@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);
> > >>> }
> > >>>
> > >
> > > .
> > >
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
2016-07-14 2:39 ` Jaegeuk Kim
@ 2016-07-14 6:07 ` hebiao (G)
0 siblings, 0 replies; 14+ messages in thread
From: hebiao (G) @ 2016-07-14 6:07 UTC (permalink / raw)
To: Jaegeuk Kim
Cc: Yuchao (T),
linux-kernel, linux-fsdevel, linux-f2fs-devel, Kongfei,
CHEN CHUN YEN (IAN)
Thanks Kim, it'll be very meaningful in block layer :-)
-----Original Message-----
From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
Sent: 2016年7月14日 10:40
To: hebiao (G) <hebiao6@huawei.com>
Cc: Yuchao (T) <yuchao0@huawei.com>; linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net; CHEN CHUN YEN (IAN) <chen.chun.yen@huawei.com>; Kongfei <kongfei@hisilicon.com>
Subject: Re: [f2fs-dev] [PATCH 3/7] f2fs: drop any block plugging
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@kernel.org]
> Sent: 2016年7月13日 1:08
> To: Yuchao (T) <yuchao0@huawei.com>
> Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net; CHEN CHUN YEN (IAN)
> <chen.chun.yen@huawei.com>; hebiao (G) <hebiao6@huawei.com>
> 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@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);
> > >>> }
> > >>>
> > >
> > > .
> > >
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/7] f2fs: skip clean segment for gc
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-06-08 17:24 ` Jaegeuk Kim
2016-06-08 17:24 ` [PATCH 5/7] f2fs: introduce force_lfs mount option Jaegeuk Kim
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Jaegeuk Kim @ 2016-06-08 17:24 UTC (permalink / raw)
To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim
If a segment in a section is clean or prefreed, we don't need to get its summary
and do gc.
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
fs/f2fs/gc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 67fd285..e1d274c 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -795,6 +795,10 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
}
for (segno = start_segno; segno < end_segno; segno++) {
+
+ if (get_valid_blocks(sbi, segno, 1) == 0)
+ continue;
+
/* find segment summary of victim */
sum_page = find_get_page(META_MAPPING(sbi),
GET_SUM_BLOCK(sbi, segno));
--
2.8.3
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/7] f2fs: introduce force_lfs mount option
2016-06-08 17:24 [PATCH 1/7] f2fs: set mapping error for EIO Jaegeuk Kim
` (2 preceding siblings ...)
2016-06-08 17:24 ` [PATCH 4/7] f2fs: skip clean segment for gc Jaegeuk Kim
@ 2016-06-08 17:24 ` 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
5 siblings, 0 replies; 14+ messages in thread
From: Jaegeuk Kim @ 2016-06-08 17:24 UTC (permalink / raw)
To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim
This mount option is to enable original log-structured filesystem forcefully.
So, there should be no random writes for main area.
Especially, this supports host-managed SMR device.
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
Documentation/filesystems/f2fs.txt | 3 +++
fs/f2fs/data.c | 2 ++
fs/f2fs/f2fs.h | 2 ++
fs/f2fs/file.c | 8 +++++++-
fs/f2fs/segment.c | 20 +++++++++++++++++++-
fs/f2fs/segment.h | 7 +++++++
fs/f2fs/super.c | 28 ++++++++++++++++++++++++++++
7 files changed, 68 insertions(+), 2 deletions(-)
diff --git a/Documentation/filesystems/f2fs.txt b/Documentation/filesystems/f2fs.txt
index e1c9f08..3a5ce24 100644
--- a/Documentation/filesystems/f2fs.txt
+++ b/Documentation/filesystems/f2fs.txt
@@ -151,6 +151,9 @@ noinline_data Disable the inline data feature, inline data feature is
enabled by default.
data_flush Enable data flushing before checkpoint in order to
persist data of regular and symlink.
+mode=%s Control block allocation mode which supports "adaptive"
+ and "lfs". In "lfs" mode, there should be no random
+ writes towards main area.
================================================================================
DEBUGFS ENTRIES
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 5f655d0..607ef43 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1710,6 +1710,8 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
if (f2fs_encrypted_inode(inode) && S_ISREG(inode->i_mode))
return 0;
+ if (test_opt(F2FS_I_SB(inode), LFS))
+ return 0;
trace_f2fs_direct_IO_enter(inode, offset, count, iov_iter_rw(iter));
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index b35b68b..d919294 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -111,6 +111,8 @@ static inline bool time_to_inject(int type)
#define F2FS_MOUNT_FORCE_FG_GC 0x00004000
#define F2FS_MOUNT_DATA_FLUSH 0x00008000
#define F2FS_MOUNT_FAULT_INJECTION 0x00010000
+#define F2FS_MOUNT_ADAPTIVE 0x00020000
+#define F2FS_MOUNT_LFS 0x00040000
#define clear_opt(sbi, option) (sbi->mount_opt.opt &= ~F2FS_MOUNT_##option)
#define set_opt(sbi, option) (sbi->mount_opt.opt |= F2FS_MOUNT_##option)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index e66d223..f1f78fb 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -917,9 +917,15 @@ static int __exchange_data_block(struct inode *inode, pgoff_t src,
return full ? truncate_hole(inode, dst, dst + 1) : 0;
if (do_replace) {
- struct page *ipage = get_node_page(sbi, inode->i_ino);
+ struct page *ipage;
struct node_info ni;
+ if (test_opt(sbi, LFS)) {
+ ret = -ENOTSUPP;
+ goto err_out;
+ }
+
+ ipage = get_node_page(sbi, inode->i_ino);
if (IS_ERR(ipage)) {
ret = PTR_ERR(ipage);
goto err_out;
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index eff046a..9ff4e30 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -594,6 +594,9 @@ bool discard_next_dnode(struct f2fs_sb_info *sbi, block_t blkaddr)
{
int err = -EOPNOTSUPP;
+ if (test_opt(sbi, LFS))
+ return false;
+
if (test_opt(sbi, DISCARD)) {
struct seg_entry *se = get_seg_entry(sbi,
GET_SEGNO(sbi, blkaddr));
@@ -707,6 +710,7 @@ void clear_prefree_segments(struct f2fs_sb_info *sbi, struct cp_control *cpc)
struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
unsigned long *prefree_map = dirty_i->dirty_segmap[PRE];
unsigned int start = 0, end = -1;
+ unsigned int secno, start_segno;
mutex_lock(&dirty_i->seglist_lock);
@@ -726,8 +730,22 @@ void clear_prefree_segments(struct f2fs_sb_info *sbi, struct cp_control *cpc)
if (!test_opt(sbi, DISCARD))
continue;
- f2fs_issue_discard(sbi, START_BLOCK(sbi, start),
+ if (!test_opt(sbi, LFS) || sbi->segs_per_sec == 1) {
+ f2fs_issue_discard(sbi, START_BLOCK(sbi, start),
(end - start) << sbi->log_blocks_per_seg);
+ continue;
+ }
+next:
+ secno = GET_SECNO(sbi, start);
+ start_segno = secno * sbi->segs_per_sec;
+ if (!IS_CURSEC(sbi, secno) &&
+ !get_valid_blocks(sbi, start, sbi->segs_per_sec))
+ f2fs_issue_discard(sbi, START_BLOCK(sbi, start_segno),
+ sbi->segs_per_sec << sbi->log_blocks_per_seg);
+
+ start = start_segno + sbi->segs_per_sec;
+ if (start < end)
+ goto next;
}
mutex_unlock(&dirty_i->seglist_lock);
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 890bb28d..d74cc33 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -470,6 +470,10 @@ static inline bool need_SSR(struct f2fs_sb_info *sbi)
{
int node_secs = get_blocktype_secs(sbi, F2FS_DIRTY_NODES);
int dent_secs = get_blocktype_secs(sbi, F2FS_DIRTY_DENTS);
+
+ if (test_opt(sbi, LFS))
+ return false;
+
return free_sections(sbi) <= (node_secs + 2 * dent_secs +
reserved_sections(sbi) + 1);
}
@@ -533,6 +537,9 @@ static inline bool need_inplace_update(struct inode *inode)
if (S_ISDIR(inode->i_mode) || f2fs_is_atomic_file(inode))
return false;
+ if (test_opt(sbi, LFS))
+ return false;
+
if (policy & (0x1 << F2FS_IPU_FORCE))
return true;
if (policy & (0x1 << F2FS_IPU_SSR) && need_SSR(sbi))
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index dc66f16..edc736d 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -94,6 +94,7 @@ enum {
Opt_noextent_cache,
Opt_noinline_data,
Opt_data_flush,
+ Opt_mode,
Opt_fault_injection,
Opt_lazytime,
Opt_nolazytime,
@@ -123,6 +124,7 @@ static match_table_t f2fs_tokens = {
{Opt_noextent_cache, "noextent_cache"},
{Opt_noinline_data, "noinline_data"},
{Opt_data_flush, "data_flush"},
+ {Opt_mode, "mode=%s"},
{Opt_fault_injection, "fault_injection=%u"},
{Opt_lazytime, "lazytime"},
{Opt_nolazytime, "nolazytime"},
@@ -506,6 +508,25 @@ static int parse_options(struct super_block *sb, char *options)
case Opt_data_flush:
set_opt(sbi, DATA_FLUSH);
break;
+ case Opt_mode:
+ name = match_strdup(&args[0]);
+
+ if (!name)
+ return -ENOMEM;
+ if (strlen(name) == 8 &&
+ !strncmp(name, "adaptive", 8)) {
+ set_opt(sbi, ADAPTIVE);
+ clear_opt(sbi, LFS);
+ } else if (strlen(name) == 3 &&
+ !strncmp(name, "lfs", 3)) {
+ clear_opt(sbi, ADAPTIVE);
+ set_opt(sbi, LFS);
+ } else {
+ kfree(name);
+ return -EINVAL;
+ }
+ kfree(name);
+ break;
case Opt_fault_injection:
if (args->from && match_int(args, &arg))
return -EINVAL;
@@ -870,6 +891,12 @@ static int f2fs_show_options(struct seq_file *seq, struct dentry *root)
seq_puts(seq, ",noextent_cache");
if (test_opt(sbi, DATA_FLUSH))
seq_puts(seq, ",data_flush");
+
+ seq_puts(seq, ",mode=");
+ if (test_opt(sbi, ADAPTIVE))
+ seq_puts(seq, "adaptive");
+ else if (test_opt(sbi, LFS))
+ seq_puts(seq, "lfs");
seq_printf(seq, ",active_logs=%u", sbi->active_logs);
return 0;
@@ -953,6 +980,7 @@ static void default_options(struct f2fs_sb_info *sbi)
set_opt(sbi, EXTENT_CACHE);
sbi->sb->s_flags |= MS_LAZYTIME;
set_opt(sbi, FLUSH_MERGE);
+ set_opt(sbi, ADAPTIVE);
#ifdef CONFIG_F2FS_FS_XATTR
set_opt(sbi, XATTR_USER);
--
2.8.3
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 6/7] f2fs: fix deadlock in add_link failure
2016-06-08 17:24 [PATCH 1/7] f2fs: set mapping error for EIO Jaegeuk Kim
` (3 preceding siblings ...)
2016-06-08 17:24 ` [PATCH 5/7] f2fs: introduce force_lfs mount option Jaegeuk Kim
@ 2016-06-08 17:24 ` Jaegeuk Kim
2016-06-08 17:24 ` [PATCH 7/7] f2fs: don't need to flush unlinked dentry pages Jaegeuk Kim
5 siblings, 0 replies; 14+ messages in thread
From: Jaegeuk Kim @ 2016-06-08 17:24 UTC (permalink / raw)
To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim
mkdir sync_dirty_inode
- init_inode_metadata
- lock_page(node)
- make_empty_dir
- filemap_fdatawrite()
- do_writepages
- lock_page(data)
- write_page(data)
- lock_page(node)
- f2fs_init_acl
- error
- truncate_inode_pages
- lock_page(data)
So, we don't need to truncate data pages in this error case, which will
be done by f2fs_evict_inode.
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
fs/f2fs/dir.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index f6ab3c2..4864824 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -450,9 +450,6 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
return page;
put_error:
- /* truncate empty dir pages */
- truncate_inode_pages(&inode->i_data, 0);
-
clear_nlink(inode);
update_inode(inode, page);
f2fs_put_page(page, 1);
--
2.8.3
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 7/7] f2fs: don't need to flush unlinked dentry pages
2016-06-08 17:24 [PATCH 1/7] f2fs: set mapping error for EIO Jaegeuk Kim
` (4 preceding siblings ...)
2016-06-08 17:24 ` [PATCH 6/7] f2fs: fix deadlock in add_link failure Jaegeuk Kim
@ 2016-06-08 17:24 ` Jaegeuk Kim
5 siblings, 0 replies; 14+ messages in thread
From: Jaegeuk Kim @ 2016-06-08 17:24 UTC (permalink / raw)
To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim
We don't need to flush any dentry pages used by unlinked directory.
They will be truncated by f2fs_evict_inode.
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
fs/f2fs/checkpoint.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 4179c7b..e6714cb 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -846,7 +846,8 @@ retry:
inode = igrab(&fi->vfs_inode);
spin_unlock(&sbi->inode_lock[type]);
if (inode) {
- filemap_fdatawrite(inode->i_mapping);
+ if (inode->i_nlink)
+ filemap_fdatawrite(inode->i_mapping);
iput(inode);
} else {
/*
--
2.8.3
^ permalink raw reply related [flat|nested] 14+ messages in thread