| 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
| ||
|
Message-ID: <CAHJ8P3LNpZamiva_Ktck+tRKXvyAAYK0dg_z2Mwjiq41aeMF0Q@mail.gmail.com>
Date: Thu, 19 Sep 2024 10:23:16 +0800
From: Zhiguo Niu <niuzhiguo84@...il.com>
To: Chao Yu <chao@...nel.org>
Cc: liuderong@...o.com, jaegeuk@...nel.org, linux-kernel@...r.kernel.org,
linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH v2 2/2] f2fs: introduce get_section_mtime
Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@...ts.sourceforge.net>
于2024年9月18日周三 14:45写道：
>
> On 2024/9/12 14:40, liuderong@...o.com wrote:
> > From: liuderong <liuderong@...o.com>
> >
> > When segs_per_sec is larger than 1, section may contain free segments,
> > mtime should be the mean value of each valid segments,
> > so introduce get_section_mtime to exclude free segments in a section.
> >
> > Signed-off-by: liuderong <liuderong@...o.com>
> > ---
> > fs/f2fs/f2fs.h | 2 ++
> > fs/f2fs/gc.c | 15 ++-------------
> > fs/f2fs/segment.c | 41 ++++++++++++++++++++++++++++++++++++-----
> > 3 files changed, 40 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 4dcdcdd..d6adf0f 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -3762,6 +3762,8 @@ enum rw_hint f2fs_io_type_to_rw_hint(struct f2fs_sb_info *sbi,
> > unsigned int f2fs_usable_segs_in_sec(struct f2fs_sb_info *sbi);
> > unsigned int f2fs_usable_blks_in_seg(struct f2fs_sb_info *sbi,
> > unsigned int segno);
> > +unsigned long long get_section_mtime(struct f2fs_sb_info *sbi,
> > + unsigned int segno);
>
> Hi Derong,
>
> It needs to add "f2fs_" prefix for get_section_mtime() to avoid global
> namespace pollution.
>
> >
> > #define DEF_FRAGMENT_SIZE 4
> > #define MIN_FRAGMENT_SIZE 1
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index 6299639..03c6117 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -332,20 +332,14 @@ static unsigned int check_bg_victims(struct f2fs_sb_info *sbi)
> > static unsigned int get_cb_cost(struct f2fs_sb_info *sbi, unsigned int segno)
> > {
> > struct sit_info *sit_i = SIT_I(sbi);
> > - unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
> > - unsigned int start = GET_SEG_FROM_SEC(sbi, secno);
> > unsigned long long mtime = 0;
> > unsigned int vblocks;
> > unsigned char age = 0;
> > unsigned char u;
> > - unsigned int i;
> > unsigned int usable_segs_per_sec = f2fs_usable_segs_in_sec(sbi);
> >
> > - for (i = 0; i < usable_segs_per_sec; i++)
> > - mtime += get_seg_entry(sbi, start + i)->mtime;
> > + mtime = get_section_mtime(sbi, segno);
> > vblocks = get_valid_blocks(sbi, segno, true);
> > -
> > - mtime = div_u64(mtime, usable_segs_per_sec);
> > vblocks = div_u64(vblocks, usable_segs_per_sec);
> >
> > u = BLKS_TO_SEGS(sbi, vblocks * 100);
> > @@ -485,10 +479,7 @@ static void add_victim_entry(struct f2fs_sb_info *sbi,
> > struct victim_sel_policy *p, unsigned int segno)
> > {
> > struct sit_info *sit_i = SIT_I(sbi);
> > - unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
> > - unsigned int start = GET_SEG_FROM_SEC(sbi, secno);
> > unsigned long long mtime = 0;
> > - unsigned int i;
> >
> > if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
> > if (p->gc_mode == GC_AT &&
> > @@ -496,9 +487,7 @@ static void add_victim_entry(struct f2fs_sb_info *sbi,
> > return;
> > }
> >
> > - for (i = 0; i < SEGS_PER_SEC(sbi); i++)
> > - mtime += get_seg_entry(sbi, start + i)->mtime;
> > - mtime = div_u64(mtime, SEGS_PER_SEC(sbi));
> > + mtime = get_section_mtime(sbi, segno);
> >
> > /* Handle if the system time has changed by the user */
> > if (mtime < sit_i->min_mtime)
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 6627394..e62e722 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -5389,6 +5389,41 @@ unsigned int f2fs_usable_segs_in_sec(struct f2fs_sb_info *sbi)
> > return SEGS_PER_SEC(sbi);
> > }
> >
> > +unsigned long long get_section_mtime(struct f2fs_sb_info *sbi,
> > + unsigned int segno)
> > +{
> > + unsigned int usable_segs_per_sec = f2fs_usable_segs_in_sec(sbi);
> > + unsigned int secno = 0, start = 0;
> > + struct free_segmap_info *free_i = FREE_I(sbi);
> > + unsigned int valid_seg_count = 0;
> > + unsigned long long mtime = 0;
> > + unsigned int i = 0;
> > +
> > + if (segno == NULL_SEGNO)
> > + return 0;
>
> No needed.
>
> > +
> > + secno = GET_SEC_FROM_SEG(sbi, segno);
> > + start = GET_SEG_FROM_SEC(sbi, secno);
> > +
> > + if (!__is_large_section(sbi))
> > + return get_seg_entry(sbi, start + i)->mtime;
> > +
> > + for (i = 0; i < usable_segs_per_sec; i++) {
> > + /* for large section, only check the mtime of valid segments */
> > + spin_lock(&free_i->segmap_lock);
> > + if (test_bit(start + i, free_i->free_segmap)) {
> > + mtime += get_seg_entry(sbi, start + i)->mtime;
> > + valid_seg_count++;
> > + }
> > + spin_unlock(&free_i->segmap_lock);
> > + }
>
> After commit 6f3a01ae9b72 ("f2fs: record average update time of segment"),
> mtime of segment starts to indicate average update time of segment.
>
> So it needs to change like this?
>
> for (i = 0; i < usable_segs_per_sec; i++) {
> struct seg_entry *se = get_seg_entry(sbi, start + i);
>
> mtime += se->mtime * se->valid_blocks;
> total_valid_blocks += se->valid_blocks;
> }
hi Chao,
after I read this patch from Derong and base on your this comment,
I have some doubts：
mtime is update in update_segment_mtime, and this API is called by
more than one path， such as f2fs_invalidate_blocks and f2fs_allocate_data_block,
and se->mtime is calculated by the following flow if se->mtime is not null.
--------------------------------
se->mtime = div_u64(se->mtime * se->valid_blocks + mtime,
se->valid_blocks + 1);
--------------------------------
if this is called from f2fs_invalidate_blocks, se->mtime is still calculated by
mtime / se->valid_blocks + 1, but the real value of se->valid_blocks will
will be reduced 1，So isn’t it a bit inaccurate just calculating valid
blocks in this patch?
thanks!
>
> if (total_valid_blocks == 0)
> return 0;
>
> return div_u64(mtime, total_valid_blocks);
>
> Thanks,
>
> > +
> > + if (valid_seg_count == 0)
> > + return 0;
> > +
> > + return div_u64(mtime, valid_seg_count);
> > +}
> > +
> > /*
> > * Update min, max modified time for cost-benefit GC algorithm
> > */
> > @@ -5402,13 +5437,9 @@ static void init_min_max_mtime(struct f2fs_sb_info *sbi)
> > sit_i->min_mtime = ULLONG_MAX;
> >
> > for (segno = 0; segno < MAIN_SEGS(sbi); segno += SEGS_PER_SEC(sbi)) {
> > - unsigned int i;
> > unsigned long long mtime = 0;
> >
> > - for (i = 0; i < SEGS_PER_SEC(sbi); i++)
> > - mtime += get_seg_entry(sbi, segno + i)->mtime;
> > -
> > - mtime = div_u64(mtime, SEGS_PER_SEC(sbi));
> > + mtime = get_section_mtime(sbi, segno);
> >
> > if (sit_i->min_mtime > mtime)
> > sit_i->min_mtime = mtime;
>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@...ts.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Powered by blists - more mailing lists