Re: [PATCH v2 04/18] btrfs: extent_io: introduce a helper to grab an existing extent buffer from a page

[Nikolay Borisov <nborisov@suse.com>]

On 10.12.20 г. 8:38 ч., Qu Wenruo wrote:
> This patch will extract the code to grab an extent buffer from a page
> into a helper, grab_extent_buffer_from_page().
> 
> This reduces one indent level, and provides the work place for later
> expansion for subapge support.
> 
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent_io.c | 52 +++++++++++++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 612fe60b367e..6350c2687c7e 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5251,6 +5251,32 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
>  }
>  #endif
>  
> +static struct extent_buffer *grab_extent_buffer_from_page(struct page *page)

nit: Make the name just grab_extent_buffer/get_extent_buffer, given you
pass in a page as an input parameter "from_page" is obvious.

> +{
> +	struct extent_buffer *exists;
> +
> +	/* Page not yet attached to an extent buffer */
> +	if (!PagePrivate(page))
> +		return NULL;
> +
> +	/*
> +	 * We could have already allocated an eb for this page
> +	 * and attached one so lets see if we can get a ref on
> +	 * the existing eb, and if we can we know it's good and
> +	 * we can just return that one, else we know we can just
> +	 * overwrite page->private.
> +	 */
> +	exists = (struct extent_buffer *)page->private;
> +	if (atomic_inc_not_zero(&exists->refs)) {
> +		mark_extent_buffer_accessed(exists, page);
> +		return exists;
> +	}

nit: This patch slightly changes the timing of
mark_extent_buffer_accessed, as it's now called under
mapping->private_lock and respective page locked. Looking at
map_extent_buffer_accessed it does iterate pages and call
mark_page_accessed on them as well as calling check_buffer_tre_ref which
does some atomic ops. While it might not be a big hit I'd expect there
will be some minimal performance regression.

> +
> +	WARN_ON(PageDirty(page));
> +	detach_page_private(page);
> +	return NULL;
> +}
> +
>  struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>  					  u64 start, u64 owner_root, int level)
>  {
> @@ -5296,26 +5322,12 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>  		}
>  
>  		spin_lock(&mapping->private_lock);
> -		if (PagePrivate(p)) {
> -			/*
> -			 * We could have already allocated an eb for this page
> -			 * and attached one so lets see if we can get a ref on
> -			 * the existing eb, and if we can we know it's good and
> -			 * we can just return that one, else we know we can just
> -			 * overwrite page->private.
> -			 */
> -			exists = (struct extent_buffer *)p->private;
> -			if (atomic_inc_not_zero(&exists->refs)) {
> -				spin_unlock(&mapping->private_lock);
> -				unlock_page(p);
> -				put_page(p);
> -				mark_extent_buffer_accessed(exists, p);
> -				goto free_eb;
> -			}
> -			exists = NULL;
> -
> -			WARN_ON(PageDirty(p));
> -			detach_page_private(p);
> +		exists = grab_extent_buffer_from_page(p);
> +		if (exists) {
> +			spin_unlock(&mapping->private_lock);
> +			unlock_page(p);
> +			put_page(p);
> +			goto free_eb;
>  		}
>  		attach_extent_buffer_page(eb, p);
>  		spin_unlock(&mapping->private_lock);
>