Re: [f2fs-dev] [PATCH 1/5] f2fs: check number of blocks in a current section

[Daeho Jeong]

On Sun, Feb 25, 2024 at 6:42 PM Chao Yu <chao@xxxxxxxxxx> wrote:
>
> On 2024/2/24 4:55, Jaegeuk Kim wrote:
> > In cfd66bb715fd ("f2fs: fix deadloop in foreground GC"), we needed to check
> > the number of blocks in a section instead of the segment.
> >
> > In addtion, let's check the entire node sections when checking the avaiable
> > node block space. It does not match one to one per temperature, but currently
>
> I tested this patch w/ testcase in [1], it doesn't complain.
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=215914
>
> > we don't have exact dirty page count per temperature. Hence, use a rough
> > estimation.
>
> Do we need more accurate per-temperature dirty node count for extreme
> corner case?
>
> e.g. node_blocks is counted from hot dirty nodes, and current hot_node
> log has no enough free space for it.
>
> Thanks,
>
> >
> > Fixes: cfd66bb715fd ("f2fs: fix deadloop in foreground GC")
> > Signed-off-by: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
> > ---
> >   fs/f2fs/segment.h | 22 +++++++++++-----------
> >   1 file changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> > index 3edd3809b6b5..15bf5edd9b3c 100644
> > --- a/fs/f2fs/segment.h
> > +++ b/fs/f2fs/segment.h
> > @@ -561,23 +561,23 @@ static inline bool has_curseg_enough_space(struct f2fs_sb_info *sbi,
> >                       unsigned int node_blocks, unsigned int dent_blocks)
> >   {
> >
> > -     unsigned int segno, left_blocks;
> > +     unsigned segno, left_blocks;
> >       int i;
> >
> > -     /* check current node segment */
> > +     /* check current node sections, which counts very roughly */
> > +     left_blocks = 0;
> >       for (i = CURSEG_HOT_NODE; i <= CURSEG_COLD_NODE; i++) {
> >               segno = CURSEG_I(sbi, i)->segno;
> > -             left_blocks = f2fs_usable_blks_in_seg(sbi, segno) -
> > -                             get_seg_entry(sbi, segno)->ckpt_valid_blocks;
> > -
> > -             if (node_blocks > left_blocks)
> > -                     return false;
> > +             left_blocks += CAP_BLKS_PER_SEC(sbi) -
> > +                             get_ckpt_valid_blocks(sbi, segno, true);
> >       }
> > +     if (node_blocks > left_blocks)
> > +             return false;

I am not sure this is okay. The current implementation of f2fs may not
be optimal when the HOT_NODE section's space requirements exceed its
current capacity. In such cases, f2fs necessitates the creation of a
new free section to accommodate the overflow, even though there might
be free space available in other NODE sections. To address this issue,
it may be necessary to implement a more accurate accounting system for
NODE sections based on their temperature levels.

> >
> > -     /* check current data segment */
> > +     /* check current data section for dentry blocks. */
> >       segno = CURSEG_I(sbi, CURSEG_HOT_DATA)->segno;
> > -     left_blocks = f2fs_usable_blks_in_seg(sbi, segno) -
> > -                     get_seg_entry(sbi, segno)->ckpt_valid_blocks;
> > +     left_blocks = CAP_BLKS_PER_SEC(sbi) -
> > +                     get_ckpt_valid_blocks(sbi, segno, true);
> >       if (dent_blocks > left_blocks)
> >               return false;
> >       return true;
> > @@ -626,7 +626,7 @@ static inline bool has_not_enough_free_secs(struct f2fs_sb_info *sbi,
> >
> >       if (free_secs > upper_secs)
> >               return false;
> > -     else if (free_secs <= lower_secs)
> > +     if (free_secs <= lower_secs)
> >               return true;
> >       return !curseg_space;
> >   }
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel