All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] f2fs: remove sleep_time under gc_urgent
@ 2019-05-14  6:36 Park Ju Hyung
  2019-05-14  6:36 ` [PATCH 2/3] f2fs: don't wait with each discards " Park Ju Hyung
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Park Ju Hyung @ 2019-05-14  6:36 UTC (permalink / raw)
  To: linux-f2fs-devel

gc_urgent is meant to be a hint from the user to force f2fs to run GC
aggressively, which means they are willing to take the hit on increased
latency during gc_urgent. It's meaningless to sleep between each GC under
gc_urgent, Not to mention that the default value of 500 ms makes gc_urgent
super ineffective.

Remove urgent_sleep_time entirely and allow GC to be finished much faster.

Use 1 for wait_ms instead of 0 to prevent possible CPU hoggings.

Signed-off-by: Park Ju Hyung <qkrwngud825@gmail.com>
---
 fs/f2fs/gc.c    | 3 +--
 fs/f2fs/gc.h    | 2 --
 fs/f2fs/sysfs.c | 3 ---
 3 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 963fb4571fd9..9c3ed89c8c5b 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -77,7 +77,7 @@ static int gc_thread_func(void *data)
 		 * So, I'd like to wait some time to collect dirty segments.
 		 */
 		if (sbi->gc_mode == GC_URGENT) {
-			wait_ms = gc_th->urgent_sleep_time;
+			wait_ms = 1;
 			mutex_lock(&sbi->gc_mutex);
 			goto do_gc;
 		}
@@ -129,7 +129,6 @@ int f2fs_start_gc_thread(struct f2fs_sb_info *sbi)
 		goto out;
 	}
 
-	gc_th->urgent_sleep_time = DEF_GC_THREAD_URGENT_SLEEP_TIME;
 	gc_th->min_sleep_time = DEF_GC_THREAD_MIN_SLEEP_TIME;
 	gc_th->max_sleep_time = DEF_GC_THREAD_MAX_SLEEP_TIME;
 	gc_th->no_gc_sleep_time = DEF_GC_THREAD_NOGC_SLEEP_TIME;
diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h
index bbac9d3787bd..de79a867837e 100644
--- a/fs/f2fs/gc.h
+++ b/fs/f2fs/gc.h
@@ -10,7 +10,6 @@
 						 * whether IO subsystem is idle
 						 * or not
 						 */
-#define DEF_GC_THREAD_URGENT_SLEEP_TIME	500	/* 500 ms */
 #define DEF_GC_THREAD_MIN_SLEEP_TIME	30000	/* milliseconds */
 #define DEF_GC_THREAD_MAX_SLEEP_TIME	60000
 #define DEF_GC_THREAD_NOGC_SLEEP_TIME	300000	/* wait 5 min */
@@ -27,7 +26,6 @@ struct f2fs_gc_kthread {
 	wait_queue_head_t gc_wait_queue_head;
 
 	/* for gc sleep time */
-	unsigned int urgent_sleep_time;
 	unsigned int min_sleep_time;
 	unsigned int max_sleep_time;
 	unsigned int no_gc_sleep_time;
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index 729f46a3c9ee..0165431e83e5 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -397,8 +397,6 @@ static struct f2fs_attr f2fs_attr_##_name = {			\
 	.id	= _id,						\
 }
 
-F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent_sleep_time,
-							urgent_sleep_time);
 F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_min_sleep_time, min_sleep_time);
 F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_max_sleep_time, max_sleep_time);
 F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_no_gc_sleep_time, no_gc_sleep_time);
@@ -459,7 +457,6 @@ F2FS_FEATURE_RO_ATTR(sb_checksum, FEAT_SB_CHECKSUM);
 
 #define ATTR_LIST(name) (&f2fs_attr_##name.attr)
 static struct attribute *f2fs_attrs[] = {
-	ATTR_LIST(gc_urgent_sleep_time),
 	ATTR_LIST(gc_min_sleep_time),
 	ATTR_LIST(gc_max_sleep_time),
 	ATTR_LIST(gc_no_gc_sleep_time),
-- 
2.21.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] f2fs: don't wait with each discards under gc_urgent
  2019-05-14  6:36 [PATCH 1/3] f2fs: remove sleep_time under gc_urgent Park Ju Hyung
@ 2019-05-14  6:36 ` Park Ju Hyung
  2019-05-14  6:36 ` [PATCH 3/3] f2fs: always assume that the device is idle " Park Ju Hyung
  2019-05-14  9:20 ` [PATCH 1/3] f2fs: remove sleep_time " Chao Yu
  2 siblings, 0 replies; 11+ messages in thread
From: Park Ju Hyung @ 2019-05-14  6:36 UTC (permalink / raw)
  To: linux-f2fs-devel

f2fs already addresses gc_urgent under discard, but still uses the
default wait_ms.

To gain as many free segments as fast as possible,
use 1 for wait_ms under gc_urgent for discards.

Signed-off-by: Park Ju Hyung <qkrwngud825@gmail.com>
---
 fs/f2fs/segment.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 8dee063c833f..dece5ebfb8f0 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1685,7 +1685,8 @@ static int issue_discard_thread(void *data)
 		wait_event_interruptible_timeout(*q,
 				kthread_should_stop() || freezing(current) ||
 				dcc->discard_wake,
-				msecs_to_jiffies(wait_ms));
+				msecs_to_jiffies((sbi->gc_mode == GC_URGENT) ?
+						 1 : wait_ms));
 
 		if (dcc->discard_wake)
 			dcc->discard_wake = 0;
-- 
2.21.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] f2fs: always assume that the device is idle under gc_urgent
  2019-05-14  6:36 [PATCH 1/3] f2fs: remove sleep_time under gc_urgent Park Ju Hyung
  2019-05-14  6:36 ` [PATCH 2/3] f2fs: don't wait with each discards " Park Ju Hyung
@ 2019-05-14  6:36 ` Park Ju Hyung
  2019-05-21  0:11   ` Jaegeuk Kim
  2019-05-14  9:20 ` [PATCH 1/3] f2fs: remove sleep_time " Chao Yu
  2 siblings, 1 reply; 11+ messages in thread
From: Park Ju Hyung @ 2019-05-14  6:36 UTC (permalink / raw)
  To: linux-f2fs-devel

This allows more aggressive discards and balancing job to be done
under gc_urgent.

