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

On Thu, Jun 25, 2026 at 11:39 PM Qu Wenruo <[email protected]> wrote:
>
>
>
> 在 2026/6/25 20:59, Filipe Manana 写道:
> [...]
> >
> >> @@ -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?
>
> Here is the microbenchmark for the runtime of btrfs_buffered_write() and
> lock_and_cleanup_extent_if_need().
>
> The workload is a very basic xfs_io -f -c "pwrite 0 1m", which is always
> appending the isize for every 4K block.
>
> Before patch:
> - lock_and_cleanup_extent_if_need():
>
>    runtime avg = 58.16 ns
>    runtime stdev = 4.2
>
> - btrfs_buffered_write():
>
>    runtime avg = 2115.6 ns
>    runtime stdev = 1822.1
>
>
> Patched:
> - lock_and_cleanup_extent_if_need():
>
>    runtime avg = 183.0 ns
>    runtime stdev = 161.6
>
> - btrfs_buffered_write():
>
>    runtime avg = 2973.3 ns
>    runtime stdev = 4225.1
>
> So yes, the runtime of lock_and_cleanup_extent_if_need() has greatly
> increased, by more than 3 times, but still less than 1 micro second.
>
> The btrfs_buffered_write() itself is also much slower due to the extra
> extent unlock, the runtime is increased by 40%, but still less than 3 us.
>
> I'm not sure if the cost is acceptable or not. The increased runtime is
> pretty huge for the affected worst case scenario, and that's undeniable.
>
> But the overall runtime of btrfs_buffered_write() itself is still at
> single digit microsecond level.
>
> If acceptable, I'll add the microbenchmark into the commit message in
> the next update.

Instead of this approach, can we wait for the ordered extent(s) in DIO
path that reverts i_size after the short write?

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