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

[Nikolay Borisov <nborisov@suse.com>]

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?




> 
> For the first run, it release the only eb of the page, and cleared page
> private.
> For the second run, since private is cleared, we need to break out.
> 
>>
>>> +		spin_lock(&page->mapping->private_lock);
>>> +		if (!PagePrivate(page)) {
>>> +			spin_unlock(&page->mapping->private_lock);
>>> +			break;
>>> +		}

Aren't we guaranteed that a page has private if this function is called ?

>>> +		subpage = (struct btrfs_subpage *)page->private;
>>> +		spin_unlock(&page->mapping->private_lock);
>>> +
>>> +		spin_lock_irqsave(&subpage->lock, flags);
>>> +		if (!(tmp & subpage->tree_block_bitmap))  {
>>> +			spin_unlock_irqrestore(&subpage->lock, flags);
>>> +			bit_start++;
>>> +			continue;
>>> +		}
>>> +		spin_unlock_irqrestore(&subpage->lock, flags);
>>> +
>>> +		start = bit_start * fs_info->sectorsize + page_start;
>>> +		bit_start += fs_info->nodesize >> fs_info->sectorsize_bits;

<snip>

> Thanks,
> Qu
>>
>>
>>> +		/*
>>> +		 * 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.

Frankly the locking in this function is insane, first mapping->private
lock is acquired to check if Page_private is set and then page->private
is referenced but that is not signalled at all. Then subpage->lock is
taken to check the tree_block_bitmap, then the lock is dropped. At that
point no locks are held so this page could possibly be referenced by
someone else? Then the buggy locking is used to get the eb, then you
lock refs_lock and call release_extent_buffer...

>>> +		ASSERT(eb);

Doing this outside of the rcu read side critical section _without_
incrementing the ref count is buggy!

>>> +		spin_lock(&eb->refs_lock);
>>> +		if (atomic_read(&eb->refs) != 1 || extent_buffer_under_io(eb) ||
>>> +		    !test_and_clear_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags)) {
>>> +			spin_unlock(&eb->refs_lock);
>>> +			continue;
>>> +		}
>>> +		/*


<snip>