linux-kernel - Re: [f2fs-dev] [PATCH] f2fs: do not use granularity control for segment or section unit discard

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] [day] [month] [year] [list]

Message-ID: <CACOAw_yOYnJ+zsFS339u8tonQNZkM9kjkAdouD9gooydQL0Zaw@mail.gmail.com>
Date: Fri, 21 Feb 2025 07:54:26 -0800
From: Daeho Jeong <daeho43@...il.com>
To: Chao Yu <chao@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net, 
	kernel-team@...roid.com, Daeho Jeong <daehojeong@...gle.com>
Subject: Re: [f2fs-dev] [PATCH] f2fs: do not use granularity control for
 segment or section unit discard

On Thu, Feb 20, 2025 at 6:19 PM Chao Yu <chao@...nel.org> wrote:
>
> On 2025/2/20 23:49, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@...gle.com>
> >
> > When we support segment or section unit discard, we should only focus on
> > how actively we submit discard commands for only one type of size, such
> > as segment or section. In this case, we don't have to manage smaller
> > sized discards.
> >
> > Reported-by: Yohan Joung <yohan.joung@...com>
> > Signed-off-by: Daeho Jeong <daehojeong@...gle.com>
> > ---
> >   fs/f2fs/segment.c | 29 ++++++++++++++++++++---------
> >   1 file changed, 20 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index c282e8a0a2ec..4316ff7aa0d1 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -1661,12 +1661,20 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> >                               f2fs_time_over(sbi, UMOUNT_DISCARD_TIMEOUT))
> >                       break;
> >
> > -             if (i + 1 < dpolicy->granularity)
> > -                     break;
> > +             /*
> > +              * Do not granularity control for segment or section
> > +              * unit discard, since we have only one type of discard length.
> > +              */
> > +             if (f2fs_block_unit_discard(sbi)) {
> > +                     if (i + 1 < dpolicy->granularity)
> > +                             break;
> >
> > -             if (i + 1 < dcc->max_ordered_discard && dpolicy->ordered) {
> > -                     __issue_discard_cmd_orderly(sbi, dpolicy, &issued);
> > -                     return issued;
> > +                     if (i + 1 < dcc->max_ordered_discard &&
> > +                                     dpolicy->ordered) {
> > +                             __issue_discard_cmd_orderly(sbi, dpolicy,
> > +                                             &issued);
> > +                             return issued;
> > +                     }
> >               }
> >
> >               pend_list = &dcc->pend_list[i];
> > @@ -1701,6 +1709,13 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> >
> >               if (issued >= dpolicy->max_requests || io_interrupted)
> >                       break;
> > +
> > +             /*
> > +              * We only use the largest discard unit for segment or
> > +              * section unit discard.
> > +              */
> > +             if (!f2fs_block_unit_discard(sbi))
> > +                     break;
> >       }
> >
> >       if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> > @@ -2320,10 +2335,6 @@ static int create_discard_cmd_control(struct f2fs_sb_info *sbi)
> >       dcc->discard_granularity = DEFAULT_DISCARD_GRANULARITY;
> >       dcc->max_ordered_discard = DEFAULT_MAX_ORDERED_DISCARD_GRANULARITY;
> >       dcc->discard_io_aware = DPOLICY_IO_AWARE_ENABLE;
> > -     if (F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SEGMENT)
> > -             dcc->discard_granularity = BLKS_PER_SEG(sbi);
> > -     else if (F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SECTION)
> > -             dcc->discard_granularity = BLKS_PER_SEC(sbi);
>
> Hi Daeho,
>
> I think this bug was introduced by commit 4f993264fe29 ("f2fs: introduce
> discard_unit mount option"), since it set discard_granularity to section
> size incorrectly for discard_unit=section mount option, once section size
> is large than segment size, discard_granularity will be larger than 512.
>
> However, w/ current implementation, we only support range of [1, 512] for
> discard_granularity parameter, resulting in failing to submitting all
> dicards.
>
> So, what do you think of setting discard_granularity to 512 for both
> discard_unit=segment and discard_unit=section mount option, as I proposed
> in [1]? Then, discard_thread in DPOLICY_BG mode can submit those large-sized
> discards.
>
> [1] https://lore.kernel.org/linux-f2fs-devel/53598146-1f01-41ad-980e-9f4b989e81ab@kernel.org/

Yes, it makes sense. Thanks.

>
> Thanks,
>
> >
> >       INIT_LIST_HEAD(&dcc->entry_list);
> >       for (i = 0; i < MAX_PLIST_NUM; i++)
>