Signed-off-by: Park Ju Hyung <qkrwngud825@gmail.com>
---
 fs/f2fs/f2fs.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 533fafca68f4..14c95116cd3d 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2192,6 +2192,9 @@ static inline struct bio *f2fs_bio_alloc(struct f2fs_sb_info *sbi,
 
 static inline bool is_idle(struct f2fs_sb_info *sbi, int type)
 {
+	if (sbi->gc_mode == GC_URGENT)
+		return true;
+
 	if (get_pages(sbi, F2FS_RD_DATA) || get_pages(sbi, F2FS_RD_NODE) ||
 		get_pages(sbi, F2FS_RD_META) || get_pages(sbi, F2FS_WB_DATA) ||
 		get_pages(sbi, F2FS_WB_CP_DATA) ||
-- 
2.21.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] f2fs: remove sleep_time under gc_urgent
  2019-05-14  6:36 [PATCH 1/3] f2fs: remove sleep_time under gc_urgent Park Ju Hyung
  2019-05-14  6:36 ` [PATCH 2/3] f2fs: don't wait with each discards " Park Ju Hyung
  2019-05-14  6:36 ` [PATCH 3/3] f2fs: always assume that the device is idle " Park Ju Hyung
@ 2019-05-14  9:20 ` Chao Yu
       [not found]   ` <CAD14+f0Zia3oAi+QO+wCBrbV_=csp1SWB4BE7yN0h+=paZpg=w@mail.gmail.com>
  2 siblings, 1 reply; 11+ messages in thread
From: Chao Yu @ 2019-05-14  9:20 UTC (permalink / raw)
  To: Park Ju Hyung, linux-f2fs-devel

Hi Ju Hyung,

On 2019/5/14 14:36, Park Ju Hyung wrote:
> gc_urgent is meant to be a hint from the user to force f2fs to run GC
> aggressively, which means they are willing to take the hit on increased
> latency during gc_urgent. It's meaningless to sleep between each GC under
> gc_urgent, Not to mention that the default value of 500 ms makes gc_urgent
> super ineffective.
> 
> Remove urgent_sleep_time entirely and allow GC to be finished much faster.
> 
> Use 1 for wait_ms instead of 0 to prevent possible CPU hoggings.

IIRC, related interfaces (gc_urgent/urgent_sleep_time) were introduced for
Android Go, if some conditions (in userspace) are satisfied, GC/discard threads
are waked up via sysfs to clean filesystem/device space. Considering the system
runs in low-performance device, we can't expect that device can handle IOs very
quickly, so that once one of condition (e.g. screen is lighted up) breaks, apps
may be stuck due to racing with IO from GC. Anyway I think we need to set proper
interval for background GC, IMO, 1 ms is too short...

And it needs to wait for Jaegeuk's opinion.

Thanks,

> 
> Signed-off-by: Park Ju Hyung <qkrwngud825@gmail.com>
> ---
>  fs/f2fs/gc.c    | 3 +--
>  fs/f2fs/gc.h    | 2 --
>  fs/f2fs/sysfs.c | 3 ---
>  3 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 963fb4571fd9..9c3ed89c8c5b 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -77,7 +77,7 @@ static int gc_thread_func(void *data)
>  		 * So, I'd like to wait some time to collect dirty segments.
>  		 */
>  		if (sbi->gc_mode == GC_URGENT) {
> -			wait_ms = gc_th->urgent_sleep_time;
> +			wait_ms = 1;
>  			mutex_lock(&sbi->gc_mutex);
>  			goto do_gc;
>  		}
> @@ -129,7 +129,6 @@ int f2fs_start_gc_thread(struct f2fs_sb_info *sbi)
>  		goto out;
>  	}
>  
> -	gc_th->urgent_sleep_time = DEF_GC_THREAD_URGENT_SLEEP_TIME;
>  	gc_th->min_sleep_time = DEF_GC_THREAD_MIN_SLEEP_TIME;
>  	gc_th->max_sleep_time = DEF_GC_THREAD_MAX_SLEEP_TIME;
>  	gc_th->no_gc_sleep_time = DEF_GC_THREAD_NOGC_SLEEP_TIME;
> diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h
> index bbac9d3787bd..de79a867837e 100644
> --- a/fs/f2fs/gc.h
> +++ b/fs/f2fs/gc.h
> @@ -10,7 +10,6 @@
>  						 * whether IO subsystem is idle
>  						 * or not
>  						 */
> -#define DEF_GC_THREAD_URGENT_SLEEP_TIME	500	/* 500 ms */
>  #define DEF_GC_THREAD_MIN_SLEEP_TIME	30000	/* milliseconds */
>  #define DEF_GC_THREAD_MAX_SLEEP_TIME	60000
>  #define DEF_GC_THREAD_NOGC_SLEEP_TIME	300000	/* wait 5 min */
> @@ -27,7 +26,6 @@ struct f2fs_gc_kthread {
>  	wait_queue_head_t gc_wait_queue_head;
>  
>  	/* for gc sleep time */
> -	unsigned int urgent_sleep_time;
>  	unsigned int min_sleep_time;
>  	unsigned int max_sleep_time;
>  	unsigned int no_gc_sleep_time;
> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> index 729f46a3c9ee..0165431e83e5 100644
> --- a/fs/f2fs/sysfs.c
> +++ b/fs/f2fs/sysfs.c
> @@ -397,8 +397,6 @@ static struct f2fs_attr f2fs_attr_##_name = {			\
>  	.id	= _id,						\
>  }
>  
> -F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent_sleep_time,
> -							urgent_sleep_time);
>  F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_min_sleep_time, min_sleep_time);
>  F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_max_sleep_time, max_sleep_time);
>  F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_no_gc_sleep_time, no_gc_sleep_time);
> @@ -459,7 +457,6 @@ F2FS_FEATURE_RO_ATTR(sb_checksum, FEAT_SB_CHECKSUM);
>  
>  #define ATTR_LIST(name) (&f2fs_attr_##name.attr)
>  static struct attribute *f2fs_attrs[] = {
> -	ATTR_LIST(gc_urgent_sleep_time),
>  	ATTR_LIST(gc_min_sleep_time),
>  	ATTR_LIST(gc_max_sleep_time),
>  	ATTR_LIST(gc_no_gc_sleep_time),
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] f2fs: remove sleep_time under gc_urgent
       [not found]   ` <CAD14+f0Zia3oAi+QO+wCBrbV_=csp1SWB4BE7yN0h+=paZpg=w@mail.gmail.com>
@ 2019-05-14 11:19     ` Chao Yu
  2019-05-14 11:30       ` Ju Hyung Park
  0 siblings, 1 reply; 11+ messages in thread
From: Chao Yu @ 2019-05-14 11:19 UTC (permalink / raw)
  To: Ju Hyung Park, linux-f2fs-devel

Hi Ju Hyung,

On 2019/5/14 17:51, Ju Hyung Park wrote:
> Hi Chao,
> 
> On Tue, May 14, 2019 at 6:20 PM Chao Yu <yuchao0@huawei.com
> <mailto:yuchao0@huawei.com>> wrote:
>> IIRC, related interfaces (gc_urgent/urgent_sleep_time) were introduced for
>> Android Go
> 
> These are used for all Android devices since Pie, and is triggered when Android
> thinks the device is idle. iirc, screen state is one of them to ensure that the
> user isn't using any apps.
> 
>> may be stuck due to racing with IO from GC. Anyway I think we need to set proper
>> interval for background GC, IMO, 1 ms is too short...
> 
> I've been using this(with a slightly different code) for years and yet to notice
> any spikes in lags/slowdowns. Worst scenario, I'd just have to deal with an
> added split second(100ms max?) delay in screen wake-up.

I'm not sure about why this happened... maybe you need to do some test to
analyse the root cause of it, filesystem/device fragment? too many undiscard
space? or non-storage issue?

> 
> I think we're thinking too theoretical and failing to address real practical issues.
> 
> Also, if that's a concern, the userspace shouldn't use gc_urgent at all. We have
> other tunables for tuning regular GC.

Yes, we can, but there is no sysfs entry can make GC running as IO
non-awareness, I guess that's why Jaegeuk expose independent entry.

> 
> Similar to fstrim on traditional filesystems(blocking everything until all trim
> is done), this is supposed to be a userspace hint to run GC aggressively in
> system where the in-kernel is_idle() isn't enough, and as it's an interface for
> "aggressive" GC, it should finish as soon as possible imo.

I agreed that it should done as soon as possible, but it needs to consider IO
race in between Apps after screen wake-up and BGGC to avoid potential ANR.

> Userspace should be calling this when it knows that the device is idle, or when
> it's willing to sacrifice some latency.
> 
> The default 500ms didn't make much sense and causes real-world problem as vold
> only waits 480 seconds.

I don't remember the threshold in Android Go (Oreo)...

It's userspace strategy, we can change both of them
(vold_wait_time/gc_urgent_sleep_time) in userspace if current value doesn't make
any sense.

Thanks,

> 
> Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] f2fs: remove sleep_time under gc_urgent
  2019-05-14 11:19     ` Chao Yu
