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

[Nikolay Borisov <nborisov@suse.com>]

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.

> +		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 ?

> +		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?  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 ?

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.


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