Re: [PATCH v2 07/18] btrfs: extent_io: make grab_extent_buffer_from_page() to handle subpage case

[Nikolay Borisov <nborisov@suse.com>]

On 10.12.20 г. 8:38 ч., Qu Wenruo wrote:
> For subpage case, grab_extent_buffer_from_page() can't really get an
> extent buffer just from btrfs_subpage.
> 
> Although we have btrfs_subpage::tree_block_bitmap, which can be used to
> grab the bytenr of an existing extent buffer, and can then go radix tree
> search to grab that existing eb.
> 
> However we are still doing radix tree insert check in
> alloc_extent_buffer(), thus we don't really need to do the extra hassle,
> just let alloc_extent_buffer() to handle existing eb in radix tree.
> 
> So for grab_extent_buffer_from_page(), just always return NULL for
> subpage case.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent_io.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 51dd7ec3c2b3..b99bd0402130 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5278,10 +5278,19 @@ 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)
> +static struct extent_buffer *grab_extent_buffer_from_page(
> +		struct btrfs_fs_info *fs_info, struct page *page)
>  {
>  	struct extent_buffer *exists;
>  
> +	/*
> +	 * For subpage case, we completely rely on radix tree to ensure we
> +	 * don't try to insert two eb for the same bytenr.
> +	 * So here we alwasy return NULL and just continue.
> +	 */
> +	if (fs_info->sectorsize < PAGE_SIZE)
> +		return NULL;
> +

Instead of hiding this in the function, just open-code it in the only caller. It would look like: 

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index b99bd0402130..440dab207944 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5370,8 +5370,9 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
                }
 
                spin_lock(&mapping->private_lock);
-               exists = grab_extent_buffer_from_page(fs_info, p);
-               if (exists) {
+               if (fs_info->sectorsize == PAGE_SIZE &&
+                   (exists = grab_extent_buffer_from_page(fs_info, p)));
+               {
                        spin_unlock(&mapping->private_lock);
                        unlock_page(p);
                        put_page(p);


Admittedly that exist = ... in the if condition is a bit of an anti-pattern but given it's used in only 1 place
and makes the flow of code more linear I'd say it's a win. But would like to hear David's opinion. 

>  	/* Page not yet attached to an extent buffer */
>  	if (!PagePrivate(page))
>  		return NULL;
> @@ -5361,7 +5370,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>  		}
>  
>  		spin_lock(&mapping->private_lock);
> -		exists = grab_extent_buffer_from_page(p);
> +		exists = grab_extent_buffer_from_page(fs_info, p);
>  		if (exists) {
>  			spin_unlock(&mapping->private_lock);
>  			unlock_page(p);
>