Re: [f2fs-dev] [PATCH 2/7] f2fs: read summary blocks with the correct amount for migration_granularity

[Daeho Jeong]

On Thu, Sep 5, 2024 at 7:56 PM Chao Yu <chao@xxxxxxxxxx> wrote:
>
> On 2024/8/30 5:52, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@xxxxxxxxxx>
> >
> > Now we do readahead for a full section by not considering
> > migration_granularity and it triggers unnecessary read. So, make it read
> > with the correct amount.
> >
> > Signed-off-by: Daeho Jeong <daehojeong@xxxxxxxxxx>
> > ---
> >   fs/f2fs/gc.c | 33 ++++++++++++++++++++-------------
> >   1 file changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index 46e3bc26b78a..b5d3fd40b17a 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -1708,24 +1708,33 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> >       struct blk_plug plug;
> >       unsigned int segno = start_segno;
> >       unsigned int end_segno = start_segno + SEGS_PER_SEC(sbi);
> > +     unsigned int sec_end_segno;
> >       int seg_freed = 0, migrated = 0;
> >       unsigned char type = IS_DATASEG(get_seg_entry(sbi, segno)->type) ?
> >                                               SUM_TYPE_DATA : SUM_TYPE_NODE;
> >       unsigned char data_type = (type == SUM_TYPE_DATA) ? DATA : NODE;
> >       int submitted = 0;
> >
> > -     if (__is_large_section(sbi))
> > -             end_segno = rounddown(end_segno, SEGS_PER_SEC(sbi));
> > +     if (__is_large_section(sbi)) {
> > +             sec_end_segno = rounddown(end_segno, SEGS_PER_SEC(sbi));
> >
> > -     /*
> > -      * zone-capacity can be less than zone-size in zoned devices,
> > -      * resulting in less than expected usable segments in the zone,
> > -      * calculate the end segno in the zone which can be garbage collected
> > -      */
> > -     if (f2fs_sb_has_blkzoned(sbi))
> > -             end_segno -= SEGS_PER_SEC(sbi) -
> > +             /*
> > +              * zone-capacity can be less than zone-size in zoned devices,
> > +              * resulting in less than expected usable segments in the zone,
> > +              * calculate the end segno in the zone which can be garbage
> > +              * collected
> > +              */
> > +             if (f2fs_sb_has_blkzoned(sbi))
> > +                     sec_end_segno -= SEGS_PER_SEC(sbi) -
> >                                       f2fs_usable_segs_in_sec(sbi, segno);
> >
> > +             if (gc_type == BG_GC)
> > +                     end_segno = start_segno + sbi->migration_granularity;
> > +
> > +             if (end_segno > sec_end_segno)
> > +                     end_segno = sec_end_segno;
> > +     }
> > +
> >       sanity_check_seg_type(sbi, get_seg_entry(sbi, segno)->type);
> >
> >       /* readahead multi ssa blocks those have contiguous address */
> > @@ -1762,9 +1771,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> >
> >               if (get_valid_blocks(sbi, segno, false) == 0)
> >                       goto freed;
> > -             if (gc_type == BG_GC && __is_large_section(sbi) &&
> > -                             migrated >= sbi->migration_granularity)
>
> It seems we change the logic from migrating "migration_granularity" segments which
> has valid blocks to scanning "migration_granularity" segments and try migrating
> valid blocks in those segments.
>
> IIUC, when background GC recycle sparse zone, it will take gc thread more round,
> it seems low efficient. How do you think of keeping previous implementation?

I got your point. However, with zoned devices having 1GB sections, per
every round, we should
touch almost 2MB size of ssa block pages, even though we didn't need
to do it. Maybe, we can introduce
another sysfs node like migration_window_limit, which can be set as
double as migration_granuality by default,
limiting the size of scanning.

>
> Thanks,
>
> > -                     goto skip;
> >               if (!PageUptodate(sum_page) || unlikely(f2fs_cp_error(sbi)))
> >                       goto skip;
> >
> > @@ -1803,7 +1809,8 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> >
> >               if (__is_large_section(sbi))
> >                       sbi->next_victim_seg[gc_type] =
> > -                             (segno + 1 < end_segno) ? segno + 1 : NULL_SEGNO;
> > +                             (segno + 1 < sec_end_segno) ?
> > +                                     segno + 1 : NULL_SEGNO;
> >   skip:
> >               f2fs_put_page(sum_page, 0);
> >       }
>