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

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

On 2020/12/12 下午5:26, Nikolay Borisov wrote:
>
>
> 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...

Nope again.
Think again, sectorsize is only 4K, while nodesize is 16K.

So it's valid (not really good though) to have eb bytenr which is only
aligned to 4K but not aligned to 16K.


> So you add extra complexity for no
> apparent reason making code which is already subtle to be even more subtle.
>
> <snip>
>