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 г. 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. 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.