Re: [PATCH 1/2] btrfs: Factor out allocating an array of pages.

Filipe Manana
2022-03-29T10:04:11Z

On Mon, Mar 28, 2022 at 04:14:27PM -0400, Sweet Tea Dorminy wrote:
> Several functions currently populate an array of page pointers one
> allocated page at a time; factor out the common code so as to allow
> improvements to all of the sites at once.
>
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
> fs/btrfs/check-integrity.c | 8 +++-----
> fs/btrfs/compression.c | 37 +++++++++++++++--------------------
> fs/btrfs/ctree.c | 25 ++++++++++++++++++++++++
> fs/btrfs/ctree.h | 2 ++
> fs/btrfs/extent_io.c | 40 +++++++++++++++++++++++---------------
> fs/btrfs/inode.c | 10 ++++------
> fs/btrfs/raid56.c | 30 ++++------------------------
> 7 files changed, 78 insertions(+), 74 deletions(-)
>
> diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
> index 7e9f90fa0388..366d5a80f3c5 100644
> --- a/fs/btrfs/check-integrity.c
> +++ b/fs/btrfs/check-integrity.c
> @@ -1553,11 +1553,9 @@ static int btrfsic_read_block(struct btrfsic_state *state,
> return -ENOMEM;
> block_ctx->datav = block_ctx->mem_to_free;
> block_ctx->pagev = (struct page **)(block_ctx->datav + num_pages);
> - for (i = 0; i < num_pages; i++) {
> - block_ctx->pagev[i] = alloc_page(GFP_NOFS);
> - if (!block_ctx->pagev[i])
> - return -1;
> - }
> + ret = btrfs_alloc_page_array(num_pages, block_ctx->pagev);
> + if (ret)
> + return ret;
>
> dev_bytenr = block_ctx->dev_bytenr;
> for (i = 0; i < num_pages;) {
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index be476f094300..0fc663b757fb 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -801,8 +801,6 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
> struct extent_map_tree *em_tree;
> struct compressed_bio *cb;
> unsigned int compressed_len;
> - unsigned int nr_pages;
> - unsigned int pg_index;
> struct bio *comp_bio = NULL;
> const u64 disk_bytenr = bio->bi_iter.bi_sector << SECTOR_SHIFT;
> u64 cur_disk_byte = disk_bytenr;
> @@ -812,7 +810,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
> u64 em_start;
> struct extent_map *em;
> blk_status_t ret;
> - int faili = 0;
> + int r;
> + int i;
> u8 *sums;
>
> em_tree = &BTRFS_I(inode)->extent_tree;
> @@ -855,25 +854,20 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
> cb->compress_type = extent_compress_type(bio_flags);
> cb->orig_bio = bio;
>
> - nr_pages = DIV_ROUND_UP(compressed_len, PAGE_SIZE);
> - cb->compressed_pages = kcalloc(nr_pages, sizeof(struct page *),
> + cb->nr_pages = DIV_ROUND_UP(compressed_len, PAGE_SIZE);
> + cb->compressed_pages = kcalloc(cb->nr_pages, sizeof(struct page *),
> GFP_NOFS);
> if (!cb->compressed_pages) {
> ret = BLK_STS_RESOURCE;
> - goto fail1;
> + goto fail;
> }
>
> - for (pg_index = 0; pg_index < nr_pages; pg_index++) {
> - cb->compressed_pages[pg_index] = alloc_page(GFP_NOFS);
> - if (!cb->compressed_pages[pg_index]) {
> - faili = pg_index - 1;
> - ret = BLK_STS_RESOURCE;
> - goto fail2;
> - }
> + r = btrfs_alloc_page_array(cb->nr_pages, cb->compressed_pages);
> + if (r) {
> + ret = BLK_STS_RESOURCE;
> + goto fail;
> }
> - faili = nr_pages - 1;
> - cb->nr_pages = nr_pages;
> -
> +
> add_ra_bio_pages(inode, em_start + em_len, cb);
>
> /* include any pages we added in add_ra-bio_pages */
> @@ -949,14 +943,15 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
> }
> return BLK_STS_OK;
>
> -fail2:
> - while (faili >= 0) {
> - __free_page(cb->compressed_pages[faili]);
> - faili--;
> +fail:
> + if (cb->compressed_pages) {
> + for (i = 0; i < cb->nr_pages; i++) {
> + if (cb->compressed_pages[i])
> + __free_page(cb->compressed_pages[i]);
> + }
> }
>
> kfree(cb->compressed_pages);
> -fail1:
> kfree(cb);
> out:
> free_extent_map(em);
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 1e24695ede0a..4e81e75c8e7c 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -90,6 +90,31 @@ void btrfs_free_path(struct btrfs_path *p)
> kmem_cache_free(btrfs_path_cachep, p);
> }
>
> +/**
> + * btrfs_alloc_page_array() - allocate an array of pages.
> + *
> + * @nr_pages: the number of pages to request
> + * @page_array: the array to fill with pages. Any existing non-null entries in
> + * the array will be skipped.
> + *
> + * Return: 0 if all pages were able to be allocated; -ENOMEM otherwise, and the
> + * caller is responsible for freeing all non-null page pointers in the array.
> + */
> +int btrfs_alloc_page_array(unsigned long nr_pages, struct page **page_array)
> +{
> + int i;
> + for (i = 0; i < nr_pages; i++) {
> + struct page *page;
> + if (page_array[i])
> + continue;
> + page = alloc_page(GFP_NOFS);
> + if (!page)
> + return -ENOMEM;
> + page_array[i] = page;
> + }
> + return 0;
> +}

Adding this helper to ctree.c is odd, as this is a module that implements
a btree and exports functions related to btree operations and btree nodes/leaves.

All the use cases for this helper relate to IO operations, so extent_io.c
is perhaps a better fit.

Thanks.

> +
> /*
> * path release drops references on the extent buffers in the path
> * and it drops any locks held by this path
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 7328fb17b7f5..e835a2bfb60a 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2969,6 +2969,8 @@ void btrfs_release_path(struct btrfs_path *p);
> struct btrfs_path *btrfs_alloc_path(void);
> void btrfs_free_path(struct btrfs_path *p);
>
> +int btrfs_alloc_page_array(unsigned long nr_pages, struct page **page_array);
> +
> int btrfs_del_items(struct btrfs_trans_handle *trans, struct btrfs_root *root,
> struct btrfs_path *path, int slot, int nr);
> static inline int btrfs_del_item(struct btrfs_trans_handle *trans,
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 53b59944013f..c1c8d770f43a 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5898,9 +5898,9 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
> struct extent_buffer *btrfs_clone_extent_buffer(const struct extent_buffer *src)
> {
> int i;
> - struct page *p;
> struct extent_buffer *new;
> int num_pages = num_extent_pages(src);
> + int r;
>
> new = __alloc_extent_buffer(src->fs_info, src->start, src->len);
> if (new == NULL)
> @@ -5913,22 +5913,23 @@ struct extent_buffer *btrfs_clone_extent_buffer(const struct extent_buffer *src)
> */
> set_bit(EXTENT_BUFFER_UNMAPPED, &new->bflags);
>
> + memset(new->pages, 0, sizeof(*new->pages) * num_pages);
> + r = btrfs_alloc_page_array(num_pages, new->pages);
> + if (r) {
> + btrfs_release_extent_buffer(new);
> + return NULL;
> + }
> +
> for (i = 0; i < num_pages; i++) {
> int ret;
> + struct page *p = new->pages[i];
>
> - p = alloc_page(GFP_NOFS);
> - if (!p) {
> - btrfs_release_extent_buffer(new);
> - return NULL;
> - }
> ret = attach_extent_buffer_page(new, p, NULL);
> if (ret < 0) {
> - put_page(p);
> btrfs_release_extent_buffer(new);
> return NULL;
> }
> WARN_ON(PageDirty(p));
> - new->pages[i] = p;
> copy_page(page_address(p), page_address(src->pages[i]));
> }
> set_extent_buffer_uptodate(new);
> @@ -5942,31 +5943,38 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
> struct extent_buffer *eb;
> int num_pages;
> int i;
> + int r;
>
> eb = __alloc_extent_buffer(fs_info, start, len);
> if (!eb)
> return NULL;
>
> num_pages = num_extent_pages(eb);
> + r = btrfs_alloc_page_array(num_pages, eb->pages);
> + if (r)
> + goto err;
> +
> for (i = 0; i < num_pages; i++) {
> int ret;
> + struct page *p = eb->pages[i];
>
> - eb->pages[i] = alloc_page(GFP_NOFS);
> - if (!eb->pages[i])
> - goto err;
> - ret = attach_extent_buffer_page(eb, eb->pages[i], NULL);
> - if (ret < 0)
> + ret = attach_extent_buffer_page(eb, p, NULL);
> + if (ret < 0) {
> goto err;
> + }
> }
> +
> set_extent_buffer_uptodate(eb);
> btrfs_set_header_nritems(eb, 0);
> set_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
>
> return eb;
> err:
> - for (; i > 0; i--) {
> - detach_extent_buffer_page(eb, eb->pages[i - 1]);
> - __free_page(eb->pages[i - 1]);
> + for (i = 0; i < num_pages; i++) {
> + if (eb->pages[i]) {
> + detach_extent_buffer_page(eb, eb->pages[i]);
> + __free_page(eb->pages[i]);
> + }
> }
> __free_extent_buffer(eb);
> return NULL;
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c7b15634fe70..121858652a09 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -10427,13 +10427,11 @@ static ssize_t btrfs_encoded_read_regular(struct kiocb *iocb,
> pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS);
> if (!pages)
> return -ENOMEM;
> - for (i = 0; i < nr_pages; i++) {
> - pages[i] = alloc_page(GFP_NOFS);
> - if (!pages[i]) {
> - ret = -ENOMEM;
> - goto out;
> + ret = btrfs_alloc_page_array(nr_pages, pages);
> + if (ret) {
> + ret = -ENOMEM;
> + goto out;
> }
> - }
>
> ret = btrfs_encoded_read_regular_fill_pages(inode, start, disk_bytenr,
> disk_io_size, pages);
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 0e239a4c3b26..ea7a9152b1cc 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1026,37 +1026,15 @@ static struct btrfs_raid_bio *alloc_rbio(struct btrfs_fs_info *fs_info,
> /* allocate pages for all the stripes in the bio, including parity */
> static int alloc_rbio_pages(struct btrfs_raid_bio *rbio)
> {
> - int i;
> - struct page *page;
> -
> - for (i = 0; i < rbio->nr_pages; i++) {
> - if (rbio->stripe_pages[i])
> - continue;
> - page = alloc_page(GFP_NOFS);
> - if (!page)
> - return -ENOMEM;
> - rbio->stripe_pages[i] = page;
> - }
> - return 0;
> + return btrfs_alloc_page_array(rbio->nr_pages, rbio->stripe_pages);
> }
>
> /* only allocate pages for p/q stripes */
> static int alloc_rbio_parity_pages(struct btrfs_raid_bio *rbio)
> {
> - int i;
> - struct page *page;
> -
> - i = rbio_stripe_page_index(rbio, rbio->nr_data, 0);
> -
> - for (; i < rbio->nr_pages; i++) {
> - if (rbio->stripe_pages[i])
> - continue;
> - page = alloc_page(GFP_NOFS);
> - if (!page)
> - return -ENOMEM;
> - rbio->stripe_pages[i] = page;
> - }
> - return 0;
> + int data_pages = rbio_stripe_page_index(rbio, rbio->nr_data, 0);
> + return btrfs_alloc_page_array(rbio->nr_pages - data_pages,
> + rbio->stripe_pages + data_pages);
> }
>
> /*
> --
> 2.35.1
>