Re: [PATCH v2] btrfs: always wait for ordered extents to avoid OE races

On Sat, Jun 27, 2026 at 12:32 AM Qu Wenruo <[email protected]> wrote:
>
> [BUG]
> Syzbot reported a bug that there can be conflicting OEs for the same
> range:
>
>  BTRFS critical (device loop4): panic in insert_ordered_extent:264: overlapping ordered extents, existing oe file_offset 16384 num_bytes 430080 flags 0x1089, new oe file_offset 16384 num_bytes 430080 flags 0x80 (errno=-17 Object alrea[  179.162726][ T6897] BTRFS critical (device loop4): panic in insert_ordered_extent:264: overlapping ordered extents, existing oe file_offset 16384 num_bytes 430080 flags 0x1089, new oe file_offset 16384 num_bytes 430080 flags 0x80 (errno=-17 Object already exists)
>  ------------[ cut here ]------------
>  kernel BUG at fs/btrfs/ordered-data.c:264!
>  Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/09/2026
>  RIP: 0010:btrfs_alloc_ordered_extent+0x943/0xad0
>  Call Trace:
>   <TASK>
>   cow_file_range+0x744/0x12a0
>   fallback_to_cow+0x5ea/0xa00
>   run_delalloc_nocow+0x110c/0x17a0
>   btrfs_run_delalloc_range+0xbe4/0x1c20
>   writepage_delalloc+0x104d/0x1ba0
>   btrfs_writepages+0x1667/0x28b0
>   do_writepages+0x338/0x560
>   filemap_fdatawrite_range+0x1f2/0x300
>   btrfs_fdatawrite_range+0x54/0xf0
>   btrfs_direct_write+0x6a0/0xc30
>   btrfs_do_write_iter+0x329/0x790
>   do_iter_readv_writev+0x624/0x8d0
>   vfs_writev+0x34c/0x990
>   __se_sys_pwritev2+0x17a/0x2a0
>   do_syscall_64+0x174/0x580
>   entry_SYSCALL_64_after_hwframe+0x77/0x7f
>   </TASK>
>  ---[ end trace 0000000000000000 ]---
>
> [CAUSE]
> Since commit ff66fe666233 ("btrfs: fix incorrect buffered IO fallback
> for append direct writes"), if the direct IO finished short, we will
> revert the isize back to the original one, so that append writes can be
> respected during the buffered fallback.
>
> Normally we rely on lock_and_cleanup_extent_if_need() function during
> buffered writeback to wait for any existing ordered extents.
>
> But that ordered extent waiting only happens if the start_pos is inside
> the isize.
> Since we have reverted the isize during failed direct IO, we will not
> wait for any ordered extents.
>
> This means we can have a race where the direct IO OE is still in the
> tree, finished but not yet removed, then we're inserting the OE for the
> buffered write, causing the above crash.
>
> [FIX]
> Make the OE wait to be unconditional, to handle the reverted isize
> situation.
>
> And since lock_and_cleanup_extent_if_need() now either lock the
> extents or return -EAGAIN, also remove the branches that handles
> no-extent-locked cases, and rename it to remove the "_if_need" suffix.
>
> The following micro benchmark shows the runtime difference for
> btrfs_buffered_write(), doing `xfs_io -f -c "pwrite 0 1m"` workload,
> all values are the average runtime in nano seconds.
>
>       function runtime              |   before    |     after
>  -----------------------------------+-------------+---------------
>  lock_and_cleanup_extent_if_need()  |     58.2    |    183.0
>  btrfs_buffered_write()             |   2115.6    |   2973.3
>
> The overall runtime of btrfs_buffered_write() is still pretty
> tiny (still less than 3 micro seconds), I'd say the extra cost is still
> acceptable.
>
> An alternative to fix this problem is to wait ordered extents during
> iomap_end() where the isize revert is done.
>
> But that solution will break nowait requirement, as if a nowait direct
> IO finished short, we have to wait for the OEs unconditionally or the
> next append buffered IO can still hit the same problem.
>
> So here we have to move the wait cost to buffered write, but at least
> the code is slightly more streamline.
>
> Reported-by: [email protected]
> Link: https://syzkaller.appspot.com/bug?extid=ba2afde329fc27e3f22e
> Fixes: ff66fe666233 ("btrfs: fix incorrect buffered IO fallback for append direct writes")
> Signed-off-by: Qu Wenruo <[email protected]>

Reviewed-by: Filipe Manana <[email protected]>

Looks good now, thanks.

> ---
> Changelog:
> v2:
> - Further cleanup the handling of lock_and_cleanup_extent_if_need()
>   As it no longer return without locking extents, cleanup the caller
>   furthermore to remove unnecessary local variables.
>
> - Add a micro bench mark for the performance impact
>
> - Explain why we have to do the wait inside buffered write not
>   in direct write path
> ---
>  fs/btrfs/file.c | 104 +++++++++++++++++++-----------------------------
>  1 file changed, 41 insertions(+), 63 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index df8f6f565070..62f41314df04 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -875,70 +875,64 @@ static noinline int prepare_one_folio(struct inode *inode, struct folio **folio_
>
>  /*
>   * Locks the extent and properly waits for data=ordered extents to finish
> - * before allowing the folios to be modified if need.
> + * before allowing the folios to be modified.
>   *
>   * Return:
> - * 1 - the extent is locked
> - * 0 - the extent is not locked, and everything is OK
> + * 0 - the extent is locked
>   * -EAGAIN - need to prepare the folios again
>   */
>  static noinline int
> -lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct folio *folio,
> -                               loff_t pos, size_t write_bytes,
> -                               u64 *lockstart, u64 *lockend, bool nowait,
> -                               struct extent_state **cached_state)
> +lock_and_cleanup_extent(struct btrfs_inode *inode, struct folio *folio,
> +                       loff_t pos, size_t write_bytes,
> +                       u64 *lockstart, u64 *lockend, bool nowait,
> +                       struct extent_state **cached_state)
>  {
>         struct btrfs_fs_info *fs_info = inode->root->fs_info;
> +       struct btrfs_ordered_extent *ordered;
>         u64 start_pos;
>         u64 last_pos;
> -       int ret = 0;
>
>         start_pos = round_down(pos, fs_info->sectorsize);
>         last_pos = round_up(pos + write_bytes, fs_info->sectorsize) - 1;
>
> -       if (start_pos < inode->vfs_inode.i_size) {
> -               struct btrfs_ordered_extent *ordered;
> -
> -               if (nowait) {
> -                       if (!btrfs_try_lock_extent(&inode->io_tree, start_pos,
> -                                                  last_pos, cached_state)) {
> -                               folio_unlock(folio);
> -                               folio_put(folio);
> -                               return -EAGAIN;
> -                       }
> -               } else {
> -                       btrfs_lock_extent(&inode->io_tree, start_pos, last_pos,
> -                                         cached_state);
> -               }
> -
> -               ordered = btrfs_lookup_ordered_range(inode, start_pos,
> -                                                    last_pos - start_pos + 1);
> -               if (ordered &&
> -                   ordered->file_offset + ordered->num_bytes > start_pos &&
> -                   ordered->file_offset <= last_pos) {
> -                       btrfs_unlock_extent(&inode->io_tree, start_pos, last_pos,
> -                                           cached_state);
> +       if (nowait) {
> +               if (!btrfs_try_lock_extent(&inode->io_tree, start_pos,
> +                                          last_pos, cached_state)) {
>                         folio_unlock(folio);
>                         folio_put(folio);
> -                       btrfs_start_ordered_extent(ordered);
> -                       btrfs_put_ordered_extent(ordered);
>                         return -EAGAIN;
>                 }
> -               if (ordered)
> -                       btrfs_put_ordered_extent(ordered);
> -
> -               *lockstart = start_pos;
> -               *lockend = last_pos;
> -               ret = 1;
> +       } else {
> +               btrfs_lock_extent(&inode->io_tree, start_pos, last_pos,
> +                                 cached_state);
>         }
>
> +       ordered = btrfs_lookup_ordered_range(inode, start_pos,
> +                                            last_pos - start_pos + 1);
> +       if (ordered &&
> +           ordered->file_offset + ordered->num_bytes > start_pos &&
> +           ordered->file_offset <= last_pos) {
> +               btrfs_unlock_extent(&inode->io_tree, start_pos, last_pos,
> +                                   cached_state);
> +               folio_unlock(folio);
> +               folio_put(folio);
> +               btrfs_start_ordered_extent(ordered);
> +               btrfs_put_ordered_extent(ordered);
> +               return -EAGAIN;
> +       }
> +       if (ordered)
> +               btrfs_put_ordered_extent(ordered);
> +
> +       *lockstart = start_pos;
> +       *lockend = last_pos;
> +
>         /*
>          * We should be called after prepare_one_folio() which should have locked
>          * all pages in the range.
>          */
>         WARN_ON(!folio_test_locked(folio));
>
> -       return ret;
> +       return 0;
>  }
>
>  /*
> @@ -1195,7 +1189,6 @@ static int copy_one_range(struct btrfs_inode *inode, struct iov_iter *iter,
>         const u64 reserved_start = round_down(start, fs_info->sectorsize);
>         u64 reserved_len;
>         struct folio *folio = NULL;
> -       int extents_locked;
>         u64 lockstart;
>         u64 lockend;
>         bool only_release_metadata = false;
> @@ -1253,18 +1246,16 @@ static int copy_one_range(struct btrfs_inode *inode, struct iov_iter *iter,
>                 reserved_len = last_block - reserved_start;
>         }
>
> -       extents_locked = lock_and_cleanup_extent_if_need(inode, folio, start,
> -                                                        write_bytes, &lockstart,
> -                                                        &lockend, nowait,
> -                                                        &cached_state);
> -       if (extents_locked < 0) {
> -               if (!nowait && extents_locked == -EAGAIN)
> +       ret = lock_and_cleanup_extent(inode, folio, start, write_bytes,
> +                                     &lockstart, &lockend, nowait, &cached_state);
> +       if (ret < 0) {
> +               if (!nowait)
>                         goto again;
>
>                 btrfs_delalloc_release_extents(inode, reserved_len);
>                 release_space(inode, *data_reserved, reserved_start, reserved_len,
>                               only_release_metadata);
> -               return extents_locked;
> +               return ret;
>         }
>
>         copied = copy_folio_from_iter_atomic(folio, offset_in_folio(folio, start),
> @@ -1288,11 +1279,8 @@ static int copy_one_range(struct btrfs_inode *inode, struct iov_iter *iter,
>
>                 /* No copied bytes, unlock, release reserved space and exit. */
>                 if (copied == 0) {
> -                       if (extents_locked)
> -                               btrfs_unlock_extent(&inode->io_tree, lockstart, lockend,
> -                                                   &cached_state);
> -                       else
> -                               btrfs_free_extent_state(cached_state);
> +                       btrfs_unlock_extent(&inode->io_tree, lockstart, lockend,
> +                                           &cached_state);
>                         btrfs_delalloc_release_extents(inode, reserved_len);
>                         release_space(inode, *data_reserved, reserved_start, reserved_len,
>                                       only_release_metadata);
> @@ -1311,17 +1299,7 @@ static int copy_one_range(struct btrfs_inode *inode, struct iov_iter *iter,
>
>         ret = btrfs_dirty_folio(inode, folio, start, copied, &cached_state,
>                                 only_release_metadata);
> -       /*
> -        * If we have not locked the extent range, because the range's start
> -        * offset is >= i_size, we might still have a non-NULL cached extent
> -        * state, acquired while marking the extent range as delalloc through
> -        * btrfs_dirty_page(). Therefore free any possible cached extent state
> -        * to avoid a memory leak.
> -        */
> -       if (extents_locked)
> -               btrfs_unlock_extent(&inode->io_tree, lockstart, lockend, &cached_state);
> -       else
> -               btrfs_free_extent_state(cached_state);
> +       btrfs_unlock_extent(&inode->io_tree, lockstart, lockend, &cached_state);
>
>         btrfs_delalloc_release_extents(inode, reserved_len);
>         if (ret) {
> --
> 2.54.0
>
>