linux-kernel - Re: [PATCH] f2fs: enable atgc if atgc_age_threshold from user is less than elapsed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 3 Jun 2024 17:05:36 +0800
From: Zhiguo Niu <niuzhiguo84@...il.com>
To: Chao Yu <chao@...nel.org>
Cc: Zhiguo Niu <zhiguo.niu@...soc.com>, jaegeuk@...nel.org, 
	linux-f2fs-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org, 
	ke.wang@...soc.com
Subject: Re: [PATCH] f2fs: enable atgc if atgc_age_threshold from user is less
 than elapsed_time

On Mon, Jun 3, 2024 at 3:41 PM Chao Yu <chao@...nel.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_threshold
> > meanwhile this vale is less than elapsed_time and ATGC mount option is on.
> >
> > Signed-off-by: Zhiguo Niu <zhiguo.niu@...soc.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_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?
2. dynamically enabling atgc may have other problems. Is this confirmed?
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;