[PATCH] f2fs: start discard thread after mount recovery

[PATCH] f2fs: start discard thread after mount recovery

[Cen Zhang]

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.

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@gmail.com>
---
 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);
-- 
2.43.0

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@gmail.com>
> ---
>   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);

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

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

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

[Cen Zhang]

Dear Chao. Yu

> 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,

I will send v2 shortly.

Best regards,
Cen