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

[Chao Yu]

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?

- f2fs_fill_super
 - f2fs_build_segment_manager
  - create discard thread
 - f2fs_build_node_manager
  - allocate memory sbi->nm_info
...
 - f2fs_destroy_node_manager
				- issue_discard_thread
		                 - f2fs_available_free_memory()
  - free sbi->nm_info
                                  - UAF: access sbi->nm_info->ram_thresh

Thanks,

> Keep create_discard_cmd_control() limited to allocating and publishing
> the command-control object, which recovery can use to queue discard
> commands. Start the discard thread later in f2fs_fill_super(), after POR,
> recovery, checkpoint option handling, and discard tuning have completed.
> If starting the thread fails, unwind the shrinker and any GC thread that
> may already have been started before continuing through the existing
> mount-failure cleanup path.
>
> Fixes: d6d2b491a82e1e411a6766fbfb87c697d8701554 ("f2fs: allow to change discard policy based on cached discard cmds")
> Signed-off-by: Cen Zhang <zzzccc427@xxxxxxxxx>
> ---
>   fs/f2fs/segment.c | 17 ++++-------------
>   fs/f2fs/super.c   | 12 ++++++++++++
>   2 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 8390994a8..deb98f564 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2302,12 +2302,10 @@ int f2fs_start_discard_thread(struct f2fs_sb_info *sbi)
>   static int create_discard_cmd_control(struct f2fs_sb_info *sbi)
>   {
>   	struct discard_cmd_control *dcc;
> -	int err = 0, i;
> +	int i;
>   
> -	if (SM_I(sbi)->dcc_info) {
> -		dcc = SM_I(sbi)->dcc_info;
> -		goto init_thread;
> -	}
> +	if (SM_I(sbi)->dcc_info)
> +		return 0;
>   
>   	dcc = f2fs_kzalloc(sbi, sizeof(struct discard_cmd_control), GFP_KERNEL);
>   	if (!dcc)
> @@ -2344,14 +2342,7 @@ static int create_discard_cmd_control(struct f2fs_sb_info *sbi)
>   
>   	init_waitqueue_head(&dcc->discard_wait_queue);
>   	SM_I(sbi)->dcc_info = dcc;
> -init_thread:
> -	err = f2fs_start_discard_thread(sbi);
> -	if (err) {
> -		kfree(dcc);
> -		SM_I(sbi)->dcc_info = NULL;
> -	}
> -
> -	return err;
> +	return 0;
>   }
>   
>   static void destroy_discard_cmd_control(struct f2fs_sb_info *sbi)
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 40079fd78..8228be53d 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -5340,6 +5340,15 @@ static int f2fs_fill_super(struct super_block *sb, struct fs_context *fc)
>   
>   	f2fs_tuning_parameters(sbi);
>   
> +	/*
> +	 * After POR and mount-time recovery, we can run the discard thread. It
> +	 * reads node-manager memory thresholds and the superblock read-only
> +	 * state, so keep it out of the fill_super() initialization window.
> +	 */
> +	err = f2fs_start_discard_thread(sbi);
> +	if (err)
> +		goto leave_shrinker;
> +
>   	f2fs_notice(sbi, "Mounted with checkpoint version = %llx",
>   		    cur_cp_version(F2FS_CKPT(sbi)));
>   	f2fs_update_time(sbi, CP_TIME);
> @@ -5349,6 +5358,9 @@ static int f2fs_fill_super(struct super_block *sb, struct fs_context *fc)
>   	sbi->umount_lock_holder = NULL;
>   	return 0;
>   
> +leave_shrinker:
> +	f2fs_leave_shrinker(sbi);
> +	f2fs_stop_gc_thread(sbi);
>   sync_free_meta:
>   	/* safe to flush all the data */
>   	sync_filesystem(sbi->sb);