Re: [PATCH 2/2] btrfs: subpage: pack all subpage bitmaps into a larger bitmap

[Nikolay Borisov <nborisov@suse.com>]

On 16.08.21 г. 16:41, Qu Wenruo wrote:
> 
> 
> On 2021/8/16 下午6:26, Nikolay Borisov wrote:
>>
>>
>> On 16.08.21 г. 9:00, Qu Wenruo wrote:
>>> Currently we use u16 bitmap to make 4k sectorsize work for 64K page
>>> size.
>>>
>>> But this u16 bitmap is not large enough to contain larger page size like
>>> 128K, nor is space efficient for 16K page size.
>>>
>>> To handle both cases, here we pack all subpage bitmaps into a larger
>>> bitmap, now btrfs_subpage::bitmaps[] will be the ultimate bitmap for
>>> subpage usage.
>>>
>>> Each sub-bitmap will has its start bit number recorded in
>>> btrfs_subpage_info::*_start, and its bitmap length will be recorded in
>>> btrfs_subpage_info::bitmap_nr_bits.
>>>
>>> All subpage bitmap operations will be converted from using direct u16
>>> operations to bitmap operations, with above *_start calculated.
>>>
>>> For 64K page size with 4K sectorsize, this should not cause much
>>> difference.
>>> While for 16K page size, we will only need 1 unsigned long (u32) to
>>> restore the bitmap.
>>> And will be able to support 128K page size in the future.
>>>

<snip>

>>
>> offtopic: Instead of having a bunch of those checks can't we replace
>> them with ASSERTS and ensure that the decision whether we do subpage or
>> not is taken at a higher level ?
> 
> Nope, in this particular call site, btrfs_alloc_subpage() can be called
> with regular page size.
> 
> I guess it's better to rename this function like btrfs_prepare_page()?

There are currently 2 callers:

btrfs_attach_subpage - this one only calls it iff sectorsize != alloc_subage

alloc_extent_buffer - here it's called unconditionally but that can
easily be rectified with an if(subpage) check

<snip>


>> 2 argument of find_next_zero_bit is 'size' which would be nbits as it
>> expects the size to be in bits , not start + nbit. Every logical bitmap
>> in ->bitmaps is defined by [start, nbits] no ? Unfortunately there is a
>> discrepancy between the order of documentation and the order of actual
>> arguments in the definition of this function....
> 
> IT'S A TRAP!
> 
> Paramater 2 (@size) is the total size of the search range, it should be
> larger than the 3rd parameter.

It's not even that it's the end index of the bit we are looking for. I.e
if we want to check bits 20-29 we'd pass 20 as start, and 29 as size ...
This is fucked, but it is what it is. I guess the documentation of the
bits function is dodgy...

<snip>