[PATCH] f2fs: set ioprio of GC kthread to idle

[PATCH] f2fs: set ioprio of GC kthread to idle

[Park Ju Hyung]

GC should run conservatively as possible to reduce latency spikes to the user.

Setting ioprio to idle class will allow the kernel to schedule GC thread's I/O
to not affect any other processes' I/O requests.

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

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index bfe6a8ccc3a0..54d2d74e9afe 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -143,6 +143,8 @@ int start_gc_thread(struct f2fs_sb_info *sbi)
 		kfree(gc_th);
 		sbi->gc_thread = NULL;
 	}
+	set_task_ioprio(sbi->gc_thread->f2fs_gc_task,
+			IOPRIO_PRIO_VALUE(IOPRIO_CLASS_IDLE, 0));
 out:
 	return err;
 }
-- 
2.14.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs: set ioprio of GC kthread to idle

[Chao Yu]

Hi Park,

On 2017/9/26 10:36, Park Ju Hyung wrote:
> GC should run conservatively as possible to reduce latency spikes to the user.
> 
> Setting ioprio to idle class will allow the kernel to schedule GC thread's I/O
> to not affect any other processes' I/O requests.

IMO, we'd better not simply changing our IO priority of background GC, because
before doing real garbage collection we will try to grab gc_mutex lock, which is
shared with checkpoint flow. So while background GC is running, if there it
comes higher priority IOs and a sudden checkpoint, the checkpoint flow will be
block for longer time when it try to grab gc_mutex, since most time checkpoint
comes from foreground application, so it may effects user experience.

What about adding IO aware point before triggering each IO inside GC flow, and
when it hits other IO, let's give up background GC?

Thanks,

> 
> Signed-off-by: Park Ju Hyung <qkrwngud825@gmail.com>
> ---
>  fs/f2fs/gc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index bfe6a8ccc3a0..54d2d74e9afe 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -143,6 +143,8 @@ int start_gc_thread(struct f2fs_sb_info *sbi)
>  		kfree(gc_th);
>  		sbi->gc_thread = NULL;
>  	}
> +	set_task_ioprio(sbi->gc_thread->f2fs_gc_task,
> +			IOPRIO_PRIO_VALUE(IOPRIO_CLASS_IDLE, 0));
>  out:
>  	return err;
>  }
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs: set ioprio of GC kthread to idle

[Chao Yu]

Hi Park,

On 2017/9/27 17:57, Ju Hyung Park wrote:
> Hi Chao,
> 
>> What about detecting pending IOs in block request queue with inner is_idle()?
> 
> This is about reducing GC's I/O priority when GC has already kicked in.

As I said, reducing background GC's I/O priority will slow down its speed, result
in delaying later checkpoint, so that would not be appropriate.

> 
> If I understood f2fs code properly, changing is_idle() would only change how and when GC would kick in.
> And I believe current is_idle() is good enough for that purpose.

Agreed.

> 
>> Hmmm.. the change will make gc_mutex held with foreground GC can be preempted by
>> checkpoint. But for background GC, normally, we just pickup one victim and do
>> garbage collection with it, so the method will not work if checkpoint comes in
>> the middle of that GC cycle.
> 
> If foreground GC takes precedence over checkpoint, yes, my suggestion would not be good.
> Would it be possible to change the background GC to be interruptible(stopped) by checkpoint?

I think it is possible. ;)

Thanks,

> 
> Thanks.
> 
> On Wed, Sep 27, 2017 at 10:36 AM, Chao Yu <yuchao0@huawei.com <mailto:yuchao0@huawei.com>> wrote:
> 
>     Hi Park,
> 
>     On 2017/9/26 22:11, Ju Hyung Park wrote:
>     > Hi Chao,
>     >
>     > I did not know that checkpoint was sharing gc_mutex.
>     >
>     > If you mean giving up background GC when the kernel detects other I/O activity,
>     > it would be hard as we need to come up with a new, general VFS API along with
>     > the existing ioprio API.
> 
>     What about detecting pending IOs in block request queue with inner is_idle()?
> 
>     >
>     > What about locking/unlocking gc_mutex more frequently by moving the mutex_lock()
>     > to f2fs_gc() and unlock and relock when 'goto gc_more' is called?
> 
>     Hmmm.. the change will make gc_mutex held with foreground GC can be preempted by
>     checkpoint. But for background GC, normally, we just pickup one victim and do
>     garbage collection with it, so the method will not work if checkpoint comes in
>     the middle of that GC cycle.
> 
>     Thanks,
> 
>     >
>     > If checkpoint is already waiting for a gc_mutex lock, it would be handled first
>     > before the next GC is handled.
>     >
>     > Thanks.
>     >
>     > On Tue, Sep 26, 2017 at 8:44 PM, Chao Yu <yuchao0@huawei.com <mailto:yuchao0@huawei.com>
>     > <mailto:yuchao0@huawei.com <mailto:yuchao0@huawei.com>>> wrote:
>     >
>     >     Hi Park,
>     >
>     >     On 2017/9/26 10:36, Park Ju Hyung wrote:
>     >     > GC should run conservatively as possible to reduce latency spikes to the user.
>     >     >
>     >     > Setting ioprio to idle class will allow the kernel to schedule GC thread's I/O
>     >     > to not affect any other processes' I/O requests.
>     >
>     >     IMO, we'd better not simply changing our IO priority of background GC, because
>     >     before doing real garbage collection we will try to grab gc_mutex lock, which is
>     >     shared with checkpoint flow. So while background GC is running, if there it
>     >     comes higher priority IOs and a sudden checkpoint, the checkpoint flow will be
>     >     block for longer time when it try to grab gc_mutex, since most time checkpoint
>     >     comes from foreground application, so it may effects user experience.
>     >
>     >     What about adding IO aware point before triggering each IO inside GC flow, and
>     >     when it hits other IO, let's give up background GC?
>     >
>     >     Thanks,
>     >
>     >     >
>     >     > Signed-off-by: Park Ju Hyung <qkrwngud825@gmail.com <mailto:qkrwngud825@gmail.com>
>     >     <mailto:qkrwngud825@gmail.com <mailto:qkrwngud825@gmail.com>>>
>     >     > ---
>     >     >  fs/f2fs/gc.c | 2 ++
>     >     >  1 file changed, 2 insertions(+)
>     >     >
>     >     > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>     >     > index bfe6a8ccc3a0..54d2d74e9afe 100644
>     >     > --- a/fs/f2fs/gc.c
>     >     > +++ b/fs/f2fs/gc.c
>     >     > @@ -143,6 +143,8 @@ int start_gc_thread(struct f2fs_sb_info *sbi)
>     >     >               kfree(gc_th);
>     >     >               sbi->gc_thread = NULL;
>     >     >       }
>     >     > +     set_task_ioprio(sbi->gc_thread->f2fs_gc_task,
>     >     > +                     IOPRIO_PRIO_VALUE(IOPRIO_CLASS_IDLE, 0));
>     >     >  out:
>     >     >       return err;
>     >     >  }
>     >     >
>     >
>     >
> 
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel