Re: [PATCH] f2fs: start discard thread after mount recovery

[Cen Zhang]

Dear Chao Yu

Thanks for taking a look, and sorry for the confusion.

>
> On 5/3/26 12:58, Cen Zhang wrote:
> > The discard command control is built while the segment manager is
> > constructed, before the node manager is built and before mount recovery
> > has completed. Starting the discard thread from that constructor lets the
> > background thread run while f2fs_fill_super() is still publishing and
> > initializing mount-time state.
> >
> > After commit d6d2b491a82e ("f2fs: allow to change discard policy based
> > on cached discard cmds"), issue_discard_thread() may consult node-manager
> > memory thresholds through f2fs_available_free_memory(). It can therefore
> > observe sbi->nm_info while f2fs_build_node_manager() is publishing and
> > initializing it. The same early-start window also lets the thread test
> > the superblock read-only state while recovery paths still make temporary
> > SB_RDONLY transitions.
>
> Not sure I understood you correctly, do you mean this?
>
No, I did not mean the old UAF-style failure path. That path should
already be covered by the existing cleanup ordering:

free_nm:
        f2fs_stop_discard_thread(sbi);
        f2fs_destroy_node_manager(sbi);

What I was trying to describe is an earlier window during mount
initialization, when the discard thread is actually started:

- f2fs_fill_super()
  - f2fs_build_segment_manager()
    - create_discard_cmd_control()
      - f2fs_start_discard_thread()
  - f2fs_build_node_manager()
    - sbi->nm_info = f2fs_kzalloc(...)
    - init_node_manager() initializes fields such as ram_thresh, counters,
      locks/lists/bitmaps, etc.

At this point issue_discard_thread() may run concurrently and call
f2fs_available_free_memory(). That helper reads NM_I(sbi), returns true
only if it is NULL, and otherwise uses node-manager fields such as
nm_i->ram_thresh. So my concern is that the thread may observe a newly
published but still being initialized node manager, rather than a freed
one.

The data race report maps to the following paths in v7.0.3:

  issue_discard_thread()                fs/f2fs/segment.c:1921
    f2fs_available_free_memory()        fs/f2fs/node.c:50
      NM_I()                            fs/f2fs/f2fs.h:2228

racing with mount-time node-manager initialization:

  f2fs_fill_super()                     fs/f2fs/super.c:5151
    f2fs_build_node_manager()           fs/f2fs/node.c:3420

There is also a similar early-start window for the SB_RDONLY check in
issue_discard_thread(), since mount recovery can still make temporary
superblock flag transitions before f2fs_fill_super() reaches the stable
mounted state. The second report maps to:

  issue_discard_thread()                fs/f2fs/segment.c:1935
    f2fs_readonly()                     fs/f2fs/f2fs.h:3665

racing with mount recovery restoring the superblock flags:

  f2fs_fill_super()                     fs/f2fs/super.c:5267
    f2fs_recover_fsync_data()           fs/f2fs/recovery.c:953

So the patch is intended to fix this lifecycle ordering: keep the discard
command control available early, but defer starting the background
discard thread until mount recovery and node-manager initialization have
completed.

Please let me know if I missed anything here.

Best regards,
Cen