Re: [PATCH 19/32] btrfs: handle sectorsize < PAGE_SIZE case for extent buffer accessors

[Nikolay Borisov <nborisov@suse.com>]

On 3.11.20 г. 15:30 ч., Qu Wenruo wrote:
> To support sectorsize < PAGE_SIZE case, we need to take extra care for
> extent buffer accessors.
> 
> Since sectorsize is smaller than PAGE_SIZE, one page can contain
> multiple tree blocks, we must use eb->start to determine the real offset
> to read/write for extent buffer accessors.
> 
> This patch introduces two helpers to do these:
> - get_eb_page_index()
>   This is to calculate the index to access extent_buffer::pages.
>   It's just a simple wrapper around "start >> PAGE_SHIFT".
> 
>   For sectorsize == PAGE_SIZE case, nothing is changed.
>   For sectorsize < PAGE_SIZE case, we always get index as 0, and
>   the existing page shift works also fine.
> 
> - get_eb_page_offset()
>   This is to calculate the offset to access extent_buffer::pages.

nit: This is the same sentence as for get_eb_page_index, I think you
mean this calculates the offset in the page to start reading from.

>   This needs to take extent_buffer::start into consideration.
> 
>   For sectorsize == PAGE_SIZE case, extent_buffer::start is always
>   aligned to PAGE_SIZE, thus adding extent_buffer::start to
>   offset_in_page() won't change the result.
>   For sectorsize < PAGE_SIZE case, adding extent_buffer::start gives
>   us the correct offset to access.
> 
> This patch will touch the following parts to cover all extent buffer
> accessors:
> 
> - BTRFS_SETGET_HEADER_FUNCS()
> - read_extent_buffer()
> - read_extent_buffer_to_user()
> - memcmp_extent_buffer()
> - write_extent_buffer_chunk_tree_uuid()
> - write_extent_buffer_fsid()
> - write_extent_buffer()
> - memzero_extent_buffer()
> - copy_extent_buffer_full()
> - copy_extent_buffer()
> - memcpy_extent_buffer()
> - memmove_extent_buffer()
> - btrfs_get_token_##bits()
> - btrfs_get_##bits()
> - btrfs_set_token_##bits()
> - btrfs_set_##bits()
> - generic_bin_search()
> 

<snip>

> @@ -3314,6 +3315,39 @@ static inline void assertfail(const char *expr, const char* file, int line) { }
>  #define ASSERT(expr)	(void)(expr)
>  #endif
>  
> +/*
> + * Get the correct offset inside the page of extent buffer.
> + *
> + * Will handle both sectorsize == PAGE_SIZE and sectorsize < PAGE_SIZE cases.
> + *
> + * @eb:		The target extent buffer
> + * @start:	The offset inside the extent buffer
> + */
> +static inline size_t get_eb_page_offset(const struct extent_buffer *eb,
> +					unsigned long offset_in_eb)

nit: Rename to offset, you already pass an extent buffer so it's natural
that the offset pertain to this eb.

> +{
> +	/*
> +	 * For sectorsize == PAGE_SIZE case, eb->start will always be aligned
> +	 * to PAGE_SIZE, thus adding it won't cause any difference.
> +	 *
> +	 * For sectorsize < PAGE_SIZE, we must only read the data belongs to
> +	 * the eb, thus we have to take the eb->start into consideration.
> +	 */
> +	return offset_in_page(offset_in_eb + eb->start);
> +}
> +
> +static inline unsigned long get_eb_page_index(unsigned long offset_in_eb)

nit: Rename to offset since "in_eb" doesn't bring any value just makes
the variable's name somewhat awkward.
> +{
> +	/*
> +	 * For sectorsize == PAGE_SIZE case, plain >> PAGE_SHIFT is enough.
> +	 *
> +	 * For sectorsize < PAGE_SIZE case, we only support 64K PAGE_SIZE,
> +	 * and has ensured all tree blocks are contained in one page, thus
> +	 * we always get index == 0.
> +	 */
> +	return offset_in_eb >> PAGE_SHIFT;
> +}
> +
>  /*
>   * Use that for functions that are conditionally exported for sanity tests but
>   * otherwise static

<snip>

> @@ -5873,10 +5873,22 @@ void copy_extent_buffer_full(const struct extent_buffer *dst,
>  
>  	ASSERT(dst->len == src->len);
>  
> -	num_pages = num_extent_pages(dst);
> -	for (i = 0; i < num_pages; i++)
> -		copy_page(page_address(dst->pages[i]),
> -				page_address(src->pages[i]));
> +	if (dst->fs_info->sectorsize == PAGE_SIZE) {
> +		num_pages = num_extent_pages(dst);
> +		for (i = 0; i < num_pages; i++)
> +			copy_page(page_address(dst->pages[i]),
> +				  page_address(src->pages[i]));
> +	} else {
> +		unsigned long src_index = get_eb_page_index(0);
> +		unsigned long dst_index = get_eb_page_index(0);

nit: unsigned long src_index = 0, dst_index = 0; and remove the ASSERT()
below

> +		size_t src_offset = get_eb_page_offset(src, 0);
> +		size_t dst_offset = get_eb_page_offset(dst, 0);
> +
> +		ASSERT(src_index == 0 && dst_index == 0);
> +		memcpy(page_address(dst->pages[dst_index]) + dst_offset,
> +		       page_address(src->pages[src_index]) + src_offset,
> +		       src->len);
> +	}
>  }

<snip>