Messages in this thread | | | Date | Thu, 31 Mar 2022 14:19:07 -0400 | Subject | Re: [PATCH v3 2/2] btrfs: allocate page arrays using bulk page allocator | From | Sweet Tea Dorminy <> |
| |
On 3/31/22 13:35, David Sterba wrote: > On Wed, Mar 30, 2022 at 04:11:23PM -0400, Sweet Tea Dorminy wrote: >> While calling alloc_page() in a loop is an effective way to populate an >> array of pages, the kernel provides a method to allocate pages in bulk. >> alloc_pages_bulk_array() populates the NULL slots in a page array, trying to >> grab more than one page at a time. >> >> Unfortunately, it doesn't guarantee allocating all slots in the array, >> but it's easy to call it in a loop and return an error if no progress >> occurs. Similar code can be found in xfs/xfs_buf.c:xfs_buf_alloc_pages(). >> >> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me> >> Reviewed-by: Nikolay Borisov <nborisov@suse.com> >> --- >> Changes in v3: >> - Added a newline after variable declaration >> Changes in v2: >> - Moved from ctree.c to extent_io.c >> --- >> fs/btrfs/extent_io.c | 24 +++++++++++++++--------- >> 1 file changed, 15 insertions(+), 9 deletions(-) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index ab4c1c4d1b59..b268e47aa2b7 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -3144,19 +3144,25 @@ static void end_bio_extent_readpage(struct bio *bio) >> */ >> int btrfs_alloc_page_array(unsigned long nr_pages, struct page **page_array) >> { >> - int i; >> + long allocated = 0; >> + >> + for (;;) { >> + long last = allocated; >> >> - for (i = 0; i < nr_pages; i++) { >> - struct page *page; >> + allocated = alloc_pages_bulk_array(GFP_NOFS, nr_pages, >> + page_array); >> + if (allocated == nr_pages) >> + return 0; >> >> - if (page_array[i]) >> + if (allocated != last) >> continue; >> - page = alloc_page(GFP_NOFS); >> - if (!page) >> - return -ENOMEM; >> - page_array[i] = page; >> + /* >> + * During this iteration, no page could be allocated, even >> + * though alloc_pages_bulk_array() falls back to alloc_page() >> + * if it could not bulk-allocate. So we must be out of memory. >> + */ >> + return -ENOMEM; >> } > > I find the way the loop is structured a bit cumbersome so I'd suggest to > rewrite it as: > > int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) > { > unsigned int allocated; > > for (allocated = 0; allocated < nr_pages;) { > unsigned int last = allocated; > > allocated = alloc_pages_bulk_array(GFP_NOFS, nr_pages, page_array); > > /* > * During this iteration, no page could be allocated, even > * though alloc_pages_bulk_array() falls back to alloc_page() > * if it could not bulk-allocate. So we must be out of memory. > */ > if (allocated == last) > return -ENOMEM; > } > return 0; > } Sounds good, I'll amend it that way.
> > Also in the xfs code there's memalloc_retry_wait() which is supposed to be > called when repeated memory allocation is retried. What was the reason > you removed it?
Trying to keep the behavior as close as possible to the existing behavior.
The current behavior of each alloc_page loop is to fail if alloc_page() fails; in the worst case, alloc_pages_bulk_array() calls alloc_page() after trying to get a batch, so I figured the worst case is still basically a loop calling alloc_page() and failing if it ever fails.
Reading up on it, though, arguably the memalloc_retry_wait() should already be in all the callsites, so maybe I should insert a patch in the middle that just adds the memalloc_retry_wait() into btrfs_alloc_page_array()? Since it's an orthogonal fixup to either the refactoring or the conversion to alloc_pages_bulk_array()?
Thanks!
Sweet Tea
| |