@ 2019-05-14 11:30       ` Ju Hyung Park
  2019-05-15  2:08         ` Chao Yu
  0 siblings, 1 reply; 11+ messages in thread
From: Ju Hyung Park @ 2019-05-14 11:30 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-f2fs-devel

Hi Chao,

On Tue, May 14, 2019 at 8:19 PM Chao Yu <yuchao0@huawei.com> wrote:
> > I've been using this(with a slightly different code) for years and yet to notice
> > any spikes in lags/slowdowns. Worst scenario, I'd just have to deal with an
> > added split second(100ms max?) delay in screen wake-up.
>
> I'm not sure about why this happened... maybe you need to do some test to
> analyse the root cause of it, filesystem/device fragment? too many undiscard
> space? or non-storage issue?

Um, I'm not sure you understood what I said.
What I meant is that I haven't found any issues with using an approach
like this(gc_urgent with 1ms sleep intervals) for years on various
Android devices.

> I agreed that it should done as soon as possible, but it needs to consider IO
> race in between Apps after screen wake-up and BGGC to avoid potential ANR.

I actually need to check whether vold turns off gc_urgent immediately
after screen turns itself back on.
I don't think we need to take potential ANR in to account *if* vold
stops gc_urgent right after screen-on. What do you think?

> It's userspace strategy, we can change both of them
> (vold_wait_time/gc_urgent_sleep_time) in userspace if current value doesn't make
> any sense.

