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

Omar Sandoval
2022-03-30T20:42:14Z

On Wed, Mar 30, 2022 at 02:48:42PM -0400, Sweet Tea Dorminy wrote:
>
>
> 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?

Yeah, vmalloc()+vmalloc_to_page() is going to be more or less equivalent
to the vmap thing I'm doing here, but a lot cleaner. Replacing this
patch with the below patch seems to work:

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index c0ca45dae6d6..e574d4f4a167 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -87,6 +87,7 @@ struct send_ctx {
* command (since protocol v2, data must be the last attribute).
*/
bool put_data;
+ struct page **send_buf_pages;
u64 flags; /* 'flags' member of btrfs_ioctl_send_args is u64 */
/* Protocol version compatibility requested */
u32 proto;
@@ -7486,12 +7487,32 @@ long btrfs_ioctl_send(struct inode *inode, struct btrfs_ioctl_send_args *arg)
sctx->clone_roots_cnt = arg->clone_sources_count;

if (sctx->proto >= 2) {
+ u32 send_buf_num_pages;
+
sctx->send_max_size = ALIGN(SZ_16K + BTRFS_MAX_COMPRESSED,
PAGE_SIZE);
+ sctx->send_buf = vmalloc(sctx->send_max_size);
+ if (!sctx->send_buf) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ send_buf_num_pages = sctx->send_max_size >> PAGE_SHIFT;
+ sctx->send_buf_pages = kcalloc(send_buf_num_pages,
+ sizeof(*sctx->send_buf_pages),
+ GFP_KERNEL);
+ if (!sctx->send_buf_pages) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ for (i = 0; i < send_buf_num_pages; i++) {
+ sctx->send_buf_pages[i] =
+ vmalloc_to_page(sctx->send_buf +
+ (i << PAGE_SHIFT));
+ }
} else {
sctx->send_max_size = BTRFS_SEND_BUF_SIZE_V1;
+ sctx->send_buf = kvmalloc(sctx->send_max_size, GFP_KERNEL);
}
- sctx->send_buf = kvmalloc(sctx->send_max_size, GFP_KERNEL);
if (!sctx->send_buf) {
ret = -ENOMEM;
goto out;
@@ -7684,6 +7705,7 @@ long btrfs_ioctl_send(struct inode *inode, struct btrfs_ioctl_send_args *arg)
fput(sctx->send_filp);

kvfree(sctx->clone_roots);
+ kfree(sctx->send_buf_pages);
kvfree(sctx->send_buf);

name_cache_free(sctx);