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

[Chao Yu]

On 5/3/26 18:16, Cen Zhang wrote:
> 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.

Cen,

I see, thanks for the explanation.

Can you please update commit message w/ the explanation? That will be helpful
for reviewer and git blame.

Thanks,

> 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