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

[Daeho Jeong]

On Fri, Aug 28, 2026 at 8:17 PM Wenjie Qi <qwjhust@xxxxxxxxx> wrote:
>
> Releasing a device alias can leave pending discard commands covering
> its range. Reserving the alias makes the same blocks valid again. If a
> pending command is submitted afterward, __check_sit_bitmap() observes
> valid blocks and triggers a BUG.
>
> The reserve ioctl is the ownership transition from F2FS allocation to
> external use. While the range is released, normal F2FS allocation
> already synchronizes a pending discard before reusing each block. A
> release-time drain would also not cover commands generated before a
> later reserve, so synchronize the range at the reserve boundary.
>
> Synchronize discard commands for the alias range under sentry_lock
> before marking the range valid. Walk overlapping commands instead of
> every block, cancel prepared command intervals in bulk, and wait each
> submitted command once. Use a 64-bit cursor and exclusive end so a range
> ending at 2^32 blocks does not wrap.
>
> Fixes: eae3faf210bd ("f2fs: support dynamic reserve/release for device aliasing")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Wenjie Qi <qiwenjie@xxxxxxxxxx>
> ---
> Changes since v2:
> - Walk overlapping discard commands instead of every filesystem block.
> - Use a 64-bit range cursor and exclusive end.
> - Make the commit message describe the standalone final change.
> - Clarify reserve as the ownership transition to external use.
>
>   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/segment.c | 159 +++++++++++++++++++++++++++-------------------
>  1 file changed, 94 insertions(+), 65 deletions(-)
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 1e7e745be71d..c4fb1e5f9a34 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1069,26 +1069,6 @@ static bool f2fs_check_discard_tree(struct f2fs_sb_info *sbi)
>         return true;
>  }
>
> -static struct discard_cmd *__lookup_discard_cmd(struct f2fs_sb_info *sbi,
> -                                               block_t blkaddr)
> -{
> -       struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> -       struct rb_node *node = dcc->root.rb_root.rb_node;
> -       struct discard_cmd *dc;
> -
> -       while (node) {
> -               dc = rb_entry(node, struct discard_cmd, rb_node);
> -
> -               if (blkaddr < dc->di.lstart)
> -                       node = node->rb_left;
> -               else if (blkaddr >= dc->di.lstart + dc->di.len)
> -                       node = node->rb_right;
> -               else
> -                       return dc;
> -       }
> -       return NULL;
> -}
> -
>  static struct discard_cmd *__lookup_discard_cmd_ret(struct rb_root_cached *root,
>                                 block_t blkaddr,
>                                 struct discard_cmd **prev_entry,
> @@ -1114,7 +1094,7 @@ static struct discard_cmd *__lookup_discard_cmd_ret(struct rb_root_cached *root,
>
>                 if (blkaddr < dc->di.lstart)
>                         pnode = &(*pnode)->rb_left;
> -               else if (blkaddr >= dc->di.lstart + dc->di.len)
> +               else if ((u64)blkaddr >= (u64)dc->di.lstart + dc->di.len)
>                         pnode = &(*pnode)->rb_right;
>                 else
>                         goto lookup_neighbors;
> @@ -1472,42 +1452,56 @@ static void __relocate_discard_cmd(struct discard_cmd_control *dcc,
>         list_move_tail(&dc->list, &dcc->pend_list[plist_idx(dc->di.len)]);
>  }
>
> -static void __punch_discard_cmd(struct f2fs_sb_info *sbi,
> -                               struct discard_cmd *dc, block_t blkaddr)
> +static void __punch_discard_cmd_range(struct f2fs_sb_info *sbi,
> +                                     struct discard_cmd *dc, block_t lstart,
> +                                     unsigned int len)
>  {
>         struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
>         struct discard_info di = dc->di;
> +       u64 end = (u64)lstart + len;
> +       u64 di_end = (u64)di.lstart + di.len;
>         bool modified = false;
>
> -       if (dc->state == D_DONE || dc->di.len == 1) {
> +       f2fs_bug_on(sbi, dc->state != D_PREP || !len ||
> +                   lstart < di.lstart || end > di_end);
> +
> +       if (lstart == di.lstart && end == di_end) {
>                 __remove_discard_cmd(sbi, dc);
>                 return;
>         }
>
>         dcc->undiscard_blks -= di.len;
> -
> -       if (blkaddr > di.lstart) {
> -               dc->di.len = blkaddr - dc->di.lstart;
> +       if (lstart > di.lstart) {
> +               dc->di.len = lstart - di.lstart;
>                 dcc->undiscard_blks += dc->di.len;
>                 __relocate_discard_cmd(dcc, dc);
>                 modified = true;
>         }
>
> -       if (blkaddr < di.lstart + di.len - 1) {
> +       if (end < di_end) {
> +               block_t right_lstart = (block_t)end;
> +               unsigned int right_len = (unsigned int)(di_end - end);
> +               block_t right_start = di.start + right_lstart - di.lstart;
> +
>                 if (modified) {
> -                       __insert_discard_cmd(sbi, dc->bdev, blkaddr + 1,
> -                                       di.start + blkaddr + 1 - di.lstart,
> -                                       di.lstart + di.len - 1 - blkaddr);
> +                       __insert_discard_cmd(sbi, dc->bdev, right_lstart,
> +                                            right_start, right_len);
>                 } else {
> -                       dc->di.lstart++;
> -                       dc->di.len--;
> -                       dc->di.start++;
> -                       dcc->undiscard_blks += dc->di.len;
> +                       dc->di.lstart = right_lstart;
> +                       dc->di.start = right_start;
> +                       dc->di.len = right_len;
> +                       dcc->undiscard_blks += right_len;
>                         __relocate_discard_cmd(dcc, dc);
>                 }
>         }
>  }
>
> +static void __punch_discard_cmd(struct f2fs_sb_info *sbi,
> +                               struct discard_cmd *dc, block_t blkaddr)
> +{
> +       __punch_discard_cmd_range(sbi, dc, blkaddr, 1);
> +}
> +
>  static void __update_discard_tree_range(struct f2fs_sb_info *sbi,
>                                 struct block_device *bdev, block_t lstart,
>                                 block_t start, block_t len)
> @@ -1872,48 +1866,82 @@ static unsigned int __wait_all_discard_cmd(struct f2fs_sb_info *sbi,
>  }
>
>  /* This should be covered by global mutex, &sit_i->sentry_lock */
> -static void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
> +static void f2fs_wait_discard_bios(struct f2fs_sb_info *sbi,
> +                                  block_t blkaddr, unsigned int len)
>  {
>         struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> -       struct discard_cmd *dc;
> -       bool need_wait = false;
> +       u64 cursor = blkaddr;
> +       u64 end = cursor + len;
>
> -       mutex_lock(&dcc->cmd_lock);
> -       dc = __lookup_discard_cmd(sbi, blkaddr);
> -#ifdef CONFIG_BLK_DEV_ZONED
> -       if (dc && f2fs_sb_has_blkzoned(sbi) && bdev_is_zoned(dc->bdev)) {
> -               int devi = f2fs_bdev_index(sbi, dc->bdev);
> +       f2fs_bug_on(sbi, end > (u64)U32_MAX + 1);
> +
> +       while (cursor < end) {
> +               struct discard_cmd *prev_dc = NULL, *next_dc = NULL;
> +               struct discard_cmd *dc;
> +               struct rb_node **insert_p = NULL, *insert_parent = NULL;
> +               u64 dc_end, overlap_end;
> +               unsigned int overlap_len;
> +               bool need_wait = false;
>
> -               if (devi < 0) {
> +               mutex_lock(&dcc->cmd_lock);
> +               dc = __lookup_discard_cmd_ret(&dcc->root, (block_t)cursor,
> +                                             &prev_dc, &next_dc, &insert_p, &insert_parent);
> +               if (!dc)
> +                       dc = next_dc;
> +               if (!dc || (u64)dc->di.lstart >= end) {
>                         mutex_unlock(&dcc->cmd_lock);
> -                       return;
> +                       break;
>                 }
>
> -               if (f2fs_blkz_is_seq(sbi, devi, dc->di.start)) {
> -                       /* force submit zone reset */
> -                       if (dc->state == D_PREP)
> -                               __submit_zone_reset_cmd(sbi, dc, REQ_SYNC,
> -                                                       &dcc->wait_list, NULL);
> -                       dc->ref++;
> -                       mutex_unlock(&dcc->cmd_lock);
> -                       /* wait zone reset */
> -                       __wait_one_discard_bio(sbi, dc);
> -                       return;
> +               if (cursor < dc->di.lstart)
> +                       cursor = dc->di.lstart;
> +               dc_end = (u64)dc->di.lstart + dc->di.len;
> +               overlap_end = min(end, dc_end);
> +               overlap_len = (unsigned int)(overlap_end - cursor);
> +
> +#ifdef CONFIG_BLK_DEV_ZONED
> +               if (f2fs_sb_has_blkzoned(sbi) && bdev_is_zoned(dc->bdev)) {
> +                       int devi = f2fs_bdev_index(sbi, dc->bdev);
> +
> +                       if (devi < 0) {
> +                               mutex_unlock(&dcc->cmd_lock);
> +                               return;
> +                       }
> +
> +                       if (f2fs_blkz_is_seq(sbi, devi, dc->di.start)) {
> +                               if (dc->state == D_PREP)
> +                                       __submit_zone_reset_cmd(sbi, dc, REQ_SYNC,
> +                                                               &dcc->wait_list, NULL);
> +                               dc->ref++;
> +                               need_wait = true;
> +                       }
>                 }
> -       }
>  #endif
> -       if (dc) {
> -               if (dc->state == D_PREP) {
> -                       __punch_discard_cmd(sbi, dc, blkaddr);
> -               } else {
> -                       dc->ref++;
> -                       need_wait = true;
> +               if (!need_wait) {
> +                       if (dc->state == D_PREP) {
> +                               if (len == 1)
> +                                       __punch_discard_cmd(sbi, dc,
> +                                                           (block_t)cursor);
> +                               else
> +                                       __punch_discard_cmd_range(sbi, dc,
> +                                                                 (block_t)cursor,
> +                                                                 overlap_len);
> +                       } else {
> +                               dc->ref++;
> +                               need_wait = true;
> +                       }
>                 }
> +               mutex_unlock(&dcc->cmd_lock);
> +               if (need_wait)
> +                       __wait_one_discard_bio(sbi, dc);
> +               cursor = overlap_end;
>         }
> -       mutex_unlock(&dcc->cmd_lock);
> +}
>
> -       if (need_wait)
> -               __wait_one_discard_bio(sbi, dc);
> +static void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi,
> +                                 block_t blkaddr)
> +{
> +       f2fs_wait_discard_bios(sbi, blkaddr, 1);
>  }
>
>  void f2fs_stop_discard_thread(struct f2fs_sb_info *sbi)
> @@ -2684,6 +2712,7 @@ void f2fs_reserve_device_alias(struct f2fs_sb_info *sbi, block_t addr,
>         seg_num = GET_SEGNO(sbi, addr_end) - segno + 1;
>
>         down_write(&sit_i->sentry_lock);
> +       f2fs_wait_discard_bios(sbi, addr, len);
>
>         if (seg_num == 1)
>                 cnt = len;
> --

Hi Wenjie,

Thanks for the updated v3 patch.

While synchronizing pending discards before reserving the device alias is the
right direction to prevent the __check_sit_bitmap() BUG, I see two major issues
with this implementation:
1. Critical Lock Contention (Sleeping under down_write(&sit_i->sentry_lock)):
   In your patch, f2fs_wait_discard_bios() is called inside
f2fs_reserve_device_alias()
   after acquiring down_write(&sit_i->sentry_lock):
       down_write(&sit_i->sentry_lock);
       f2fs_wait_discard_bios(sbi, addr, len);
   `sentry_lock` is one of the most contended global locks in F2FS. Sleeping
   for storage I/O completion while holding
`down_write(&sit_i->sentry_lock)` will
   freeze all concurrent block allocations and write operations across
the entire
   system for hundreds of milliseconds.
2. Unnecessary Code Complexity:
   Modifying the core discard punch logic with __punch_discard_cmd_range() and
   re-inserting split discard entries into the rbtree via __insert_discard_cmd()
   introduces substantial complexity and potential regression risks to core
   discard paths. For device aliasing, reservation covers the whole
target device
   range, so overlapping pending commands can simply be dropped in bulk.

### Proposed Solution
We can resolve this much more simply and safely by introducing a
lightweight range
helper `f2fs_drop_discard_cmd_range(sbi, start, len)` and calling it inside
`f2fs_ioc_reserve_dev_alias()` BEFORE acquiring `sentry_lock`:
- It drops pending D_PREP commands in O(log N) and waits for in-flight
bios under
  `dcc->cmd_lock` without holding `sentry_lock` (zero global lock contention).
- It requires only ~35 lines of self-contained code without altering existing
  punch/lookup mechanisms.
- It completely eliminates the __check_sit_bitmap() crash.

I have already verified this cleaner fix and will send out the patch
shortly for review.

Thanks,

> 2.43.0