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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2020/11/6 下午8:51, Nikolay Borisov wrote:
>
>
> 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.

Sorry, the for index, it's "to calculate the *index* to access", while
for this it's "to calculate the *offset* to access".

>
>>   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.

I intended to reduce the confusion to allow caller to know what to pass in.

But you're right, the "offset_in_eb" doesn't really bring anything.

Will rename them.

Thanks,
Qu
>
>> +{
>> +	/*
>> +	 * 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>
>