Re: [PATCH RFC] Btrfs: only subtract from len_to_oe_boundary when it is tracking an extent

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

On 2023/8/1 02:10, Chris Mason wrote:
> On 7/31/23 3:02 AM, Christoph Hellwig wrote:
>> On Mon, Jul 31, 2023 at 10:27:02AM +0800, Qu Wenruo wrote:
>>> Personally speaking, I think we'd better moving the ordered extent based
>>> split (only for zoned devices) to btrfs bio layer.
>>
>> That goes completely counter the direction I've been working to.  The
>> ordered extent is the "container" for writeback, so having a bio
>> that spawns them creates all kinds of problems.  Thats's the reason
>> why we now have a bbio->ordered pointer now.
>>
>>> Another concern is, how we could hit a bio which has a size larger than
>>> U32_MAX?
>>>
>>> The bio->bi_iter.size is only unsigned int, it should never exceed U32_MAX.
>>>
>>> It would help a lot if you can provide a backtrace of such unaligned bio.
>>
>> That's indeed a bit weird.
>>
>
> bio_full() is using a slightly different test:
>
>          if (bio->bi_iter.bi_size > UINT_MAX - len)
>                  return true;
>
> We're doing:
>
>                  /* Cap to the current ordered extent boundary if there is one. */
>                  if (len > bio_ctrl->len_to_oe_boundary) {
>                          ASSERT(bio_ctrl->compress_type == BTRFS_COMPRESS_NONE);
>                          ASSERT(is_data_inode(&inode->vfs_inode));
>                          len = bio_ctrl->len_to_oe_boundary;
>                  }
>
> The end result of this is that when we get to a
> bio sized U32_MAX - PAGE_SIZE, both our len_to_oe_boundary and the
> bio_add_page() check will correctly decide we can only fit 4095 bytes.

Thanks for the details, now I can understand where the problem is.

Mind to add above explanation into the commit message?
Otherwise the fix looks good to me.

Thanks,
Qu
>
> The difference is bio_add_page() would just say no triggering bio
> submission.  But submit_extent_page()'s check is first, cutting the
> IO down to 4095 bytes instead.
>
> (I do have the stack trace, it's just a boring filemap_fdata_write_and_wait() path off of file release)
>
> -chris
>