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

[Chao Yu]

On 2026/3/18 00:15, Daeho Jeong wrote:
> 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.

It makes sense.

In addition, I think freezing(current) will only be true in context of gc thread,
for such background migration condition, we don't care the error number.

Thanks,

> > > +                     }
> > >                }
> > >    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;
> > >        }