Received-SPF: pass (google.com: domain of linux-kernel+bounces-198832-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <1716204978-29455-1-git-send-email-zhiguo.niu@unisoc.com> <446c7e4a-2a1e-402d-8238-2eee7cdfd5c2@kernel.org>
In-Reply-To: <446c7e4a-2a1e-402d-8238-2eee7cdfd5c2@kernel.org>
From: Zhiguo Niu <niuzhiguo84@gmail.com>
Date: Mon, 3 Jun 2024 17:05:36 +0800
Message-ID: <CAHJ8P3J2NkSfVTEcpxR+n3CjtywMxxEDMg4cLSL=8UOX8KcM+Q@mail.gmail.com>
Subject: Re: [PATCH] f2fs: enable atgc if atgc_age_threshold from user is less
 than elapsed_time
To: Chao Yu <chao@kernel.org>
Cc: Zhiguo Niu <zhiguo.niu@unisoc.com>, jaegeuk@kernel.org, 
	linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, 
	ke.wang@unisoc.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Jun 3, 2024 at 3:41=E2=80=AFPM Chao Yu <chao@kernel.org> 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_thresh=
old
> > meanwhile this vale is less than elapsed_time and ATGC mount option is =
on.
> >
> > Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> > ---
> >   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_i=
nfo *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 ty=
pe,
> >                                       unsigned int start, unsigned int =
end);
> >   int f2fs_allocate_new_section(struct f2fs_sb_info *sbi, int type, boo=
l 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_inf=
o *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 =3D CURSEG_I(sbi, CURSEG_ALL_DATA_ATGC=
);
> >       int ret =3D 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 ty=
pe)
> > 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 =3D t;
> > +             if (sbi->am.atgc_enabled)
> > +                     return count;
> > +
> > +             if (test_opt(sbi, ATGC) &&
> > +                     le64_to_cpu(sbi->ckpt->elapsed_time) >=3D 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 >=3D DEF_GC_THREAD_AGE_THRESHOLD=
)
>                 am->atgc_enabled =3D 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?
2. dynamically enabling atgc may have other problems. Is this confirmed?
thanks=EF=BC=81
>
> Thanks,
>
> > +                     if (f2fs_init_atgc_curseg(sbi))
> > +                             return -EINVAL;
> > +                     sbi->am.atgc_enabled =3D true;
> > +             }
> > +             return count;
> > +     }
> > +
> >       if (!strcmp(a->attr.name, "gc_segment_mode")) {
> >               if (t < MAX_GC_MODE)
> >                       sbi->gc_segment_mode =3D t;