Even the user can set the tunables themselves, the default should be
sensical imo.
An "urgent" GC that only GCs up-to 2 segments per second doesn't sound
that "urgent" :p

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] f2fs: remove sleep_time under gc_urgent
  2019-05-14 11:30       ` Ju Hyung Park
@ 2019-05-15  2:08         ` Chao Yu
  2019-05-21  0:10           ` Jaegeuk Kim
  0 siblings, 1 reply; 11+ messages in thread
From: Chao Yu @ 2019-05-15  2:08 UTC (permalink / raw)
  To: Ju Hyung Park; +Cc: linux-f2fs-devel

Hi Ju Hyung,

On 2019/5/14 19:30, Ju Hyung Park wrote:
> Hi Chao,
> 
> On Tue, May 14, 2019 at 8:19 PM Chao Yu <yuchao0@huawei.com> wrote:
>>> I've been using this(with a slightly different code) for years and yet to notice
>>> any spikes in lags/slowdowns. Worst scenario, I'd just have to deal with an
>>> added split second(100ms max?) delay in screen wake-up.
>>
>> I'm not sure about why this happened... maybe you need to do some test to
>> analyse the root cause of it, filesystem/device fragment? too many undiscard
>> space? or non-storage issue?
> 
> Um, I'm not sure you understood what I said.
> What I meant is that I haven't found any issues with using an approach
> like this(gc_urgent with 1ms sleep intervals) for years on various
> Android devices.

Ah, sorry, I misread what you said.

> 
>> I agreed that it should done as soon as possible, but it needs to consider IO
>> race in between Apps after screen wake-up and BGGC to avoid potential ANR.
> 
> I actually need to check whether vold turns off gc_urgent immediately
> after screen turns itself back on.
> I don't think we need to take potential ANR in to account *if* vold
> stops gc_urgent right after screen-on. What do you think?

What do you mean, I didn't catch it...

> 
>> It's userspace strategy, we can change both of them
>> (vold_wait_time/gc_urgent_sleep_time) in userspace if current value doesn't make
>> any sense.
> 
> Even the user can set the tunables themselves, the default should be
> sensical imo.

Agreed, how about adjusting this default value according device type, for fast
device, like SSD, we can set default interval to very small value.

Maybe we can implement this base on below commit if you agreed.

f2fs: support tunning for multiple kind of storage device

Thanks,

> An "urgent" GC that only GCs up-to 2 segments per second doesn't sound
> that "urgent" :p
> 
> Thanks.
> .
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] f2fs: remove sleep_time under gc_urgent
  2019-05-15  2:08         ` Chao Yu
@ 2019-05-21  0:10           ` Jaegeuk Kim
  2019-05-22 12:10             ` Chao Yu
  0 siblings, 1 reply; 11+ messages in thread
From: Jaegeuk Kim @ 2019-05-21  0:10 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-f2fs-devel

