Re: [f2fs-dev] [PATCH v3] f2fs: fix to freeze GC and discard threads quickly

[Daeho Jeong]

On Mon, Mar 16, 2026 at 6:56 PM Chao Yu <chao@xxxxxxxxxx> wrote:
>
> On 2026/3/17 02:59, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@xxxxxxxxxx>
> >
> > Suspend can fail if kernel threads do not freeze for a while.
> > f2fs_gc and f2fs_discard threads can perform long-running operations
> > that prevent them from reaching a freeze point in a timely manner.
> >
> > This patch adds explicit freezing checks in the following locations:
> > 1. f2fs_gc: Added a check at the 'retry' label to exit the loop quickly
> >     if freezing is requested, especially during heavy GC rounds.
> > 2. __issue_discard_cmd: Added a 'suspended' flag to break both inner and
> >     outer loops during discard command issuance if freezing is detected
> >     after at least one command has been issued.
> > 3. __issue_discard_cmd_orderly: Added a similar check for orderly discard
> >     to ensure responsiveness.
> >
> > These checks ensure that the threads release locks safely and enter the
> > frozen state.
> >
> > Signed-off-by: Daeho Jeong <daehojeong@xxxxxxxxxx>
> > ---
> > v3: removed unnecessary suspended check
> > v2: add freezing check in do_garbage_collect()
> > ---
> >   fs/f2fs/gc.c      | 10 ++++++++++
> >   fs/f2fs/segment.c | 12 +++++++++++-
> >   2 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index 246496fbe5e5..2305f16cbabb 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -1889,12 +1889,18 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
> >                               sbi->next_victim_seg[gc_type] =
> >                                       (cur_segno + 1 < sec_end_segno) ?
> >                                       cur_segno + 1 : NULL_SEGNO;
> > +
> > +                     if (unlikely(freezing(current))) {
> > +                             folio_put_refs(sum_folio, 2);
> > +                             goto stop;
>
> Do we need to return EBUSY or something like that to indicate GC was
> interrupted?

In the case of do_garbage_collect(), it is originally designed to stop before
cleaning the entire section depending on the migration_window size or
zoned GC operations, and it returns the count of segments reclaimed until
that point. Therefore, returning the segment count reclaimed up to the
moment of suspension seems more consistent with the existing behavior.
If we return an error instead, the caller might miss the count of segments
that were actually reclaimed.

>
> > +                     }
> >               }
> >   next_block:
> >               folio_put_refs(sum_folio, 2);
> >               segno = block_end_segno;
> >       }
> >
> > +stop:
> >       if (submitted)
> >               f2fs_submit_merged_write(sbi, data_type);
> >
> > @@ -1968,6 +1974,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control)
> >               goto stop;
> >       }
> >   retry:
> > +     if (unlikely(freezing(current))) {
> > +             ret = 0;
>
> Ditto.

For f2fs_gc(), if we return -EBUSY, the caller might perceive it as a
situation where no more victims
can be selected. This could lead to an issue where the idle time is
incorrectly extended under the
"no GC" condition.

>
> Thanks,
>
> > +             goto stop;
> > +     }
> >       ret = __get_victim(sbi, &segno, gc_type, gc_control->one_time);
> >       if (ret) {
> >               /* allow to search victim from sections has pinned data */
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index e9b6d774b985..0c4fb4270185 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -1606,6 +1606,9 @@ static void __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi,
> >               if (dc->state != D_PREP)
> >                       goto next;
> >
> > +             if (*issued > 0 && unlikely(freezing(current)))
> > +                     break;
> > +
> >               if (dpolicy->io_aware && !is_idle(sbi, DISCARD_TIME)) {
> >                       io_interrupted = true;
> >                       break;
> > @@ -1645,6 +1648,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> >       struct blk_plug plug;
> >       int i, issued;
> >       bool io_interrupted = false;
> > +     bool suspended = false;
> >
> >       if (dpolicy->timeout)
> >               f2fs_update_time(sbi, UMOUNT_DISCARD_TIMEOUT);
> > @@ -1675,6 +1679,11 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> >               list_for_each_entry_safe(dc, tmp, pend_list, list) {
> >                       f2fs_bug_on(sbi, dc->state != D_PREP);
> >
> > +                     if (issued > 0 && unlikely(freezing(current))) {
> > +                             suspended = true;
> > +                             break;
> > +                     }
> > +
> >                       if (dpolicy->timeout &&
> >                               f2fs_time_over(sbi, UMOUNT_DISCARD_TIMEOUT))
> >                               break;
> > @@ -1694,7 +1703,8 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> >   next:
> >               mutex_unlock(&dcc->cmd_lock);
> >
> > -             if (issued >= dpolicy->max_requests || io_interrupted)
> > +             if (issued >= dpolicy->max_requests || io_interrupted ||
> > +                                     suspended)
> >                       break;
> >       }
> >
>