[v1] f2fs: sync pending discards before reserving device alias

[PATCH] f2fs: sync pending discards before reserving device alias

Posted by Wenjie Qi 4 weeks, 1 day ago

A released device-alias range can accumulate pending discard
commands. The reserve path then marks the same range valid again
through f2fs_reserve_device_alias() without first synchronizing
those discards.

If the discard thread later submits one of the stale commands,
__check_sit_bitmap() can still see valid blocks in that range and
trigger a BUG.

Synchronize pending discard commands in the alias range before
making it valid again.

Fixes: eae3faf210bd ("f2fs: support dynamic reserve/release for device aliasing")
Cc: stable@vger.kernel.org
Signed-off-by: Wenjie Qi <qiwenjie@xiaomi.com>
---
  kernel BUG at fs/f2fs/segment.c:1228!
  Oops: invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN PTI
  CPU: 1 UID: 0 PID: 79 Comm: f2fs_discard-25
  RIP: 0010:__check_sit_bitmap+0x2e1/0x4e0
  Call Trace:
   __submit_discard_cmd+0x921/0x1140
   __issue_discard_cmd+0x524/0x12f0
   issue_discard_thread+0x686/0xe20

 fs/f2fs/f2fs.h    | 2 ++
 fs/f2fs/file.c    | 1 +
 fs/f2fs/segment.c | 9 +++++++++
 3 files changed, 12 insertions(+)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 16720f1f0a9..2b9c6bf310b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -4067,6 +4067,8 @@ int f2fs_start_discard_thread(struct f2fs_sb_info *sbi);
 void f2fs_drop_discard_cmd(struct f2fs_sb_info *sbi);
 void f2fs_stop_discard_thread(struct f2fs_sb_info *sbi);
 bool f2fs_issue_discard_timeout(struct f2fs_sb_info *sbi, bool need_check);
+void f2fs_wait_discard_bios(struct f2fs_sb_info *sbi,
+			    block_t blkaddr, unsigned int len);
 void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
 					struct cp_control *cpc);
 void f2fs_dirty_to_prefree(struct f2fs_sb_info *sbi);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index d440231b8cb..58cea2aa314 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3847,6 +3847,7 @@ static int f2fs_ioc_reserve_dev_alias(struct file *filp)
 	write_unlock(&et->lock);
 	clear_inode_flag(inode, FI_NO_EXTENT);
 
+	f2fs_wait_discard_bios(sbi, ei.blk, ei.len);
 	f2fs_reserve_device_alias(sbi, ei.blk, ei.len);
 
 	i_size_write(inode, (loff_t)ei.len << sbi->log_blocksize);
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 1e7e745be71..7f4fe8c34c2 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1916,6 +1916,15 @@ static void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
 		__wait_one_discard_bio(sbi, dc);
 }
 
