Re: [PATCH v2 12/18] btrfs: extent_io: implement try_release_extent_buffer() for subpage metadata support

[Nikolay Borisov <nborisov@suse.com>]

On 12.12.20 г. 3:28 ч., Qu Wenruo wrote:
> 
> 
> On 2020/12/12 上午12:57, Nikolay Borisov wrote:
>>
>>
>> On 11.12.20 г. 14:11 ч., Qu Wenruo wrote:
>>>
>>>
>>> On 2020/12/11 下午8:00, Nikolay Borisov wrote:
>>>>
>>>>
>>>> On 10.12.20 г. 8:38 ч., Qu Wenruo wrote:
>>>>> Unlike the original try_release_extent_buffer,
>>>>> try_release_subpage_extent_buffer() will iterate through
>>>>> btrfs_subpage::tree_block_bitmap, and try to release each extent
>>>>> buffer.
>>>>>
>>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>>> ---
>>>>>   fs/btrfs/extent_io.c | 73
>>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 73 insertions(+)
>>>>>
>>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>>> index 141e414b1ab9..4d55803302e9 100644
>>>>> --- a/fs/btrfs/extent_io.c
>>>>> +++ b/fs/btrfs/extent_io.c
>>>>> @@ -6258,10 +6258,83 @@ void memmove_extent_buffer(const struct
>>>>> extent_buffer *dst,
>>>>>       }
>>>>>   }
>>>>>
>>>>> +static int try_release_subpage_extent_buffer(struct page *page)
>>>>> +{
>>>>> +    struct btrfs_fs_info *fs_info =
>>>>> btrfs_sb(page->mapping->host->i_sb);
>>>>> +    u64 page_start = page_offset(page);
>>>>> +    int bitmap_size = BTRFS_SUBPAGE_BITMAP_SIZE;
>>>>
>>>> Remove this variable and directly use BTRFS_SUBPAGE_BITMAP_SIZE as a
>>>> terminating condition
>>>>
>>>>> +    int bit_start = 0;
>>>>> +    int ret;
>>>>> +
>>>>> +    while (bit_start < bitmap_size) {
>>>>
>>>> You really want to iterate for a fixed number of items so switch
>>>> that to
>>>> a for loop.
>>>
>>> The problem here is, it's not always fixed.
>>>
>>> If it finds one bit set, it will skip (nodesize >> sectorsize_bits)
>>> bits.
>>>
>>> But if not found, it will skip to just next bit.
>>>
>>> Thus I'm not sure if for loop is really a good choice here for
>>> differential step.
>>>
>>>>
>>>>> +        struct btrfs_subpage *subpage;
>>>>> +        struct extent_buffer *eb;
>>>>> +        unsigned long flags;
>>>>> +        u16 tmp = 1 << bit_start;
>>>>> +        u64 start;
>>>>> +
>>>>> +        /*
>>>>> +         * Make sure the page still has private, as previous run can
>>>>> +         * detach the private
>>>>> +         */
>>>>
>>>> But if previous run has run it would have disposed of this eb and you
>>>> won't find this page at all, no ?
>>>
>>> For the "previous run" I mean, previous iteration in the same loop.
>>>
>>> E.g. the page has 4 bits set, just one eb (16K nodesize).
>>
>> Isn't it guaranteed that if you iterate the eb's in a page if you meet
>> an empty block then the whole extent buffer is gone, hence instead of
>> doing bit_start++ you ought to also increment by the size of nodesize.
>>
>> For example, assume a page contains 4 EBs:
>>
>> 0|1|2|3|4|5|6|7|8|9|10|11|12|13|14|15
>> x|x|x|x|0|0|0|0|x|x| x|x |0 | 0|0 |0
>>
>> So first bit is set, so you proceed to call release_extent_buffer on it,
>> which clears the first 4 bits in tree_block_bitmap, in this case you've
>> incremented by nodesize so next iteration begins at index 4. You detect
>> it's unset (0) hence you increment it byte 1 and you repeat this for the
>> next 3 bits, then you free the whole of the next eb. I argue that you
>> also need to increment by nodesize in the case of a bit which is not
>> set, because you cannot really see partially freed eb i.e you cannot see
>> the following state:
>>
>> 0|1|2|3|4|5|6|7|8|9|10|11|12|13|14|15
>> x|x|x|x|x|0|0|0|x|x| x|x |0 | 0|0 |0
>>
>> Am I missing something?
> 
> It's not for partly freed eb, but nodesize unaligned eb.
> 
> E.g. if we have a eb starts at sector 1 of a page, your nodesize based
> iteration would go crazy.
> Although we have ensured no subpage eb can cross page boundary, but it's
> not the same requirement for nodesize alignment.
> 
> Thus I uses the extra safe way for the empty bit.

Which of course cannot happen because the allocator ensures that
returned addresses are always aligned to fs_info::stripeize which in
turn is always equal to sectorsize... So you add extra complexity for no
apparent reason making code which is already subtle to be even more subtle.

<snip>