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

On Thu, Jun 25, 2026 at 6:41 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_needed() 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_needed() can no longer turn 0, we
> can also remove the branches that handles no-extent-locked cases.
>
> 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]>
> ---
>  fs/btrfs/file.c | 77 +++++++++++++++++++------------------------------
>  1 file changed, 30 insertions(+), 47 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index df8f6f565070..ef31140e0942 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -879,7 +879,6 @@ static noinline int prepare_one_folio(struct inode *inode, struct folio **folio_
>   *
>   * Return:
>   * 1 - the extent is locked
> - * 0 - the extent is not locked, and everything is OK

So it stops making sense to return 1...
Just change the function to always return 0, meaning the extent range
is locked, or -EAGAIN.

>   * -EAGAIN - need to prepare the folios again
>   */
>  static noinline int
> @@ -889,6 +888,7 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct folio *folio,
>                                 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;

Here initialized to 0, but we can never return such value anymore.

> @@ -896,42 +896,38 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct folio *folio,
>         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;
> +       ret = 1;

And this is pointless, right below we can use "return 1;" and get rid
of the 'ret' variable.

Since returning 1 no longer makes sense, just make it return 0 and remove 'ret'.

Also in the caller, since lock_and_cleanup_extent_if_need() now always
returns with the extent range locked, we can get rid of it's local
variable named 'extents_locked' and use 'ret' as it does not make
sense anymore.

Finally, with this change we now always look for ordered extents and
lock the range. Previously, we only did this if writes were within
i_size - which makes sense because, except for his new direct IO case
falling back to buffered IO, we could never find ordered extents
beyond i_size.

Have you checked whether performance is affected?

Thanks.

> +
>         /*
>          * We should be called after prepare_one_folio() which should have locked
>          * all pages in the range.
> @@ -1288,11 +1284,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 +1304,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
>
>