+void f2fs_wait_discard_bios(struct f2fs_sb_info *sbi,
+			    block_t blkaddr, unsigned int len)
+{
+	block_t end = blkaddr + len;
+
+	while (blkaddr < end)
+		f2fs_wait_discard_bio(sbi, blkaddr++);
+}
+
 void f2fs_stop_discard_thread(struct f2fs_sb_info *sbi)
 {
 	struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
-- 
2.43.0

Re: [f2fs-dev] [PATCH] f2fs: sync pending discards before reserving device alias

Posted by Daeho Jeong 4 weeks, 1 day ago

On Fri, Aug 28, 2026 at 4:44 AM Wenjie Qi <qwjhust@gmail.com> wrote:
>
> A released device-alias range can accumulate pending discard
> commands. The reserve path then marks the same range valid again
> through f2fs_reserve_device_alias() without first synchronizing
> those discards.
>
> If the discard thread later submits one of the stale commands,
> __check_sit_bitmap() can still see valid blocks in that range and
> trigger a BUG.
>
> Synchronize pending discard commands in the alias range before
> making it valid again.
>
> Fixes: eae3faf210bd ("f2fs: support dynamic reserve/release for device aliasing")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wenjie Qi <qiwenjie@xiaomi.com>
> ---
>   kernel BUG at fs/f2fs/segment.c:1228!
>   Oops: invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN PTI
>   CPU: 1 UID: 0 PID: 79 Comm: f2fs_discard-25
>   RIP: 0010:__check_sit_bitmap+0x2e1/0x4e0
>   Call Trace:
>    __submit_discard_cmd+0x921/0x1140
>    __issue_discard_cmd+0x524/0x12f0
>    issue_discard_thread+0x686/0xe20
>
>  fs/f2fs/f2fs.h    | 2 ++
>  fs/f2fs/file.c    | 1 +
>  fs/f2fs/segment.c | 9 +++++++++
>  3 files changed, 12 insertions(+)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 16720f1f0a9..2b9c6bf310b 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -4067,6 +4067,8 @@ int f2fs_start_discard_thread(struct f2fs_sb_info *sbi);
>  void f2fs_drop_discard_cmd(struct f2fs_sb_info *sbi);
>  void f2fs_stop_discard_thread(struct f2fs_sb_info *sbi);
>  bool f2fs_issue_discard_timeout(struct f2fs_sb_info *sbi, bool need_check);
> +void f2fs_wait_discard_bios(struct f2fs_sb_info *sbi,
> +                           block_t blkaddr, unsigned int len);
>  void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
>                                         struct cp_control *cpc);
>  void f2fs_dirty_to_prefree(struct f2fs_sb_info *sbi);
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index d440231b8cb..58cea2aa314 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3847,6 +3847,7 @@ static int f2fs_ioc_reserve_dev_alias(struct file *filp)
>         write_unlock(&et->lock);
>         clear_inode_flag(inode, FI_NO_EXTENT);
>
> +       f2fs_wait_discard_bios(sbi, ei.blk, ei.len);
>         f2fs_reserve_device_alias(sbi, ei.blk, ei.len);
>
>         i_size_write(inode, (loff_t)ei.len << sbi->log_blocksize);
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 1e7e745be71..7f4fe8c34c2 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1916,6 +1916,15 @@ static void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
>                 __wait_one_discard_bio(sbi, dc);
>  }
>
> +void f2fs_wait_discard_bios(struct f2fs_sb_info *sbi,
> +                           block_t blkaddr, unsigned int len)
> +{
> +       block_t end = blkaddr + len;
> +
> +       while (blkaddr < end)
> +               f2fs_wait_discard_bio(sbi, blkaddr++);
> +}
> +
>  void f2fs_stop_discard_thread(struct f2fs_sb_info *sbi)
>  {
>         struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> --
> 2.43.0
>

Hi Wenjie,

Thanks for catching this discard race and for sending the patch.

This is a critical issue, but we need to address it differently:
1. Data corruption risk during the released state:
Once released, external applications write data directly to the raw
block device (/dev/block/...). If delayed discard commands remain in
the background queue, F2FS's discard thread will asynchronously erase
the data written by the external application.
Thus, handling this at `reserve` is too late—we must clean up all
discards at `release` time before returning to userspace.
2. Performance issue with block iteration:
Looping block-by-block (`while (blkaddr < end)`) causes millions of
lock acquisitions and rbtree lookups for an 8GB+ device.

Fix:
In `f2fs_ioc_release_dev_alias()`, right after
`f2fs_write_checkpoint()`, we should either:
- Drop/cancel pending discard commands for that range from the discard tree, or
- Synchronously flush and wait for all discard commands in `[ei.blk,
ei.blk + ei.len)`

If you'd like, I can write and post the fix for this. Let me know what
you think.

Thanks,

>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: sync pending discards before reserving device alias

Posted by Wenjie Qi 4 weeks ago

Hi Daeho,

Thanks for the review.

I agree that walking the range block by block is too expensive. I have
reworked v3 to walk overlapping discard commands, cancel prepared
intervals in bulk, and wait for each submitted command only once.

Regarding the synchronization point, my understanding of the documented
ownership model is:

- reserve: In-service to Aliased, handing the range to external use
- release: Aliased to In-service, returning the range to F2FS allocation

Therefore, external raw-device I/O should stop before release and start
again only after reserve succeeds. While the range is released and owned
by F2FS, normal block allocation already waits for a pending discard
before reusing each block.

Also, cleaning the range only after the release checkpoint would not
cover new discard commands generated while F2FS owns the range before a
later reserve. For that reason, v3 keeps the final range synchronization
under sentry_lock at the reserve boundary, before marking the range
valid.

I have sent v3 separately with the command-range implementation. Does
this match the intended ownership contract?

Thanks,
Wenjie