Re: [PATCH v3] btrfs: use ordered extent to grab the logical address for submission

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2026/9/3 03:53, Daniel Vacek 写道:
> On Tue, 1 Sept 2026 at 02:02, Qu Wenruo <wqu@suse.com> wrote:
>> In submit_one_sector() we call btrfs_get_extent() to grab the IO extent
>> map so that we know where the logical location to submit the block.
>>
>> However there is no guarantee that there is an IO extent map for the
>> block, and if there is no IO extent map nor ordered extent,
>> btrfs_get_extent() can grab the file extent from on-disk metadata.
>>
>> That's why we have ASSERT()s to reject holes and compressed file
>> extents.
>>
>> On the other hand, for the write range we should have both an IO extent
>> map and an ordered extent, so there is no reason not to grab the ordered
>> extent instead.
>>
>> There is some minor advantages:
>>
>> - No hole ordered extent
>>    So no need to rely on ASSERT()s to reject hole extents.
>>
>>    And the ASSERT()s are depending on the kernel config, without
>>    CONFIG_BTRFS_ASSERT those ASSERT()s won't even trigger.
>>
>> - No IO errors
>>    Unlike btrfs_get_extent() which can return IO error when doing the
>>    metadata tree search, btrfs_lookup_ordered_extent() will either return
>>    an OE or not found.
>>
>> - Cached OE in bio_ctrl->bbio
>>    At bbio allocation we have already did an OE lookup, and we have a
>>    high chance that the current block also belongs to that OE.
>>    Use that cached OE can reduce the frequency to do an rb-tree search.
>>
>> - Smaller rb-tree
>>    Unlike extent-map-tree, which can contain cached extent maps, the life
>>    span of ordered extents are much shorter, they get removed from the
>>    ordered tree after the file extent item is inserted into the subvolume
>>    tree.
>>
>>    So doing ordered extent tree search can be a tiny faster.
>>
>> And since we're here, also address some minor points:
>>
>> - Add error message for every EUCLEAN error
>>
>> - Remove a dead comment on btrfs_folio_clear_dirty()
>>    We no longer call folio_clear_dirty_for_io() since commit 095be159f3eb
>>    ("btrfs: unify folio dirty flag clearing"), so the folio flag is
>>    still dirty, and the folio dirty flag will be cleared by the last dirty
>>    block.
>>
>> Reviewed-by: Boris Burkov <boris@bur.io>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>
>> ---
>> Changelog:
>> v3:
>> - Refactor the helper to grab the OE and increase its refs in one go
>>
>> v2:
>> - Extract a helper to determine if the @filepos is in the bbio OE range
>>    Which reduces several "bio_ctrl->bbio->ordered" duplication, and use
>>    in_range() to make is easier to read.
>> ---
>>   fs/btrfs/extent_io.c | 64 +++++++++++++++++++++++++++-----------------
>>   1 file changed, 40 insertions(+), 24 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index d7600e5fa3d9..a221b63bdb20 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -1808,6 +1808,22 @@ static noinline_for_stack int writepage_delalloc(struct btrfs_inode *inode,
>>          return 0;
>>   }
>>
>> +static struct btrfs_ordered_extent *get_oe_from_bbio(const struct btrfs_bio *bbio,
>> +                                                    u64 filepos)
>> +{
>> +       struct btrfs_ordered_extent *oe;
>> +
>> +       if (!bbio || !bbio->ordered)
>> +               return NULL;
>> +
>> +       oe = bbio->ordered;
>> +       if (!in_range(filepos, oe->file_offset, oe->num_bytes))
>> +               return NULL;
>> +
>> +       refcount_inc(&oe->refs);
> 
> Thinking about it again, do we even need to raise the refcount?

That is to co-operate with that btrfs_put_ordered_extent().


> 
> One reference is already held by the bbio itself, so the lifetime is guaranteed.
> 
>> +       return oe;
>> +}
>> +
>>   /*
>>    * Return 0 if we have submitted or queued the sector for submission.
>>    * Return <0 for critical errors, and the involved sector will be cleaned up.
>> @@ -1820,11 +1836,10 @@ static int submit_one_sector(struct btrfs_inode *inode,
>>                               loff_t i_size)
>>   {
>>          struct btrfs_fs_info *fs_info = inode->root->fs_info;
>> -       struct extent_map *em;
>> +       struct btrfs_ordered_extent *oe;
>>          u64 block_start;
>>          u64 disk_bytenr;
>>          u64 extent_offset;
>> -       u64 em_end;
>>          const u32 sectorsize = fs_info->sectorsize;
>>          unsigned int queued;
>>
>> @@ -1833,8 +1848,11 @@ static int submit_one_sector(struct btrfs_inode *inode,
>>          /* @filepos >= i_size case should be handled by the caller. */
>>          ASSERT(filepos < i_size);
>>
>> -       em = btrfs_get_extent(inode, NULL, filepos, sectorsize);
>> -       if (IS_ERR(em)) {
>> +       /* Try to reuse the existing OE from bbio first. */
>> +       oe = get_oe_from_bbio(bio_ctrl->bbio, filepos);
>> +       if (!oe)
>          {
>> +               oe = btrfs_lookup_ordered_extent(inode, filepos);
>                  put = true; // or !!oe

I prefer not to introduce an extra if check just for a single case.

So the existing unconditional put and refcount inc for the OE from bbio 
looks fine to me.

Thanks,
Qu


>          }
>> +       if (unlikely(!oe)) {
>>                  /*
>>                   * bio_ctrl may contain a bio crossing several folios.
>>                   * Submit it immediately so that the bio has a chance
>> @@ -1857,31 +1875,25 @@ static int submit_one_sector(struct btrfs_inode *inode,
>>                   */
>>                  btrfs_mark_ordered_io_finished(inode, filepos, fs_info->sectorsize,
>>                                                 false);
>> -               return PTR_ERR(em);
>> +               btrfs_err_rl(fs_info,
>> +               "no ordered extent for root %lld ino %llu filepos %llu",
>> +                            btrfs_root_id(inode->root), btrfs_ino(inode),
>> +                            filepos);
>> +               return -EUCLEAN;
>>          }
>>
>> -       extent_offset = filepos - em->start;
>> -       em_end = btrfs_extent_map_end(em);
>> -       ASSERT(filepos <= em_end);
>> -       ASSERT(IS_ALIGNED(em->start, sectorsize));
>> -       ASSERT(IS_ALIGNED(em->len, sectorsize));
>> +       extent_offset = filepos - oe->file_offset;
>> +       ASSERT(filepos < oe->file_offset + oe->num_bytes);
>> +       ASSERT(IS_ALIGNED(oe->file_offset, sectorsize));
>> +       ASSERT(IS_ALIGNED(oe->num_bytes, sectorsize));
>> +       ASSERT(oe->compress_type == BTRFS_COMPRESS_NONE);
>> +       ASSERT(!test_bit(BTRFS_ORDERED_COMPRESSED, &oe->flags));
>>
>> -       block_start = btrfs_extent_map_block_start(em);
>> -       disk_bytenr = btrfs_extent_map_block_start(em) + extent_offset;
>> +       block_start = oe->disk_bytenr + oe->offset;
>> +       disk_bytenr = block_start + extent_offset;
>>
>> -       ASSERT(!btrfs_extent_map_is_compressed(em));
>> -       ASSERT(block_start != EXTENT_MAP_HOLE);
>> -       ASSERT(block_start != EXTENT_MAP_INLINE);
> 
>          if (put)
> 
>> +       btrfs_put_ordered_extent(oe);
>>
>> -       btrfs_free_extent_map(em);
>> -       em = NULL;
>> -
>> -       /*
>> -        * Although the PageDirty bit is cleared before entering this
>> -        * function, subpage dirty bit is not cleared.
>> -        * So clear subpage dirty bit here so next time we won't submit
>> -        * a folio for a range already written to disk.
>> -        */
>>          btrfs_folio_clear_dirty(fs_info, folio, filepos, sectorsize);
>>          btrfs_folio_set_writeback(fs_info, folio, filepos, sectorsize);
>>          /*
>> @@ -1898,6 +1910,10 @@ static int submit_one_sector(struct btrfs_inode *inode,
>>                  btrfs_folio_clear_writeback(fs_info, folio, filepos, sectorsize);
>>                  btrfs_mark_ordered_io_finished(inode, filepos, fs_info->sectorsize,
>>                                                 false);
>> +               btrfs_err_rl(fs_info,
>> +               "failed to queue sector for root %lld ino %llu filepos %llu",
>> +                            btrfs_root_id(inode->root),
>> +                            btrfs_ino(inode), filepos);
>>                  return -EUCLEAN;
>>          }
>>          return 0;
>> --
>> 2.55.0
>>
>>
>