On 05/15, Chao Yu wrote:
> Hi Ju Hyung,
> 
> On 2019/5/14 19:30, Ju Hyung Park wrote:
> > Hi Chao,
> > 
> > On Tue, May 14, 2019 at 8:19 PM Chao Yu <yuchao0@huawei.com> wrote:
> >>> I've been using this(with a slightly different code) for years and yet to notice
> >>> any spikes in lags/slowdowns. Worst scenario, I'd just have to deal with an
> >>> added split second(100ms max?) delay in screen wake-up.
> >>
> >> I'm not sure about why this happened... maybe you need to do some test to
> >> analyse the root cause of it, filesystem/device fragment? too many undiscard
> >> space? or non-storage issue?
> > 
> > Um, I'm not sure you understood what I said.
> > What I meant is that I haven't found any issues with using an approach
> > like this(gc_urgent with 1ms sleep intervals) for years on various
> > Android devices.
> 
> Ah, sorry, I misread what you said.
> 
> > 
> >> I agreed that it should done as soon as possible, but it needs to consider IO
> >> race in between Apps after screen wake-up and BGGC to avoid potential ANR.
> > 
> > I actually need to check whether vold turns off gc_urgent immediately
> > after screen turns itself back on.
> > I don't think we need to take potential ANR in to account *if* vold
> > stops gc_urgent right after screen-on. What do you think?
> 
> What do you mean, I didn't catch it...
> 
> > 
> >> It's userspace strategy, we can change both of them
> >> (vold_wait_time/gc_urgent_sleep_time) in userspace if current value doesn't make
> >> any sense.
> > 
> > Even the user can set the tunables themselves, the default should be
> > sensical imo.
> 
> Agreed, how about adjusting this default value according device type, for fast
> device, like SSD, we can set default interval to very small value.
> 
> Maybe we can implement this base on below commit if you agreed.
> 
> f2fs: support tunning for multiple kind of storage device
> 
> Thanks,
> 
> > An "urgent" GC that only GCs up-to 2 segments per second doesn't sound
> > that "urgent" :p

Yes, it seems I set it too conservatively at that time. It'd be fine to decrease
the default time, but I'd prefer to remain the sysfs entry in order for user to
configure it just in case. It'd be also great to have the above Chao's patch to
have some default values regarding to discard/GC policies.

Thanks,

> > 
> > Thanks.
> > .
> > 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] f2fs: always assume that the device is idle under gc_urgent
  2019-05-14  6:36 ` [PATCH 3/3] f2fs: always assume that the device is idle " Park Ju Hyung
@ 2019-05-21  0:11   ` Jaegeuk Kim
  2019-05-22 12:10     ` Chao Yu
  0 siblings, 1 reply; 11+ messages in thread
From: Jaegeuk Kim @ 2019-05-21  0:11 UTC (permalink / raw)
  To: Park Ju Hyung; +Cc: linux-f2fs-devel

On 05/14, Park Ju Hyung wrote:
> This allows more aggressive discards and balancing job to be done
> under gc_urgent.

Let me merge this first. :)

> 
> Signed-off-by: Park Ju Hyung <qkrwngud825@gmail.com>
> ---
>  fs/f2fs/f2fs.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 533fafca68f4..14c95116cd3d 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2192,6 +2192,9 @@ static inline struct bio *f2fs_bio_alloc(struct f2fs_sb_info *sbi,
>  
>  static inline bool is_idle(struct f2fs_sb_info *sbi, int type)
>  {
> +	if (sbi->gc_mode == GC_URGENT)
> +		return true;
> +
>  	if (get_pages(sbi, F2FS_RD_DATA) || get_pages(sbi, F2FS_RD_NODE) ||
>  		get_pages(sbi, F2FS_RD_META) || get_pages(sbi, F2FS_WB_DATA) ||
>  		get_pages(sbi, F2FS_WB_CP_DATA) ||
> -- 
> 2.21.0
> 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] f2fs: remove sleep_time under gc_urgent
  2019-05-21  0:10           ` Jaegeuk Kim
