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

[Josef Bacik <josef@toxicpanda.com>]

On 1/16/21 2:15 AM, Qu Wenruo wrote:
> Unlike the original try_release_extent_buffer(),
> try_release_subpage_extent_buffer() will iterate through all the ebs in
> the page, and try to release each eb.
> 
> And only if the page and no private attached, which implies we have
> released all ebs of the page, then we can release the full page.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/extent_io.c | 106 ++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 104 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 74a37eec921f..9414219fa28b 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -6335,13 +6335,115 @@ void memmove_extent_buffer(const struct extent_buffer *dst,
>   	}
>   }
>   
> +static struct extent_buffer *get_next_extent_buffer(
> +		struct btrfs_fs_info *fs_info, struct page *page, u64 bytenr)
> +{
> +	struct extent_buffer *gang[BTRFS_SUBPAGE_BITMAP_SIZE];
> +	struct extent_buffer *found = NULL;
> +	u64 page_start = page_offset(page);
> +	int ret;
> +	int i;
> +
> +	ASSERT(in_range(bytenr, page_start, PAGE_SIZE));
> +	ASSERT(PAGE_SIZE / fs_info->nodesize <= BTRFS_SUBPAGE_BITMAP_SIZE);
> +	lockdep_assert_held(&fs_info->buffer_lock);
> +
> +	ret = radix_tree_gang_lookup(&fs_info->buffer_radix, (void **)gang,
> +			bytenr >> fs_info->sectorsize_bits,
> +			PAGE_SIZE / fs_info->nodesize);
> +	for (i = 0; i < ret; i++) {
> +		/* Already beyond page end */
> +		if (gang[i]->start >= page_start + PAGE_SIZE)
> +			break;
> +		/* Found one */
> +		if (gang[i]->start >= bytenr) {
> +			found = gang[i];
> +			break;
> +		}
> +	}
> +	return found;
> +}
> +
> +static int try_release_subpage_extent_buffer(struct page *page)
> +{
> +	struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> +	u64 cur = page_offset(page);
> +	const u64 end = page_offset(page) + PAGE_SIZE;
> +	int ret;
> +
> +	while (cur < end) {
> +		struct extent_buffer *eb = NULL;
> +
> +		/*
> +		 * Unlike try_release_extent_buffer() which uses page->private
> +		 * to grab buffer, for subpage case we rely on radix tree, thus
> +		 * we need to ensure radix tree consistency.
> +		 *
> +		 * We also want an atomic snapshot of the radix tree, thus go
> +		 * spinlock other than RCU.
> +		 */
> +		spin_lock(&fs_info->buffer_lock);
> +		eb = get_next_extent_buffer(fs_info, page, cur);
> +		if (!eb) {
> +			/* No more eb in the page range after or at @cur */
> +			spin_unlock(&fs_info->buffer_lock);
> +			break;
> +		}
> +		cur = eb->start + eb->len;
> +
> +		/*
> +		 * The same as try_release_extent_buffer(), to ensure the eb
> +		 * won't disappear out from under us.
> +		 */
> +		spin_lock(&eb->refs_lock);
> +		if (atomic_read(&eb->refs) != 1 || extent_buffer_under_io(eb)) {
> +			spin_unlock(&eb->refs_lock);
> +			spin_unlock(&fs_info->buffer_lock);

Why continue at this point?  We know we can't drop this thing, break here.

<snip>

> +}
> +
>   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);

You're using sectorsize again here.  I realize the problem is sectorsize != 
PAGE_SIZE, but sectorsize != nodesize all the time, so please change all of the 
patches to check the actual relevant size for the data/metadata type.  Thanks,

Josef