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

[Qu Wenruo <wqu@suse.com>]

On 2020/12/12 下午6:30, Nikolay Borisov wrote:
> 
> 
> On 12.12.20 г. 7:44 ч., 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:
>>>>>
>>>>>
> 
> <snip>
> 
>>>>>
>>>>>> +        /*
>>>>>> +         * Here we can't call find_extent_buffer() which will
>>>>>> increase
>>>>>> +         * eb->refs.
>>>>>> +         */
>>>>>> +        rcu_read_lock();
>>>>>> +        eb = radix_tree_lookup(&fs_info->buffer_radix,
>>>>>> +                start >> fs_info->sectorsize_bits);
>>>>>> +        rcu_read_unlock();
>>>
>>> Your usage of radix_tree_lookup + rcu lock is wrong. rcu guarantees that
>>> an EB you get won't be freed while the rcu section is active, however
>>> you get a reference to the EB and you do not increment the ref count
>>> WHILE holding the RCU critical section, consult find_extent_buffer
>>> what's the correct usage pattern.
>>
>> Nope, you just fall into the trap what I fell before.
>>
>> Here if the eb has no other referencer, its refs is just 1 (because it's
>> still in the tree).
>>
>> If you go increase the refs, the eb becomes referenced again and
>> release_extent_buffer() won't free it at all.
>>
>> Causing no eb to be freed whatever.
> 
> After the rcu_read_unlock you hold a reference to eb, without having
> incremented the eb's refs, without having locked eb's refs_lock.

Haven't you checked the original try_release_extent_buffer()?

  At this
> point nothing prevents the eb from disappearing from underneath you. The
> correct way would be to increment the eb's ref and check if ref is > 2
> (1 for the buffer radix tree, 1 for you), then you acquire the refs_lock
> and drop your current ref leaving it to 1 and call release_extent_buffer.
> 
>