@ 2019-05-22 12:10             ` Chao Yu
  0 siblings, 0 replies; 11+ messages in thread
From: Chao Yu @ 2019-05-22 12:10 UTC (permalink / raw)
  To: Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel

On 2019-5-21 8:10, Jaegeuk Kim wrote:
> On 05/15, Chao Yu wrote:
>> Hi Ju Hyung,
>>
>> On 2019/5/14 19:30, Ju Hyung Park wrote:
>>> Hi Chao,
>>>
>>> On Tue, May 14, 2019 at 8:19 PM Chao Yu <yuchao0@huawei.com> wrote:
>>>>> I've been using this(with a slightly different code) for years and yet to notice
>>>>> any spikes in lags/slowdowns. Worst scenario, I'd just have to deal with an
>>>>> added split second(100ms max?) delay in screen wake-up.
>>>>
>>>> I'm not sure about why this happened... maybe you need to do some test to
>>>> analyse the root cause of it, filesystem/device fragment? too many undiscard
>>>> space? or non-storage issue?
>>>
>>> Um, I'm not sure you understood what I said.
>>> What I meant is that I haven't found any issues with using an approach
>>> like this(gc_urgent with 1ms sleep intervals) for years on various
>>> Android devices.
>>
>> Ah, sorry, I misread what you said.
>>
>>>
>>>> I agreed that it should done as soon as possible, but it needs to consider IO
>>>> race in between Apps after screen wake-up and BGGC to avoid potential ANR.
>>>
>>> I actually need to check whether vold turns off gc_urgent immediately
>>> after screen turns itself back on.
>>> I don't think we need to take potential ANR in to account *if* vold
>>> stops gc_urgent right after screen-on. What do you think?
>>
>> What do you mean, I didn't catch it...
>>
>>>
>>>> It's userspace strategy, we can change both of them
>>>> (vold_wait_time/gc_urgent_sleep_time) in userspace if current value doesn't make
>>>> any sense.
>>>
>>> Even the user can set the tunables themselves, the default should be
>>> sensical imo.
>>
>> Agreed, how about adjusting this default value according device type, for fast
>> device, like SSD, we can set default interval to very small value.
>>
>> Maybe we can implement this base on below commit if you agreed.
>>
>> f2fs: support tunning for multiple kind of storage device
>>
>> Thanks,
>>
>>> An "urgent" GC that only GCs up-to 2 segments per second doesn't sound
>>> that "urgent" :p
> 
> Yes, it seems I set it too conservatively at that time. It'd be fine to decrease
> the default time, but I'd prefer to remain the sysfs entry in order for user to
> configure it just in case. It'd be also great to have the above Chao's patch to
> have some default values regarding to discard/GC policies.

Cool, let me rework on that patch. :)

Thanks,

> 
> Thanks,
> 
>>>
>>> Thanks.
>>> .
>>>
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] f2fs: always assume that the device is idle under gc_urgent
  2019-05-21  0:11   ` Jaegeuk Kim
@ 2019-05-22 12:10     ` Chao Yu
  0 siblings, 0 replies; 11+ messages in thread
From: Chao Yu @ 2019-05-22 12:10 UTC (permalink / raw)
  To: Jaegeuk Kim, Park Ju Hyung; +Cc: linux-f2fs-devel

On 2019-5-21 8:11, Jaegeuk Kim wrote:
> On 05/14, Park Ju Hyung wrote:
>> This allows more aggressive discards and balancing job to be done
>> under gc_urgent.
> 
> Let me merge this first. :)
> 
>>
>> Signed-off-by: Park Ju Hyung <qkrwngud825@gmail.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-05-22 12:11 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-14  6:36 [PATCH 1/3] f2fs: remove sleep_time under gc_urgent Park Ju Hyung
2019-05-14  6:36 ` [PATCH 2/3] f2fs: don't wait with each discards " Park Ju Hyung
2019-05-14  6:36 ` [PATCH 3/3] f2fs: always assume that the device is idle " Park Ju Hyung
2019-05-21  0:11   ` Jaegeuk Kim
2019-05-22 12:10     ` Chao Yu
2019-05-14  9:20 ` [PATCH 1/3] f2fs: remove sleep_time " Chao Yu
     [not found]   ` <CAD14+f0Zia3oAi+QO+wCBrbV_=csp1SWB4BE7yN0h+=paZpg=w@mail.gmail.com>
2019-05-14 11:19     ` Chao Yu
2019-05-14 11:30       ` Ju Hyung Park
2019-05-15  2:08         ` Chao Yu
2019-05-21  0:10           ` Jaegeuk Kim
2019-05-22 12:10             ` Chao Yu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.