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

[Chao Yu <chao@kernel.org>]

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