Re: [PATCH] f2fs: enable atgc if atgc_age_threshold from user is less than elapsed_time
From: Zhiguo Niu
Date: Wed Jun 05 2024 - 02:17:01 EST
On Wed, Jun 5, 2024 at 1:59 PM Zhiguo Niu <niuzhiguo84@xxxxxxxxx> wrote:
>
> On Wed, Jun 5, 2024 at 11:48 AM Chao Yu <chao@xxxxxxxxxx> wrote:
> >
> > On 2024/6/3 17:05, Zhiguo Niu wrote:
> > > On Mon, Jun 3, 2024 at 3:41 PM Chao Yu <chao@xxxxxxxxxx> wrote:
> > >>
> > >> On 2024/5/20 19:36, Zhiguo Niu wrote:
> > >>> Now atgc can be enabled based on the following conditions:
> > >>> -ATGC mount option is set
> > >>> -elapsed_time is more than atgc_age_threshold already
> > >>> but these conditions are check when umounted->mounted device again.
> > >>> If the device has not be umounted->mounted for a long time, atgc can
> > >>> not work even the above conditions met.
> > >>>
> > >>> It is better to enable atgc dynamiclly when user change atgc_age_threshold
> > >>> meanwhile this vale is less than elapsed_time and ATGC mount option is on.
> > >>>
> > >>> Signed-off-by: Zhiguo Niu <zhiguo.niu@xxxxxxxxxx>
> > >>> ---
> > >>> fs/f2fs/f2fs.h | 1 +
> > >>> fs/f2fs/segment.c | 9 ++++-----
> > >>> fs/f2fs/sysfs.c | 16 ++++++++++++++++
> > >>> 3 files changed, 21 insertions(+), 5 deletions(-)
> > >>>
> > >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > >>> index 1974b6a..e441d2d 100644
> > >>> --- a/fs/f2fs/f2fs.h
> > >>> +++ b/fs/f2fs/f2fs.h
> > >>> @@ -3694,6 +3694,7 @@ void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
> > >>> int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi);
> > >>> void f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi);
> > >>> void f2fs_restore_inmem_curseg(struct f2fs_sb_info *sbi);
> > >>> +int f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi);
> > >>> int f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> > >>> unsigned int start, unsigned int end);
> > >>> int f2fs_allocate_new_section(struct f2fs_sb_info *sbi, int type, bool force);
> > >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > >>> index 71dc8042..44923d4 100644
> > >>> --- a/fs/f2fs/segment.c
> > >>> +++ b/fs/f2fs/segment.c
> > >>> @@ -2931,14 +2931,11 @@ static int get_atssr_segment(struct f2fs_sb_info *sbi, int type,
> > >>> return ret;
> > >>> }
> > >>>
> > >>> -static int __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
> > >>> +int f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
> > >>> {
> > >>> struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_ALL_DATA_ATGC);
> > >>> int ret = 0;
> > >>>
> > >>> - if (!sbi->am.atgc_enabled)
> > >>> - return 0;
> > >>> -
> > >>> f2fs_down_read(&SM_I(sbi)->curseg_lock);
> > >>>
> > >>> mutex_lock(&curseg->curseg_mutex);
> > >>> @@ -2955,7 +2952,9 @@ static int __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
> > >>> }
> > >>> int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi)
> > >>> {
> > >>> - return __f2fs_init_atgc_curseg(sbi);
> > >>> + if (!sbi->am.atgc_enabled)
> > >>> + return 0;
> > >>> + return f2fs_init_atgc_curseg(sbi);
> > >>> }
> > >>>
> > >>> static void __f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi, int type)
> > >>> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> > >>> index 09d3ecf..72676c5 100644
> > >>> --- a/fs/f2fs/sysfs.c
> > >>> +++ b/fs/f2fs/sysfs.c
> > >>> @@ -673,6 +673,22 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
> > >>> return count;
> > >>> }
> > >>>
> > >>> + if (!strcmp(a->attr.name, "atgc_age_threshold")) {
> > >>> + if (t < 0)
> > >>> + return -EINVAL;
> > >>> + sbi->am.age_threshold = t;
> > >>> + if (sbi->am.atgc_enabled)
> > >>> + return count;
> > >>> +
> > >>> + if (test_opt(sbi, ATGC) &&
> > >>> + le64_to_cpu(sbi->ckpt->elapsed_time) >= t) {
> > >>
> > > Hi Chao,
> > >> Oh, I guess you want to fix this:
> > > Yes, Sorry for not making it clear before.
> > >>
> > >> static void init_atgc_management(struct f2fs_sb_info *sbi)
> > >> {
> > >> ...
> > >> if (test_opt(sbi, ATGC) &&
> > >> SIT_I(sbi)->elapsed_time >= DEF_GC_THREAD_AGE_THRESHOLD)
> > >> am->atgc_enabled = true;
> > >>
> > >> What about enabling atgc_enabled during checkpoint once elapsed time is
> > >> satisfed w/ the condition? I guess this can give another chance whenever
> > >> CP is been triggered, and it can avoid the racing condition as well.
> > > 1. I'm not sure if this will increase checkpoint time consumption?
> >
> > Since it won't increase any IO time, I guess it's fine tolerate time consumed
> > by initialization of atgc curseg.
> >
> > > 2. dynamically enabling atgc may have other problems. Is this confirmed?
> >
> > I think so, checkpoint has avoided most race cases.
> >
> > So, how do you think of below diff:
> Hi Chao,
> I think flow is ok, some details need to be discussed.
> >
> > ---
> > fs/f2fs/checkpoint.c | 2 ++
> > fs/f2fs/f2fs.h | 1 +
> > fs/f2fs/segment.c | 26 +++++++++++++++++++++++---
> > 3 files changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 55d444bec5c0..4a73bd481a25 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -1718,6 +1718,8 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> > }
> >
> > f2fs_restore_inmem_curseg(sbi);
> > + f2fs_reinit_atgc_curseg(sbi);
> > +
> > stat_inc_cp_count(sbi);
> > stop:
> > unblock_operations(sbi);
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 9688df332147..8d385a1c75ad 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -3693,6 +3693,7 @@ void f2fs_release_discard_addrs(struct f2fs_sb_info *sbi);
> > int f2fs_npages_for_summary_flush(struct f2fs_sb_info *sbi, bool for_ra);
> > bool f2fs_segment_has_free_slot(struct f2fs_sb_info *sbi, int segno);
> > int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi);
> > +int f2fs_reinit_atgc_curseg(struct f2fs_sb_info *sbi);
> > void f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi);
> > void f2fs_restore_inmem_curseg(struct f2fs_sb_info *sbi);
> > int f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index bdc2247387e8..6d4273faf061 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -2949,12 +2949,12 @@ static int get_atssr_segment(struct f2fs_sb_info *sbi, int type,
> > return ret;
> > }
> >
> > -static int __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
> > +static int __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi, bool force)
> > {
> > struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_ALL_DATA_ATGC);
> > int ret = 0;
> >
> > - if (!sbi->am.atgc_enabled)
> > + if (!sbi->am.atgc_enabled && !force)
> Is this parameter "force" unnecessary?
> in mount flow, even atgc enable conditions are all met, it is not
> enabled because of force=false.
> Or am I missing something?
hi Chao,
sorry, I read && as || , Please ignore the above comment,
but Is it still OK without the "force" parameter?
thanks！
> > return 0;
> >
> > f2fs_down_read(&SM_I(sbi)->curseg_lock);
> > @@ -2971,9 +2971,29 @@ static int __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
> > f2fs_up_read(&SM_I(sbi)->curseg_lock);
> > return ret;
> > }
> > +
> > int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi)
> > {
> > - return __f2fs_init_atgc_curseg(sbi);
> > + return __f2fs_init_atgc_curseg(sbi, false);
> > +}
> > +
> > +int f2fs_reinit_atgc_curseg(struct f2fs_sb_info *sbi)
> > +{
> > + int ret;
> > +
> > + if (!test_opt(sbi, ATGC))
> > + return 0;
> > + if (sbi->am.atgc_enabled)
> > + return 0;
> > + if (SIT_I(sbi)->elapsed_time < sbi->am.age_threshold)
> SIT(sbi)->elapsed_time is just updated in mount flow, so
> ckpt->elapsed_time is more suitable here?
> thanks!
> > + return 0;
> > +
> > + ret = __f2fs_init_atgc_curseg(sbi, true);
> > + if (!ret) {
> > + sbi->am.atgc_enabled = true;
> > + f2fs_info(sbi, "reenabled age threshold GC");
> > + }
> > + return ret;
> > }
> >
> > static void __f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi, int type)
> > --
> > 2.40.1
> >
> > > thanks！
> > >>
> > >> Thanks,
> > >>
> > >>> + if (f2fs_init_atgc_curseg(sbi))
> > >>> + return -EINVAL;
> > >>> + sbi->am.atgc_enabled = true;
> > >>> + }
> > >>> + return count;
> > >>> + }
> > >>> +
> > >>> if (!strcmp(a->attr.name, "gc_segment_mode")) {
> > >>> if (t < MAX_GC_MODE)
> > >>> sbi->gc_segment_mode = t;