Received-SPF: pass (google.com: domain of linux-kernel+bounces-82039-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: <20240223205535.307307-1-jaegeuk@kernel.org> <afa444c5-7ac1-457e-9559-6d7c6623c426@kernel.org>
In-Reply-To: <afa444c5-7ac1-457e-9559-6d7c6623c426@kernel.org>
From: Daeho Jeong <daeho43@gmail.com>
Date: Mon, 26 Feb 2024 09:17:50 -0800
Message-ID: <CACOAw_zpmxHnG_n4pyKzqWEY2cGyJ5fQXuWfBgDx8-V0Nqu8QA@mail.gmail.com>
Subject: Re: [f2fs-dev] [PATCH 1/5] f2fs: check number of blocks in a current section
To: Chao Yu <chao@kernel.org>
Cc: Jaegeuk Kim <jaegeuk@kernel.org>, linux-kernel@vger.kernel.org, 
	linux-f2fs-devel@lists.sourceforge.net
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, Feb 25, 2024 at 6:42=E2=80=AFPM Chao Yu <chao@kernel.org> wrote:
>
> On 2024/2/24 4:55, Jaegeuk Kim wrote:
> > In cfd66bb715fd ("f2fs: fix deadloop in foreground GC"), we needed to c=
heck
> > the number of blocks in a section instead of the segment.
> >
> > In addtion, let's check the entire node sections when checking the avai=
able
> > node block space. It does not match one to one per temperature, but cur=
rently
>
> I tested this patch w/ testcase in [1], it doesn't complain.
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=3D215914
>
> > we don't have exact dirty page count per temperature. Hence, use a roug=
h
> > 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@kernel.org>
> > ---
> >   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_block=
s)
> >   {
> >
> > -     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 =3D 0;
> >       for (i =3D CURSEG_HOT_NODE; i <=3D CURSEG_COLD_NODE; i++) {
> >               segno =3D CURSEG_I(sbi, i)->segno;
> > -             left_blocks =3D f2fs_usable_blks_in_seg(sbi, segno) -
> > -                             get_seg_entry(sbi, segno)->ckpt_valid_blo=
cks;
> > -
> > -             if (node_blocks > left_blocks)
> > -                     return false;
> > +             left_blocks +=3D 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 =3D CURSEG_I(sbi, CURSEG_HOT_DATA)->segno;
> > -     left_blocks =3D f2fs_usable_blks_in_seg(sbi, segno) -
> > -                     get_seg_entry(sbi, segno)->ckpt_valid_blocks;
> > +     left_blocks =3D 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 <=3D lower_secs)
> > +     if (free_secs <=3D lower_secs)
> >               return true;
> >       return !curseg_space;
> >   }
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel