Re: [PATCH 09/17] btrfs: refactor btrfs_release_extent_buffer_pages()

[Nikolay Borisov <nborisov@suse.com>]

On 8.09.20 г. 10:52 ч., Qu Wenruo wrote:
> We have attach_extent_buffer_page() and it get utilized in
> btrfs_clone_extent_buffer() and alloc_extent_buffer().
> 
> But in btrfs_release_extent_buffer_pages() we manually call
> detach_page_private().
> 
> This is fine for current code, but if we're going to support subpage
> size, we will do a lot of more work other than just calling
> detach_page_private().
> 
> This patch will extract the main work of btrfs_clone_extent_buffer()

Did you mean to type btrfs_release_extent_buffer_pages instead of
clone_extent_buffer ?

> into detach_extent_buffer_page() so that later subpage size support can
> put their own code into them.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent_io.c | 58 +++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 3c8fe40f67fa..1cb41dab7a1d 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4920,6 +4920,29 @@ int extent_buffer_under_io(const struct extent_buffer *eb)
>  		test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags));
>  }
>  
> +static void detach_extent_buffer_page(struct extent_buffer *eb,
> +				      struct page *page)
> +{
> +	bool mapped = !test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);

nit: Now you are performing the atomic op once per page rather than once
per-eb.

> +
> +	if (!page)
> +		return;
> +
> +	if (mapped)
> +		spin_lock(&page->mapping->private_lock);
> +	if (PagePrivate(page) && page->private == (unsigned long)eb) {
> +		BUG_ON(test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags));
> +		BUG_ON(PageDirty(page));
> +		BUG_ON(PageWriteback(page));
> +		/* We need to make sure we haven't be attached to a new eb. */
> +		detach_page_private(page);
> +	}
> +	if (mapped)
> +		spin_unlock(&page->mapping->private_lock);
> +	/* One for when we allocated the page */
> +	put_page(page);
> +}
> +
>  /*
>   * Release all pages attached to the extent buffer.
>   */
> @@ -4927,43 +4950,12 @@ static void btrfs_release_extent_buffer_pages(struct extent_buffer *eb)
>  {
>  	int i;
>  	int num_pages;
> -	int mapped = !test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
>  
>  	BUG_ON(extent_buffer_under_io(eb));
>  
>  	num_pages = num_extent_pages(eb);
> -	for (i = 0; i < num_pages; i++) {
> -		struct page *page = eb->pages[i];
> -
> -		if (!page)
> -			continue;
> -		if (mapped)
> -			spin_lock(&page->mapping->private_lock);
> -		/*
> -		 * We do this since we'll remove the pages after we've
> -		 * removed the eb from the radix tree, so we could race
> -		 * and have this page now attached to the new eb.  So
> -		 * only clear page_private if it's still connected to
> -		 * this eb.
> -		 */
> -		if (PagePrivate(page) &&
> -		    page->private == (unsigned long)eb) {
> -			BUG_ON(test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags));
> -			BUG_ON(PageDirty(page));
> -			BUG_ON(PageWriteback(page));
> -			/*
> -			 * We need to make sure we haven't be attached
> -			 * to a new eb.
> -			 */
> -			detach_page_private(page);
> -		}
> -
> -		if (mapped)
> -			spin_unlock(&page->mapping->private_lock);
> -
> -		/* One for when we allocated the page */
> -		put_page(page);
> -	}
> +	for (i = 0; i < num_pages; i++)
> +		detach_extent_buffer_page(eb, eb->pages[i]);
>  }
>  
>  /*
>