Re: [PATCH v14 5/7] btrfs: send: allocate send buffer with alloc_page() and vmap() for v2

Sweet Tea Dorminy
2022-03-30T18:48:52Z

On 3/30/22 13:13, Omar Sandoval wrote:
> On Wed, Mar 30, 2022 at 12:33:48PM -0400, Sweet Tea Dorminy wrote:
>>
>>
>> On 3/30/22 12:03, Omar Sandoval wrote:
>>> On Thu, Mar 24, 2022 at 01:53:20PM -0400, Sweet Tea Dorminy wrote:
>>>>
>>>>
>>>> On 3/17/22 13:25, Omar Sandoval wrote:
>>>>> From: Omar Sandoval <osandov@fb.com>
>>>>>
>>>>> For encoded writes, we need the raw pages for reading compressed data
>>>>> directly via a bio.
>>>> Perhaps:
>>>> "For encoded writes, the existing btrfs_encoded_read*() functions expect a
>>>> list of raw pages."
>>>>
>>>> I think it would be a better to continue just vmalloc'ing a large continuous
>>>> buffer and translating each page in the buffer into its raw page with
>>>> something like is_vmalloc_addr(data) ? vmalloc_to_page(data) :
>>>> virt_to_page(data). Vmalloc can request a higher-order allocation, which
>>>> probably doesn't matter but might slightly improve memory locality. And in
>>>> terms of readability, I somewhat like the elegance of having a single
>>>> identical kvmalloc call to allocate and send_buf in both cases, even if we
>>>> do need to initialize the page list for some v2 commands.
>>>
>>> I like this, but are we guaranteed that kvmalloc() will return a
>>> page-aligned buffer? It seems reasonable to me that it would for
>>> allocations of at least one page, but I can't find that written down
>>> anywhere.
>>
>> Since vmalloc allocates whole pages, and kmalloc guarantees alignment to the
>> allocation size for powers of 2 sizes (and PAGE_SIZE is required to be a
>> power of 2), I think that adds up to a guarantee of page alignment both
>> ways?
>>
>> https://elixir.bootlin.com/linux/v5.17.1/source/include/linux/slab.h#L522 :
>> kmalloc: "For @size of power of two bytes, the alignment is also guaranteed
>> to be at least to the size."
>
> Our allocation size is ALIGN(SZ_16K + BTRFS_MAX_COMPRESSED, PAGE_SIZE),
> which is 144K for PAGE_SIZE = 4k. If I interpret the kmalloc() comment
> very literally, since this isn't a power of two, it's not guaranteed to
> be aligned, right?

Ah, an excellent point.

Now that I think about it, the kmalloc path picks a slab to allocate
from based on the log_2 of the size:
https://elixir.bootlin.com/linux/v5.17.1/source/mm/slab_common.c#L733 so
we'd end up wasting 128k-16k space using kmalloc, whether it's aligned
or not, I think?

So maybe it should just always use vmalloc and get the page alignment?