* [PATCH] f2fs: remove a waiter for checkpoint completion @ 2020-08-03 17:28 Jaegeuk Kim 2020-08-04 0:55 ` [f2fs-dev] " Chao Yu 0 siblings, 1 reply; 6+ messages in thread From: Jaegeuk Kim @ 2020-08-03 17:28 UTC (permalink / raw) To: linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Jaegeuk Kim, Eric Biggers It doesn't need to wait for checkpoint being completed triggered by end_io. [ 20.157753] ------------[ cut here ]------------ [ 20.158393] do not call blocking ops when !TASK_RUNNING; state=2 set at [<0000000096354225>] prepare_to_wait+0xcd/0x430 [ 20.159858] WARNING: CPU: 1 PID: 1152 at kernel/sched/core.c:7142 __might_sleep+0x149/0x1a0 ... [ 20.176110] __submit_merged_write_cond+0x191/0x310 [ 20.176739] f2fs_submit_merged_write+0x18/0x20 [ 20.177323] f2fs_wait_on_all_pages+0x269/0x2d0 [ 20.177899] ? block_operations+0x980/0x980 [ 20.178441] ? __kasan_check_read+0x11/0x20 [ 20.178975] ? finish_wait+0x260/0x260 [ 20.179488] ? percpu_counter_set+0x147/0x230 [ 20.180049] do_checkpoint+0x1757/0x2a50 [ 20.180558] f2fs_write_checkpoint+0x840/0xaf0 [ 20.181126] f2fs_sync_fs+0x287/0x4a0 Reported-by: Eric Biggers <ebiggers@kernel.org> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/checkpoint.c | 6 +----- fs/f2fs/data.c | 4 ---- fs/f2fs/f2fs.h | 1 - fs/f2fs/super.c | 1 - 4 files changed, 1 insertion(+), 11 deletions(-) diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 99c8061da55b9..2bdddc725e677 100644 --- a/fs/f2fs/checkpoint.c +++ b/fs/f2fs/checkpoint.c @@ -1255,11 +1255,7 @@ static void unblock_operations(struct f2fs_sb_info *sbi) void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) { - DEFINE_WAIT(wait); - for (;;) { - prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE); - if (!get_pages(sbi, type)) break; @@ -1271,9 +1267,9 @@ void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) FS_CP_META_IO); else if (type == F2FS_WB_CP_DATA) f2fs_submit_merged_write(sbi, DATA); + io_schedule_timeout(DEFAULT_IO_TIMEOUT); } - finish_wait(&sbi->cp_wait, &wait); } static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc) diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index c1b676be67b9a..588d2871bbe69 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -383,10 +383,6 @@ static void f2fs_write_end_io(struct bio *bio) clear_cold_data(page); end_page_writeback(page); } - if (!get_pages(sbi, F2FS_WB_CP_DATA) && - wq_has_sleeper(&sbi->cp_wait)) - wake_up(&sbi->cp_wait); - bio_put(bio); } diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 02811ce15059b..a036539363030 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -1437,7 +1437,6 @@ struct f2fs_sb_info { struct rw_semaphore cp_rwsem; /* blocking FS operations */ struct rw_semaphore node_write; /* locking node writes */ struct rw_semaphore node_change; /* locking node change */ - wait_queue_head_t cp_wait; unsigned long last_time[MAX_TIME]; /* to store time in jiffies */ long interval_time[MAX_TIME]; /* to store thresholds */ diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 83bf9a02f83f2..5f4d9abc079d6 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -3540,7 +3540,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) init_rwsem(&sbi->cp_rwsem); init_rwsem(&sbi->quota_sem); - init_waitqueue_head(&sbi->cp_wait); init_sb_info(sbi); err = init_percpu_info(sbi); -- 2.28.0.163.g6104cc2f0b6-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: remove a waiter for checkpoint completion 2020-08-03 17:28 [PATCH] f2fs: remove a waiter for checkpoint completion Jaegeuk Kim @ 2020-08-04 0:55 ` Chao Yu 2020-08-04 1:04 ` Jaegeuk Kim 0 siblings, 1 reply; 6+ messages in thread From: Chao Yu @ 2020-08-04 0:55 UTC (permalink / raw) To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Eric Biggers On 2020/8/4 1:28, Jaegeuk Kim wrote: > It doesn't need to wait for checkpoint being completed triggered by end_io. > > [ 20.157753] ------------[ cut here ]------------ > [ 20.158393] do not call blocking ops when !TASK_RUNNING; state=2 set at [<0000000096354225>] prepare_to_wait+0xcd/0x430 > [ 20.159858] WARNING: CPU: 1 PID: 1152 at kernel/sched/core.c:7142 __might_sleep+0x149/0x1a0 > ... > [ 20.176110] __submit_merged_write_cond+0x191/0x310 > [ 20.176739] f2fs_submit_merged_write+0x18/0x20 > [ 20.177323] f2fs_wait_on_all_pages+0x269/0x2d0 > [ 20.177899] ? block_operations+0x980/0x980 > [ 20.178441] ? __kasan_check_read+0x11/0x20 > [ 20.178975] ? finish_wait+0x260/0x260 > [ 20.179488] ? percpu_counter_set+0x147/0x230 > [ 20.180049] do_checkpoint+0x1757/0x2a50 > [ 20.180558] f2fs_write_checkpoint+0x840/0xaf0 > [ 20.181126] f2fs_sync_fs+0x287/0x4a0 > > Reported-by: Eric Biggers <ebiggers@kernel.org> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > fs/f2fs/checkpoint.c | 6 +----- > fs/f2fs/data.c | 4 ---- > fs/f2fs/f2fs.h | 1 - > fs/f2fs/super.c | 1 - > 4 files changed, 1 insertion(+), 11 deletions(-) > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > index 99c8061da55b9..2bdddc725e677 100644 > --- a/fs/f2fs/checkpoint.c > +++ b/fs/f2fs/checkpoint.c > @@ -1255,11 +1255,7 @@ static void unblock_operations(struct f2fs_sb_info *sbi) > > void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) > { > - DEFINE_WAIT(wait); > - > for (;;) { > - prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE); Wouldn't that case high cpu usage before io end? > - > if (!get_pages(sbi, type)) > break; > > @@ -1271,9 +1267,9 @@ void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) > FS_CP_META_IO); > else if (type == F2FS_WB_CP_DATA) > f2fs_submit_merged_write(sbi, DATA); > + > io_schedule_timeout(DEFAULT_IO_TIMEOUT); > } > - finish_wait(&sbi->cp_wait, &wait); > } > > static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc) > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index c1b676be67b9a..588d2871bbe69 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -383,10 +383,6 @@ static void f2fs_write_end_io(struct bio *bio) > clear_cold_data(page); > end_page_writeback(page); > } > - if (!get_pages(sbi, F2FS_WB_CP_DATA) && > - wq_has_sleeper(&sbi->cp_wait)) > - wake_up(&sbi->cp_wait); > - > bio_put(bio); > } > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 02811ce15059b..a036539363030 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1437,7 +1437,6 @@ struct f2fs_sb_info { > struct rw_semaphore cp_rwsem; /* blocking FS operations */ > struct rw_semaphore node_write; /* locking node writes */ > struct rw_semaphore node_change; /* locking node change */ > - wait_queue_head_t cp_wait; > unsigned long last_time[MAX_TIME]; /* to store time in jiffies */ > long interval_time[MAX_TIME]; /* to store thresholds */ > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 83bf9a02f83f2..5f4d9abc079d6 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -3540,7 +3540,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > > init_rwsem(&sbi->cp_rwsem); > init_rwsem(&sbi->quota_sem); > - init_waitqueue_head(&sbi->cp_wait); > init_sb_info(sbi); > > err = init_percpu_info(sbi); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: remove a waiter for checkpoint completion 2020-08-04 0:55 ` [f2fs-dev] " Chao Yu @ 2020-08-04 1:04 ` Jaegeuk Kim 2020-08-04 1:17 ` Chao Yu 0 siblings, 1 reply; 6+ messages in thread From: Jaegeuk Kim @ 2020-08-04 1:04 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Eric Biggers On 08/04, Chao Yu wrote: > On 2020/8/4 1:28, Jaegeuk Kim wrote: > > It doesn't need to wait for checkpoint being completed triggered by end_io. > > > > [ 20.157753] ------------[ cut here ]------------ > > [ 20.158393] do not call blocking ops when !TASK_RUNNING; state=2 set at [<0000000096354225>] prepare_to_wait+0xcd/0x430 > > [ 20.159858] WARNING: CPU: 1 PID: 1152 at kernel/sched/core.c:7142 __might_sleep+0x149/0x1a0 > > ... > > [ 20.176110] __submit_merged_write_cond+0x191/0x310 > > [ 20.176739] f2fs_submit_merged_write+0x18/0x20 > > [ 20.177323] f2fs_wait_on_all_pages+0x269/0x2d0 > > [ 20.177899] ? block_operations+0x980/0x980 > > [ 20.178441] ? __kasan_check_read+0x11/0x20 > > [ 20.178975] ? finish_wait+0x260/0x260 > > [ 20.179488] ? percpu_counter_set+0x147/0x230 > > [ 20.180049] do_checkpoint+0x1757/0x2a50 > > [ 20.180558] f2fs_write_checkpoint+0x840/0xaf0 > > [ 20.181126] f2fs_sync_fs+0x287/0x4a0 > > > > Reported-by: Eric Biggers <ebiggers@kernel.org> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > fs/f2fs/checkpoint.c | 6 +----- > > fs/f2fs/data.c | 4 ---- > > fs/f2fs/f2fs.h | 1 - > > fs/f2fs/super.c | 1 - > > 4 files changed, 1 insertion(+), 11 deletions(-) > > > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > > index 99c8061da55b9..2bdddc725e677 100644 > > --- a/fs/f2fs/checkpoint.c > > +++ b/fs/f2fs/checkpoint.c > > @@ -1255,11 +1255,7 @@ static void unblock_operations(struct f2fs_sb_info *sbi) > > void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) > > { > > - DEFINE_WAIT(wait); > > - > > for (;;) { > > - prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE); > > Wouldn't that case high cpu usage before io end? This is a critical path to wait for IO completion in checkpoint, which would be better to wait for it to avoid long latency to continue filesystem operations. Moreover, I expect io_schedule_timeout() can mitigate such the CPU overhead and actually we don't need to make there-in context switches as well. > > > - > > if (!get_pages(sbi, type)) > > break; > > @@ -1271,9 +1267,9 @@ void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) > > FS_CP_META_IO); > > else if (type == F2FS_WB_CP_DATA) > > f2fs_submit_merged_write(sbi, DATA); > > + > > io_schedule_timeout(DEFAULT_IO_TIMEOUT); > > } > > - finish_wait(&sbi->cp_wait, &wait); > > } > > static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > index c1b676be67b9a..588d2871bbe69 100644 > > --- a/fs/f2fs/data.c > > +++ b/fs/f2fs/data.c > > @@ -383,10 +383,6 @@ static void f2fs_write_end_io(struct bio *bio) > > clear_cold_data(page); > > end_page_writeback(page); > > } > > - if (!get_pages(sbi, F2FS_WB_CP_DATA) && > > - wq_has_sleeper(&sbi->cp_wait)) > > - wake_up(&sbi->cp_wait); > > - > > bio_put(bio); > > } > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > index 02811ce15059b..a036539363030 100644 > > --- a/fs/f2fs/f2fs.h > > +++ b/fs/f2fs/f2fs.h > > @@ -1437,7 +1437,6 @@ struct f2fs_sb_info { > > struct rw_semaphore cp_rwsem; /* blocking FS operations */ > > struct rw_semaphore node_write; /* locking node writes */ > > struct rw_semaphore node_change; /* locking node change */ > > - wait_queue_head_t cp_wait; > > unsigned long last_time[MAX_TIME]; /* to store time in jiffies */ > > long interval_time[MAX_TIME]; /* to store thresholds */ > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > index 83bf9a02f83f2..5f4d9abc079d6 100644 > > --- a/fs/f2fs/super.c > > +++ b/fs/f2fs/super.c > > @@ -3540,7 +3540,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > > init_rwsem(&sbi->cp_rwsem); > > init_rwsem(&sbi->quota_sem); > > - init_waitqueue_head(&sbi->cp_wait); > > init_sb_info(sbi); > > err = init_percpu_info(sbi); > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: remove a waiter for checkpoint completion 2020-08-04 1:04 ` Jaegeuk Kim @ 2020-08-04 1:17 ` Chao Yu 2020-08-04 2:43 ` Jaegeuk Kim 0 siblings, 1 reply; 6+ messages in thread From: Chao Yu @ 2020-08-04 1:17 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Eric Biggers On 2020/8/4 9:04, Jaegeuk Kim wrote: > On 08/04, Chao Yu wrote: >> On 2020/8/4 1:28, Jaegeuk Kim wrote: >>> It doesn't need to wait for checkpoint being completed triggered by end_io. >>> >>> [ 20.157753] ------------[ cut here ]------------ >>> [ 20.158393] do not call blocking ops when !TASK_RUNNING; state=2 set at [<0000000096354225>] prepare_to_wait+0xcd/0x430 >>> [ 20.159858] WARNING: CPU: 1 PID: 1152 at kernel/sched/core.c:7142 __might_sleep+0x149/0x1a0 >>> ... >>> [ 20.176110] __submit_merged_write_cond+0x191/0x310 >>> [ 20.176739] f2fs_submit_merged_write+0x18/0x20 >>> [ 20.177323] f2fs_wait_on_all_pages+0x269/0x2d0 >>> [ 20.177899] ? block_operations+0x980/0x980 >>> [ 20.178441] ? __kasan_check_read+0x11/0x20 >>> [ 20.178975] ? finish_wait+0x260/0x260 >>> [ 20.179488] ? percpu_counter_set+0x147/0x230 >>> [ 20.180049] do_checkpoint+0x1757/0x2a50 >>> [ 20.180558] f2fs_write_checkpoint+0x840/0xaf0 >>> [ 20.181126] f2fs_sync_fs+0x287/0x4a0 >>> >>> Reported-by: Eric Biggers <ebiggers@kernel.org> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>> --- >>> fs/f2fs/checkpoint.c | 6 +----- >>> fs/f2fs/data.c | 4 ---- >>> fs/f2fs/f2fs.h | 1 - >>> fs/f2fs/super.c | 1 - >>> 4 files changed, 1 insertion(+), 11 deletions(-) >>> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c >>> index 99c8061da55b9..2bdddc725e677 100644 >>> --- a/fs/f2fs/checkpoint.c >>> +++ b/fs/f2fs/checkpoint.c >>> @@ -1255,11 +1255,7 @@ static void unblock_operations(struct f2fs_sb_info *sbi) >>> void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) >>> { >>> - DEFINE_WAIT(wait); >>> - >>> for (;;) { >>> - prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE); >> >> Wouldn't that case high cpu usage before io end? > > This is a critical path to wait for IO completion in checkpoint, which would be > better to wait for it to avoid long latency to continue filesystem operations. Yup, in previous implementation, last end_io wakes up checkpoint() waiter, we didn't waste any time there. > Moreover, I expect io_schedule_timeout() can mitigate such the CPU overhead and > actually we don't need to make there-in context switches as well. Then io_schedule_timeout() in this loop may give CPU time slice to other thread until scheduler reselect checkpoint(), that would cause longer latency? > >> >>> - >>> if (!get_pages(sbi, type)) >>> break; >>> @@ -1271,9 +1267,9 @@ void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) >>> FS_CP_META_IO); >>> else if (type == F2FS_WB_CP_DATA) >>> f2fs_submit_merged_write(sbi, DATA); >>> + >>> io_schedule_timeout(DEFAULT_IO_TIMEOUT); >>> } >>> - finish_wait(&sbi->cp_wait, &wait); >>> } >>> static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc) >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>> index c1b676be67b9a..588d2871bbe69 100644 >>> --- a/fs/f2fs/data.c >>> +++ b/fs/f2fs/data.c >>> @@ -383,10 +383,6 @@ static void f2fs_write_end_io(struct bio *bio) >>> clear_cold_data(page); >>> end_page_writeback(page); >>> } >>> - if (!get_pages(sbi, F2FS_WB_CP_DATA) && >>> - wq_has_sleeper(&sbi->cp_wait)) >>> - wake_up(&sbi->cp_wait); >>> - >>> bio_put(bio); >>> } >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>> index 02811ce15059b..a036539363030 100644 >>> --- a/fs/f2fs/f2fs.h >>> +++ b/fs/f2fs/f2fs.h >>> @@ -1437,7 +1437,6 @@ struct f2fs_sb_info { >>> struct rw_semaphore cp_rwsem; /* blocking FS operations */ >>> struct rw_semaphore node_write; /* locking node writes */ >>> struct rw_semaphore node_change; /* locking node change */ >>> - wait_queue_head_t cp_wait; >>> unsigned long last_time[MAX_TIME]; /* to store time in jiffies */ >>> long interval_time[MAX_TIME]; /* to store thresholds */ >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>> index 83bf9a02f83f2..5f4d9abc079d6 100644 >>> --- a/fs/f2fs/super.c >>> +++ b/fs/f2fs/super.c >>> @@ -3540,7 +3540,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) >>> init_rwsem(&sbi->cp_rwsem); >>> init_rwsem(&sbi->quota_sem); >>> - init_waitqueue_head(&sbi->cp_wait); >>> init_sb_info(sbi); >>> err = init_percpu_info(sbi); >>> > . > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: remove a waiter for checkpoint completion 2020-08-04 1:17 ` Chao Yu @ 2020-08-04 2:43 ` Jaegeuk Kim 2020-08-04 3:00 ` Chao Yu 0 siblings, 1 reply; 6+ messages in thread From: Jaegeuk Kim @ 2020-08-04 2:43 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Eric Biggers On 08/04, Chao Yu wrote: > On 2020/8/4 9:04, Jaegeuk Kim wrote: > > On 08/04, Chao Yu wrote: > > > On 2020/8/4 1:28, Jaegeuk Kim wrote: > > > > It doesn't need to wait for checkpoint being completed triggered by end_io. > > > > > > > > [ 20.157753] ------------[ cut here ]------------ > > > > [ 20.158393] do not call blocking ops when !TASK_RUNNING; state=2 set at [<0000000096354225>] prepare_to_wait+0xcd/0x430 > > > > [ 20.159858] WARNING: CPU: 1 PID: 1152 at kernel/sched/core.c:7142 __might_sleep+0x149/0x1a0 > > > > ... > > > > [ 20.176110] __submit_merged_write_cond+0x191/0x310 > > > > [ 20.176739] f2fs_submit_merged_write+0x18/0x20 > > > > [ 20.177323] f2fs_wait_on_all_pages+0x269/0x2d0 > > > > [ 20.177899] ? block_operations+0x980/0x980 > > > > [ 20.178441] ? __kasan_check_read+0x11/0x20 > > > > [ 20.178975] ? finish_wait+0x260/0x260 > > > > [ 20.179488] ? percpu_counter_set+0x147/0x230 > > > > [ 20.180049] do_checkpoint+0x1757/0x2a50 > > > > [ 20.180558] f2fs_write_checkpoint+0x840/0xaf0 > > > > [ 20.181126] f2fs_sync_fs+0x287/0x4a0 > > > > > > > > Reported-by: Eric Biggers <ebiggers@kernel.org> > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > --- > > > > fs/f2fs/checkpoint.c | 6 +----- > > > > fs/f2fs/data.c | 4 ---- > > > > fs/f2fs/f2fs.h | 1 - > > > > fs/f2fs/super.c | 1 - > > > > 4 files changed, 1 insertion(+), 11 deletions(-) > > > > > > > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > > > > index 99c8061da55b9..2bdddc725e677 100644 > > > > --- a/fs/f2fs/checkpoint.c > > > > +++ b/fs/f2fs/checkpoint.c > > > > @@ -1255,11 +1255,7 @@ static void unblock_operations(struct f2fs_sb_info *sbi) > > > > void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) > > > > { > > > > - DEFINE_WAIT(wait); > > > > - > > > > for (;;) { > > > > - prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE); > > > > > > Wouldn't that case high cpu usage before io end? > > > > This is a critical path to wait for IO completion in checkpoint, which would be > > better to wait for it to avoid long latency to continue filesystem operations. > > Yup, in previous implementation, last end_io wakes up checkpoint() waiter, we > didn't waste any time there. > > > Moreover, I expect io_schedule_timeout() can mitigate such the CPU overhead and > > actually we don't need to make there-in context switches as well. > > Then io_schedule_timeout() in this loop may give CPU time slice to other thread > until scheduler reselect checkpoint(), that would cause longer latency? Hmm, how about this then? From 4956afa1cedc019cabf6e8bff7bc48d3bcf7a3f5 Mon Sep 17 00:00:00 2001 From: Jaegeuk Kim <jaegeuk@kernel.org> Date: Mon, 3 Aug 2020 19:37:12 -0700 Subject: [PATCH] f2fs: prepare a waiter before entering io_schedule This is to avoid sleep() in the waiter thread. [ 20.157753] ------------[ cut here ]------------ [ 20.158393] do not call blocking ops when !TASK_RUNNING; state=2 set at [<0000000096354225>] prepare_to_wait+0xcd/0x430 [ 20.159858] WARNING: CPU: 1 PID: 1152 at kernel/sched/core.c:7142 __might_sleep+0x149/0x1a0 ... [ 20.176110] __submit_merged_write_cond+0x191/0x310 [ 20.176739] f2fs_submit_merged_write+0x18/0x20 [ 20.177323] f2fs_wait_on_all_pages+0x269/0x2d0 [ 20.177899] ? block_operations+0x980/0x980 [ 20.178441] ? __kasan_check_read+0x11/0x20 [ 20.178975] ? finish_wait+0x260/0x260 [ 20.179488] ? percpu_counter_set+0x147/0x230 [ 20.180049] do_checkpoint+0x1757/0x2a50 [ 20.180558] f2fs_write_checkpoint+0x840/0xaf0 [ 20.181126] f2fs_sync_fs+0x287/0x4a0 Reported-by: Eric Biggers <ebiggers@kernel.org> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/checkpoint.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 99c8061da55b9..ff807e14c8911 100644 --- a/fs/f2fs/checkpoint.c +++ b/fs/f2fs/checkpoint.c @@ -1258,8 +1258,6 @@ void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) DEFINE_WAIT(wait); for (;;) { - prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE); - if (!get_pages(sbi, type)) break; @@ -1271,6 +1269,8 @@ void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) FS_CP_META_IO); else if (type == F2FS_WB_CP_DATA) f2fs_submit_merged_write(sbi, DATA); + + prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE); io_schedule_timeout(DEFAULT_IO_TIMEOUT); } finish_wait(&sbi->cp_wait, &wait); -- 2.28.0.163.g6104cc2f0b6-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: remove a waiter for checkpoint completion 2020-08-04 2:43 ` Jaegeuk Kim @ 2020-08-04 3:00 ` Chao Yu 0 siblings, 0 replies; 6+ messages in thread From: Chao Yu @ 2020-08-04 3:00 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Eric Biggers On 2020/8/4 10:43, Jaegeuk Kim wrote: > On 08/04, Chao Yu wrote: >> On 2020/8/4 9:04, Jaegeuk Kim wrote: >>> On 08/04, Chao Yu wrote: >>>> On 2020/8/4 1:28, Jaegeuk Kim wrote: >>>>> It doesn't need to wait for checkpoint being completed triggered by end_io. >>>>> >>>>> [ 20.157753] ------------[ cut here ]------------ >>>>> [ 20.158393] do not call blocking ops when !TASK_RUNNING; state=2 set at [<0000000096354225>] prepare_to_wait+0xcd/0x430 >>>>> [ 20.159858] WARNING: CPU: 1 PID: 1152 at kernel/sched/core.c:7142 __might_sleep+0x149/0x1a0 >>>>> ... >>>>> [ 20.176110] __submit_merged_write_cond+0x191/0x310 >>>>> [ 20.176739] f2fs_submit_merged_write+0x18/0x20 >>>>> [ 20.177323] f2fs_wait_on_all_pages+0x269/0x2d0 >>>>> [ 20.177899] ? block_operations+0x980/0x980 >>>>> [ 20.178441] ? __kasan_check_read+0x11/0x20 >>>>> [ 20.178975] ? finish_wait+0x260/0x260 >>>>> [ 20.179488] ? percpu_counter_set+0x147/0x230 >>>>> [ 20.180049] do_checkpoint+0x1757/0x2a50 >>>>> [ 20.180558] f2fs_write_checkpoint+0x840/0xaf0 >>>>> [ 20.181126] f2fs_sync_fs+0x287/0x4a0 >>>>> >>>>> Reported-by: Eric Biggers <ebiggers@kernel.org> >>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>>>> --- >>>>> fs/f2fs/checkpoint.c | 6 +----- >>>>> fs/f2fs/data.c | 4 ---- >>>>> fs/f2fs/f2fs.h | 1 - >>>>> fs/f2fs/super.c | 1 - >>>>> 4 files changed, 1 insertion(+), 11 deletions(-) >>>>> >>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c >>>>> index 99c8061da55b9..2bdddc725e677 100644 >>>>> --- a/fs/f2fs/checkpoint.c >>>>> +++ b/fs/f2fs/checkpoint.c >>>>> @@ -1255,11 +1255,7 @@ static void unblock_operations(struct f2fs_sb_info *sbi) >>>>> void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) >>>>> { >>>>> - DEFINE_WAIT(wait); >>>>> - >>>>> for (;;) { >>>>> - prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE); >>>> >>>> Wouldn't that case high cpu usage before io end? >>> >>> This is a critical path to wait for IO completion in checkpoint, which would be >>> better to wait for it to avoid long latency to continue filesystem operations. >> >> Yup, in previous implementation, last end_io wakes up checkpoint() waiter, we >> didn't waste any time there. >> >>> Moreover, I expect io_schedule_timeout() can mitigate such the CPU overhead and >>> actually we don't need to make there-in context switches as well. >> >> Then io_schedule_timeout() in this loop may give CPU time slice to other thread >> until scheduler reselect checkpoint(), that would cause longer latency? > > Hmm, how about this then? > >>From 4956afa1cedc019cabf6e8bff7bc48d3bcf7a3f5 Mon Sep 17 00:00:00 2001 > From: Jaegeuk Kim <jaegeuk@kernel.org> > Date: Mon, 3 Aug 2020 19:37:12 -0700 > Subject: [PATCH] f2fs: prepare a waiter before entering io_schedule > > This is to avoid sleep() in the waiter thread. > > [ 20.157753] ------------[ cut here ]------------ > [ 20.158393] do not call blocking ops when !TASK_RUNNING; state=2 set at [<0000000096354225>] prepare_to_wait+0xcd/0x430 > [ 20.159858] WARNING: CPU: 1 PID: 1152 at kernel/sched/core.c:7142 __might_sleep+0x149/0x1a0 > ... > [ 20.176110] __submit_merged_write_cond+0x191/0x310 > [ 20.176739] f2fs_submit_merged_write+0x18/0x20 > [ 20.177323] f2fs_wait_on_all_pages+0x269/0x2d0 > [ 20.177899] ? block_operations+0x980/0x980 > [ 20.178441] ? __kasan_check_read+0x11/0x20 > [ 20.178975] ? finish_wait+0x260/0x260 > [ 20.179488] ? percpu_counter_set+0x147/0x230 > [ 20.180049] do_checkpoint+0x1757/0x2a50 > [ 20.180558] f2fs_write_checkpoint+0x840/0xaf0 > [ 20.181126] f2fs_sync_fs+0x287/0x4a0 > > Reported-by: Eric Biggers <ebiggers@kernel.org> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> Better, Reviewed-by: Chao Yu <yuchao0@huawei.com> Thanks, > --- > fs/f2fs/checkpoint.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > index 99c8061da55b9..ff807e14c8911 100644 > --- a/fs/f2fs/checkpoint.c > +++ b/fs/f2fs/checkpoint.c > @@ -1258,8 +1258,6 @@ void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) > DEFINE_WAIT(wait); > > for (;;) { > - prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE); > - > if (!get_pages(sbi, type)) > break; > > @@ -1271,6 +1269,8 @@ void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int type) > FS_CP_META_IO); > else if (type == F2FS_WB_CP_DATA) > f2fs_submit_merged_write(sbi, DATA); > + > + prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE); > io_schedule_timeout(DEFAULT_IO_TIMEOUT); > } > finish_wait(&sbi->cp_wait, &wait); > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-04 3:00 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-08-03 17:28 [PATCH] f2fs: remove a waiter for checkpoint completion Jaegeuk Kim 2020-08-04 0:55 ` [f2fs-dev] " Chao Yu 2020-08-04 1:04 ` Jaegeuk Kim 2020-08-04 1:17 ` Chao Yu 2020-08-04 2:43 ` Jaegeuk Kim 2020-08-04 3:00 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox