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/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).

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;
>> +		}
>> +		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;
> 
> By doing this you are really saying "skip all blocks pertaining to this
> eb". In order for this to be correct it would imply that bit_start
> should _always_ be 0,4,8,12 - am I correct? 

Nope. As long as no eb crosses page boundary, it won't cause problem.
So in theory we support case like eb spans sector 1~5.

> But what happens if
> if (!(tmp & subpage->tree_block_bitmap))  has executed and bit_start is
> now 1, then you'd make start = page_start + 4k , skip next 4(16k) blocks
> but that would be wrong, no ?

For (!(tmp & subpage->tree_block_bitmap)) branch, isn't bit_start just
increased by one?
Exactly like what I said, we will check next sector, until we hit the
first bit set.

And only when we hit a bit, we increase the bit_start by nodesize /
sectorsize.

> 
> Essentially the page would look like:
> 
> |0|1|2|3|4|5|6|7|8|9|10|11|12|13|14|15
> 
> So you want to release the EB's that spawn 0-3, 4-7, 8-11, 12-15, but
> what if bit_start becomes 1 and you add 4 to that, this offsets all
> further calculation by 1 i.e you are going into the next eb.

Nope, 4 is only added we we hit a bit set.
If we hit a bit zero, we jump to next bit, not following nodesize >>
sectorsize.

That's exactly the reason I'm not using for() loop here, due to the
difference in step size.

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();
>> +		ASSERT(eb);
>> +		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;
>> +		}
>> +		/*
>> +		 * Here we don't care the return value, we will always check
>> +		 * the page private at the end.
>> +		 * And release_extent_buffer() will release the refs_lock.
>> +		 */
>> +		release_extent_buffer(eb);
>> +	}
>> +	/* Finally to check if we have cleared page private */
>> +	spin_lock(&page->mapping->private_lock);
>> +	if (!PagePrivate(page))
>> +		ret = 1;
>> +	else
>> +		ret = 0;
>> +	spin_unlock(&page->mapping->private_lock);
>> +	return ret;
>> +
>> +}
>> +
>>  int try_release_extent_buffer(struct page *page)
>>  {
>>  	struct extent_buffer *eb;
>>  
>> +	if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
>> +		return try_release_subpage_extent_buffer(page);
>> +
>>  	/*
>>  	 * We need to make sure nobody is attaching this page to an eb right
>>  	 * now.
>>
>