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 <[email protected] 
> <mailto:[email protected]>> 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 <[email protected] 
> <mailto:[email protected]>
>     > <mailto:[email protected] <mailto:[email protected]>>> 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 <[email protected] 
> <mailto:[email protected]>
>     >     <mailto:[email protected] <mailto:[email protected]>>>
>     >     > ---
>     >     >  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
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Reply via email to