Re: [PATCH] btrfs: refactor __extent_writepage_io() to do sector-by-sector submission

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

在 2024/8/7 23:48, Josef Bacik 写道:
> On Wed, Aug 07, 2024 at 06:21:00PM +0930, Qu Wenruo wrote:
>> Unlike the bitmap usage inside raid56, for __extent_writepage_io() we
>> handle the subpage submission not sector-by-sector, but for each dirty
>> range we found.
>>
>> This is not a big deal normally, as normally the subpage complex code is
>> already mostly optimized out.
>>
>> For the sake of consistency and for the future of subpage sector-perfect
>> compression support, this patch does:
>>
>> - Extract the sector submission code into submit_one_sector()
>>
>> - Add the needed code to extract the dirty bitmap for subpage case
>>
>> - Use bitmap_and() to calculate the target sectors we need to submit
>>
>> For x86_64, the dirty bitmap will be fixed to 1, with the length of 1,
>> so we're still doing the same workload per sector.
>>
>> For larger page sizes, the overhead will be a little larger, as previous
>> we only need to do one extent_map lookup per-dirty-range, but now it
>> will be one extent_map lookup per-sector.
>
> I'd like this to be a followup, because even in the normal page case (or hell
> the subpage case) we shouldn't have to look up the extent map over and over
> again, it could span a much larger area.

Yep, that's also something I want to improve.

My guess is, using a cached extent_map can improve the situation?

But that will be another patch, purely for performance improvement.

[...]
>>
>> +	if (btrfs_is_subpage(fs_info, inode->vfs_inode.i_mapping)) {
>> +		ASSERT(fs_info->subpage_info);
>> +		btrfs_get_subpage_dirty_bitmap(fs_info, folio, &dirty_bitmap);
>> +		bitmap_size = fs_info->subpage_info->bitmap_nr_bits;
>> +	}
>> +	for (cur = start; cur < start + len; cur += fs_info->sectorsize)
>> +		set_bit((cur - start) >> fs_info->sectorsize_bits, &range_bitmap);
>> +	bitmap_and(&dirty_bitmap, &dirty_bitmap, &range_bitmap, bitmap_size);
>
> I'd rather move this completely under the btrfs_is_subpage() case, we don't need
> to do this where sectorsize == page size.

This has a tiny problem, that page dirty and subpage dirty is not in sync.

For sectorsize == PAGE_SIZE, the set_bit() is only called once, and
bitmap_and() is very cheap for single bit operation too.

The complex part is already under btrfs_is_subpage() check.

In fact the set_bit() and bitmap_and() will later be reused for
inline/compression skip, for both regular sectorsize == page size and
subpage cases.

(There will be another new bitmap introduced into bio_ctrl, to indicate
which sector(s) do not need writeback, thus ther bitmap operations will
be there for that future change)

Thanks,
Qu

>
> Other than that this cleans things up nicely, you can add my
>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
>
> to v2.  Thanks,
>
> Josef
>